lib.makeAlias: init with by-name support#442066
lib.makeAlias: init with by-name support#442066wolfgangwalther wants to merge 6 commits intoNixOS:masterfrom
Conversation
|
Thanks, I will take a look at this soon. I expect to have many opinions :) |
|
Looks really promising |
f893021 to
a174751
Compare
|
Latest push makes performance ( |
This allows adding more error conditions easily.
This is not a scope, so the callPackage protection doesn't apply. Also, since these aliases are defined in the `linuxKernel.kernels` attribute set and not at the top-level, the inherited top-level aliases are not protected against inclusion in callPackage either. For this to work, these aliases would just need to be moved to the top-level instead. However, evaluation with and without config.allowAliases will be the same here, providing the same safety against actually using these attributes. Also, of course, they have metadata. This is mostly to show how a package set could use `lib.mkAlias` - it will work the same in a proper scope.
a174751 to
9a9f61e
Compare
|
Also added an example how to use |
emilazy
left a comment
There was a problem hiding this comment.
As you know, I am very much in favour of the general approach :) This doesn’t quite look like what I hope for yet, but I am optimistic we can get there.
It would be nice to see pkgs/test/default.nix adapted for this as an example of how you can consume the structured information here.
| # These need to be abort so they can't be caught with `builtins.tryEval`, | ||
| # which is used by nix-env and CI to filter out packages that don't evaluate. | ||
| # This way we're forced to fix such errors in Nixpkgs. | ||
| if aliasArgs != { } then | ||
| abort "lib.customisation.callPackageWith: ${aliasError}" |
There was a problem hiding this comment.
If we made aliases become throws when !config.allowAliases, would we need this? i.e., they would be filtered out by CI, but would it also just filter out packages that use those aliases? (Just thinking about if we can minimize the number of places that have to know about aliases, for both cleanliness and performance.)
There was a problem hiding this comment.
It would still be possible to add aliases to the package arguments, if eval doesn't hit them.
We had this case for python3Packages, where this was already tried - somone tried to somehow override python3Packages in the python scope with aliases turned on or so, to prevent this from being used - but it didn't work, because Ci is swallowing those errors.
This could be hacked by putting abort in that place, I guess - but here I just demonstrated how we could use the structured alias information to implement such a check. This has a tiny effect on performance, so we can also entirely leave this part out, if we want to.
| else | ||
| mapAttrs mkAttrOverridable pkgs; | ||
|
|
||
| makeAlias = |
There was a problem hiding this comment.
Nit: We should call this mkAlias for consistency with mkDerivation, I think.
Also, I feel that “alias” may be a confusing word in general. llvmPackages = llvmPackages_21; is an “alias”, but it’s distinct from both the end‐user aliases.nix aliases we have for convenience (like llvmPackages_latest), temporary compatibility aliases, and from removed‐package throws. I do not know what a better word would be; maybe having helper functions will reduce the need here.
There was a problem hiding this comment.
Since I started in customisation.nix, I had makeOverridablenext door, so... but yeah, I'm fine with mkX, too.
| { | ||
| type, | ||
| package ? null, | ||
| reason ? null, |
There was a problem hiding this comment.
It would be nice to support multiple reasons, if we can ensure the interface remains convenient. I also think that we should ensure the reasons are structured by default rather than free‐form strings, because our existing throws are wildly inconsistent, and it’s much more convenient to write unmaintained than unmaintained in Nixpkgs but can give the wrong idea about upstream status.
Vague draft: reason.broken = true; reason.unmaintained = true; → “it was broken and unmaintained in Nixpkgs”? And then we could also have reason.insecure = true; or reason.insecure = ["CVE1" "CVE2"]; or such, and of course reason.custom = "I just really don’t like it";.
| reason ? null, | ||
| release ? null, | ||
| }: | ||
| assert type == "alias"; |
There was a problem hiding this comment.
Seems like busywork to have to set this? It should be relatively low‐ceremony to stub out a package. lib.optionalAttrs (!config.allowAliases) is already annoying.
There was a problem hiding this comment.
Well, that's the part that we need to keep the typing a bit stronger than freeform. If we ever want to extend this to other things like scopes or so, we better have type here, so we're not "an attrset means alias" deadend.
| self.${package} | ||
| else if release != currentRelease then | ||
| if package == null then | ||
| abort "Throw for '${name}' was added in ${release}. The alias must now be removed." |
There was a problem hiding this comment.
A warning seems like it may be better? Then you can scoop them all up at once, and perhaps do something with unsafeGetAttrPos to list all the files that need changing.
| { | ||
| type = "alias"; | ||
| release = "25.11"; | ||
| package = "opentimelineio"; | ||
| } |
There was a problem hiding this comment.
Ah… so the reason for the mandatory type = "alias"; is that we don’t have a wrapper function here at all.
But this is no good: you can have fooFull replaced with foo.override { withStuff = true; } and this does happen.
I know this ties in to supporting more non‐package stuff in by-name, but I think this would at least have to be package = { opentimelineio }: opentimelineio; or something. (Also, we should probably call it target or something rather than package, because we can have aliases for package sets too.)
Is the idea that this makes .override work properly, compared to { mkAlias, opentimelineio }: mkAlias { release = "25.11"; target = opentimelineio; }? Or is there a more fundamental reason that won’t work with this approach?
There was a problem hiding this comment.
So there are a couple of things going on here:
- We can't
callPackage, because then the "argument is an alias" check won't work - it hits infinite recursion, because we have no way of telling whether something is an alias before we docallPackage. If we're happy to give up on that check, which I am not sold on, yet, we can probably also put this behindcallPackage. - If we wanted to have overrides, we could still have this as an attrset, but have a function inside, like
override = { foo, bar }: bar.override { foo }or whatever (don't worry the details, just meant to say that we can have a function in the attrset, too) - Yes, this will cause fewer complications with
.overrideif we want overriden aliases to be done via this system as well.
| type, | ||
| package ? null, | ||
| reason ? null, | ||
| release ? null, |
There was a problem hiding this comment.
I have a mild preference for since over release here – it seems less ambiguous that it’s when the alias was added, rather than when it’s due for complete removal.
| pgroonga = throw "'pgroonga' has been removed. Use 'postgresqlPackages.pgroonga' instead."; # Added 2025-07-19 | ||
| pgtap = throw "'pgtap' has been removed. Use 'postgresqlPackages.pgtap' instead."; # Added 2025-07-19 | ||
| plv8 = throw "'plv8' has been removed. Use 'postgresqlPackages.plv8' instead."; # Added 2025-07-19 | ||
| postcss-cli = throw "postcss-cli has been removed because it was broken"; # added 2025-03-24 |
There was a problem hiding this comment.
Is it intentional that this was removed in this PR without replacement?
There was a problem hiding this comment.
Yes, because it's exactly the case that would hit abort. The throw as added in 2025-03, so needs to be removed in 25.11 - so right now. This is done in separate commits, it is first moved to an alias, which is then confirmed to hit the abort - and then later removed in a second commit to clean that up. This shows how this workflow would work at branch-off.
| # and ideally https://github.com/NixOS/nix/pull/8895 | ||
| self: super: | ||
| { | ||
| _isAlias = name: packages.${name}.type or null == "alias"; |
There was a problem hiding this comment.
I think we should use double underscores for this, to match __spliced and the like.
There was a problem hiding this comment.
I think double underscores are wrong, my understanding is that double underscores mean "internal to Nix", while single underscore means "internal to Nixpkgs". __spliced is just.. bad.
| }; | ||
| }; | ||
| in | ||
| (lib.mapAttrs (lib.makeAlias kernels) aliases) // { _isAlias = name: aliases ? ${name}; } |
There was a problem hiding this comment.
Yuck. This is too much ceremony – I think we have to find something better here. Whether that’s just defining these under an __aliases sub‐attribute‐set, or having these in line as mkAlias calls adjacent to the regular packages, or whatever. (I haven’t reviewed the implementation strategy enough to predict the issues these will cause, but I think this is no good, and I believe we can do better by making our scope creation functions etc. smarter.)
There was a problem hiding this comment.
Yes, I am not happy with this, yet, either. I added this to have a look at how this would look like... but yeah, don't like it, yet.
My current idea would be: These kinds of aliases don't need to support non by-name structures at all. Let's implement them with by-name in mind, and then work on migrating everything to by-name. This renders this case void.
wolfgangwalther
left a comment
There was a problem hiding this comment.
Thanks for your feedback, so far.
| # These need to be abort so they can't be caught with `builtins.tryEval`, | ||
| # which is used by nix-env and CI to filter out packages that don't evaluate. | ||
| # This way we're forced to fix such errors in Nixpkgs. | ||
| if aliasArgs != { } then | ||
| abort "lib.customisation.callPackageWith: ${aliasError}" |
There was a problem hiding this comment.
It would still be possible to add aliases to the package arguments, if eval doesn't hit them.
We had this case for python3Packages, where this was already tried - somone tried to somehow override python3Packages in the python scope with aliases turned on or so, to prevent this from being used - but it didn't work, because Ci is swallowing those errors.
This could be hacked by putting abort in that place, I guess - but here I just demonstrated how we could use the structured alias information to implement such a check. This has a tiny effect on performance, so we can also entirely leave this part out, if we want to.
| else | ||
| mapAttrs mkAttrOverridable pkgs; | ||
|
|
||
| makeAlias = |
There was a problem hiding this comment.
Since I started in customisation.nix, I had makeOverridablenext door, so... but yeah, I'm fine with mkX, too.
| reason ? null, | ||
| release ? null, | ||
| }: | ||
| assert type == "alias"; |
There was a problem hiding this comment.
Well, that's the part that we need to keep the typing a bit stronger than freeform. If we ever want to extend this to other things like scopes or so, we better have type here, so we're not "an attrset means alias" deadend.
| else | ||
| abort "Warning for '${name}' was added in ${release}. The alias must now be converted to a throw." |
There was a problem hiding this comment.
I consider Nixpkgs versions mostly freeform, so we can only say "still matches the current release" or "was added in a previous release".
Now, if we don't want to stick with these forever, then we need two points where things change:
- One from alias+warning to error
- One removing the error
If we only have one piece of information, we can't identify both.
To be able to identify both, we must put "structure" into the Nixpkgs version, too. Aka we must give it a semantic meaning, saying "25.11" is "25.05 + 1" and "26.05" is "25.05 + 2". We can do that, but I'm not sure if and how, yet.
| abort "Warning for '${name}' was added in ${release}. The alias must now be converted to a throw." | ||
| else if package == null || !self.config.allowAliases then | ||
| { | ||
| type = "error"; |
There was a problem hiding this comment.
The idea is to say we already have type = "derivation", so let's stick to that?
Aka we never want something like a derivation with a different _type. So arguably _type would weaken the typing.
| else if package == null || !self.config.allowAliases then | ||
| { | ||
| type = "error"; | ||
| meta = throw "'${name}' was removed because it was ${reason}."; |
There was a problem hiding this comment.
meta is specifically for lib.meta.availableOn to fail.
(we should probably also add __toString, just to make sure)
| { | ||
| type = "alias"; | ||
| release = "25.11"; | ||
| package = "opentimelineio"; | ||
| } |
There was a problem hiding this comment.
So there are a couple of things going on here:
- We can't
callPackage, because then the "argument is an alias" check won't work - it hits infinite recursion, because we have no way of telling whether something is an alias before we docallPackage. If we're happy to give up on that check, which I am not sold on, yet, we can probably also put this behindcallPackage. - If we wanted to have overrides, we could still have this as an attrset, but have a function inside, like
override = { foo, bar }: bar.override { foo }or whatever (don't worry the details, just meant to say that we can have a function in the attrset, too) - Yes, this will cause fewer complications with
.overrideif we want overriden aliases to be done via this system as well.
| pgroonga = throw "'pgroonga' has been removed. Use 'postgresqlPackages.pgroonga' instead."; # Added 2025-07-19 | ||
| pgtap = throw "'pgtap' has been removed. Use 'postgresqlPackages.pgtap' instead."; # Added 2025-07-19 | ||
| plv8 = throw "'plv8' has been removed. Use 'postgresqlPackages.plv8' instead."; # Added 2025-07-19 | ||
| postcss-cli = throw "postcss-cli has been removed because it was broken"; # added 2025-03-24 |
There was a problem hiding this comment.
Yes, because it's exactly the case that would hit abort. The throw as added in 2025-03, so needs to be removed in 25.11 - so right now. This is done in separate commits, it is first moved to an alias, which is then confirmed to hit the abort - and then later removed in a second commit to clean that up. This shows how this workflow would work at branch-off.
| # and ideally https://github.com/NixOS/nix/pull/8895 | ||
| self: super: | ||
| { | ||
| _isAlias = name: packages.${name}.type or null == "alias"; |
There was a problem hiding this comment.
I think double underscores are wrong, my understanding is that double underscores mean "internal to Nix", while single underscore means "internal to Nixpkgs". __spliced is just.. bad.
| }; | ||
| }; | ||
| in | ||
| (lib.mapAttrs (lib.makeAlias kernels) aliases) // { _isAlias = name: aliases ? ${name}; } |
There was a problem hiding this comment.
Yes, I am not happy with this, yet, either. I added this to have a look at how this would look like... but yeah, don't like it, yet.
My current idea would be: These kinds of aliases don't need to support non by-name structures at all. Let's implement them with by-name in mind, and then work on migrating everything to by-name. This renders this case void.
|
Not going to pursue this anymore myself. |
WIP for "structured aliases in by-name".
Features:
by-name/.x.type != "error"orlib.isDerivation xcallPackageprevents alias arguments.lib.meta.availableOn ... pkgs.<alias>fails, too, if somebody works around thecallPackagelimitation.The first commit converts some aliases - two of these fail CI immediately, The last two commits fix that.
TODO:
The design of "by-name returns an attrset" is extensible. I'm thinking we could represent package sets in by-name in the future with something like
type = "scope"or so - working towards unified APIs / tools / design of package sets.Supersedes #441492. More discussion in #441108.
WDYT, @emilazy?
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.