fix: Correctly resolve packages from preferences.packages.version #33

austreelis · 2025-10-03T12:46:27Z

austreelis commented

2025-10-03 12:46:27 +00:00

Before this, a value that does not correspond to an attribute under a
package alias' versions would make it default to the latest as defined
by lib.packages.getLatest, which isn't a silver bullt: it choose
"bash5.2.15-bootstrap" for instance instead of stage1-passthrough (and I
don;t think we have an easy fix for that).

Given values like "1.2.3" makes little sense in
config.preferences.packages.version (because it's a package-set-wide
preference), this means it was hardly ever useful.

before:

❯ nix eval -f tidepool --apply 't:
  t.extend {modules = [{config.preferences.packages.version = "latest";}];}
  |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash)
  |> (p: p.version)'
"5.2.15-bootstrap"
❯ nix eval -f tidepool --apply 't:
  t.extend {modules = [{config.preferences.packages.version = "stable";}];}
  |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash)
  |> (p: p.version)'
"5.2.15-bootstrap"

after:

❯ nix eval -f tidepool --apply 't:
  t.extend {modules = [{config.preferences.packages.version = "latest";}];}
  |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash)
  |> (p: p.version)'
"5.2.15-stage1-passthrough"
❯ nix eval -f tidepool --apply 't:
  t.extend {modules = [{config.preferences.packages.version = "stable";}];}
  |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash)
  |> (p: p.version)'
"5.2.15-bootstrap"

Before this, a value that does not correspond to an attribute under a package alias' `versions` would make it default to the latest as defined by `lib.packages.getLatest`, which isn't a silver bullt: it choose "bash5.2.15-bootstrap" for instance instead of stage1-passthrough (and I don;t think we have an easy fix for that). Given values like `"1.2.3"` makes little sense in `config.preferences.packages.version` (because it's a package-set-wide preference), this means it was hardly ever useful. before: ```console ❯ nix eval -f tidepool --apply 't: t.extend {modules = [{config.preferences.packages.version = "latest";}];} |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash) |> (p: p.version)' "5.2.15-bootstrap" ❯ nix eval -f tidepool --apply 't: t.extend {modules = [{config.preferences.packages.version = "stable";}];} |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash) |> (p: p.version)' "5.2.15-bootstrap" ``` after: ```console ❯ nix eval -f tidepool --apply 't: t.extend {modules = [{config.preferences.packages.version = "latest";}];} |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash) |> (p: p.version)' "5.2.15-stage1-passthrough" ❯ nix eval -f tidepool --apply 't: t.extend {modules = [{config.preferences.packages.version = "stable";}];} |> (t: t.config.lib.packages.resolve t.config.packages.foundation.bash) |> (p: p.version)' "5.2.15-bootstrap" ```

austreelis added 1 commit 2025-10-03 12:46:27 +00:00

fix: Correctly resolve packages from preferences.packages.version eee7a761ea

Before this, a value that does not correspond to an attribute under a
package alias' `versions` would make it default to the latest as defined
by `lib.packages.getLatest`, which isn't a silver bullt: it choose
"bash5.2.15-bootstrap" for instance instead of stage1-passthrough (and I
don;t think we have an easy fix for that).

Given values like `"1.2.3"` makes little sense in
`config.preferences.packages.version` (because it's a package-set-wide
preference), this means it was hardly ever useful. I've kept the behavior
allowing it still but I think it's not ever useful given of coarse grain
its impact is on resolution in package set.

srxl requested changes 2025-10-03 13:57:46 +00:00

Dismissed

tidepool/src/lib/packages.nix

					
				@ -243,11 +243,22 @@ in

				        in

				        outputs;

				      WELL_KNOWN_ALIASES = [

srxl commented

2025-10-03 13:57:20 +00:00

We should be able to dynamically compute this from the keys of lib.types.alias.getSubOptions {} and some filtering. Whether we want to do that is another question, though.

We should be able to dynamically compute this from the keys of `lib.types.alias.getSubOptions {}` and some filtering. Whether we want to do that is another question, though.

austreelis commented

2025-10-03 14:34:18 +00:00

I thought about it but:

the filtering needs to be updated whenever we touch something unrelated to those version aliases, but to types.alias (e.g. if we add some platform config in it, we'd need to filter those out too), so I thought it wasn't worth it
we could instead generate options for types.alias from those well-known version aliases, but then I think we might still need to manually define how their default gets plumbed together.

But I agree that getting rid of those notes would be nice :I

I thought about it but: - the filtering needs to be updated whenever we touch something unrelated to those version aliases, but to `types.alias` (e.g. if we add some platform config in it, we'd need to filter those out too), so I thought it wasn't worth it - we could instead *generate* options for `types.alias` from those well-known version aliases, but then I think we might still need to manually define how their default gets plumbed together. But I agree that getting rid of those notes would be nice :I

austreelis commented

2025-10-03 18:30:14 +00:00

Oh crap, I just realized types.platform also needs updating like types.alias does.

You know what, I'll work on that in a follow-up, this PR was supposed to be a fix. the user-facing behavior is fixed, I (or anyone, but I can) will improve the internals later, but there might be more untangling needed than I expected.

edit: added a note and opened #34

Oh crap, I just realized `types.platform` also needs updating like `types.alias` does. You know what, I'll work on that in a follow-up, this PR was supposed to be a fix. the user-facing behavior is fixed, I (or anyone, but I can) will improve the internals later, but there might be more untangling needed than I expected. edit: added a note and opened [#34](https://git.auxolotl.org/auxolotl/labs/issues/34)

srxl marked this conversation as resolved

tidepool/src/lib/packages.nix

					
				@ -251,0 +255,4 @@

				          prefered = config.preferences.packages.version;

				          isWellKnown = builtins.elem prefered lib.packages.WELL_KNOWN_ALIASES;

				        in

				        if isWellKnown then

srxl commented

2025-10-03 13:52:31 +00:00

I think this condition isn't quite right - if a well known alias is configured in preferences, it'll always trigger this branch no matter what. It's causing eval failures on my end when this function is called with actual packages instead of an alias (since packages don't have a latest attribute).

I think this condition isn't quite right - if a well known alias is configured in preferences, it'll *always* trigger this branch no matter what. It's causing eval failures on my end when this function is called with actual packages instead of an alias (since packages don't have a `latest` attribute).

austreelis commented

2025-10-03 14:36:16 +00:00

I think resolve is not supposed to be only used to resolve "latest" or "stable" aliases. I agree that it's only used through this mechanics as of now and so this branch must always be taken if not given a proper derivation, but I don't think the function was supposed to be limited to to that usecase ?

If we don't see another, I'm enclined to remove that behavior, I'd rather add it again later if needed.

I think resolve is not supposed to be only used to resolve "latest" or "stable" aliases. I agree that it's only used through this mechanics as of now and so this branch must always be taken if not given a proper derivation, but I don't think the function was supposed to be limited to to that usecase ? If we don't see another, I'm enclined to remove that behavior, I'd rather add it again later if needed.

srxl commented

2025-10-03 14:41:56 +00:00

I'm not sure what you mean here - the issue with this condition is that it's causing eval failures, ie.

       error: attribute 'latest' missing
       at /home/ruby/devel/auxolotl/labs/tidepool/src/lib/packages.nix:259:17:
          258|         if isWellKnown then
          259|           alias.${prefered}
             |                 ^
          260|         else if alias ? versions then

These happen because sometimes resolve is called with a package (ie. lib.types.package and not lib.types.alias). Only the latter type actually has latest or stable defined, and because the condition doesn't distinguish between the two types, it fails when it gets the former type.

I'm not sure what you mean here - the issue with this condition is that it's causing eval failures, ie. ``` error: attribute 'latest' missing at /home/ruby/devel/auxolotl/labs/tidepool/src/lib/packages.nix:259:17: 258| if isWellKnown then 259| alias.${prefered} | ^ 260| else if alias ? versions then ``` These happen because sometimes `resolve` is called with a package (ie. `lib.types.package` and not `lib.types.alias`). Only the latter type actually has `latest` or `stable` defined, and because the condition doesn't distinguish between the two types, it fails when it gets the former type.

austreelis commented

2025-10-03 18:19:04 +00:00

Oh my bad, seriously misread your comment 😅

I think what I meant to do was if isWellKnown && alias ? ${prefered} then, that's a mistake I think, thanks !

edit: pushed the change, passing a package directly doesn't fail anymore :3

Oh my bad, seriously misread your comment 😅 I think what I meant to do was `if isWellKnown && alias ? ${prefered} then`, that's a mistake I think, thanks ! edit: pushed the change, passing a package directly doesn't fail anymore :3

srxl marked this conversation as resolved