pkgs/top-level: make package sets composable (reapply)#376988
pkgs/top-level: make package sets composable (reapply)#376988wolfgangwalther merged 3 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
6.topic: nixos
wolfgangwalther
force-pushed
the
compose-pkgs-sets-reapply
branch
from
d87d7e7 to
d3716ed
Compare
github-actions
bot
added
10.rebuild-darwin: 0
philiptaron
added
the
1.severity: significant
…form,hostPlatform} write only
The description for options.nixpkgs.system already hints at this:
Neither ${opt.system} nor any other option in nixpkgs.* is meant
to be read by modules and configurations.
Use pkgs.stdenv.hostPlatform instead.
We can support this goal by not elaborating the systems anymore, forcing
users to go via pkgs.stdenv.
This will prevent problems when making the top-level package sets
composable in the next commit. For this to work, you should pass a fully
elaborated system to nixpkgs' localSystem or crossSystem options.
The commit to make package sets composable depends on the fact that localSystem and crossSystem can be passed forward to lower package sets for composition. Once we pass a fully elaborated system, this will break down - getters like "isStatic", "isMusl" etc. will not change with the package set anymore, but be stuck on the value passed in via those options. This is a limitation of lib.systems.elaborate primarily, because it can't deal with arbitrary overrides properly. freshBootstrapTools on darwin used to do this, although in this case it didn't do any harm: There shouldn't be any package sets composed during bootstrap anyway. Refactor it, to avoid throwing the assert.
wolfgangwalther
force-pushed
the
compose-pkgs-sets-reapply
branch
from
d3716ed to
434e36a
Compare
|
I don't really want to delay this PR much more, because the merge conflicts last time, after a new package set had been added, were annoying. Since I don't seem to get any feedback on this, I'll need to handle this to the best of my knowledge. I'm sure I fixed the problem with failing tests, that lead to the revert. I'm fairly sure my changes to the nixos/nixpkgs module make sense. My best course of action right now seems to be to merge and hope it doesn't need to be reverted again. I rebased this on latest master again, to confirm that eval still passes. I intend to merge this in the next couple of days, either tomorrow or Monday. |
| # Make sure that the final value has all fields for sake of other modules | ||
| # referring to this. TODO make `lib.systems` itself use the module system. | ||
| apply = lib.systems.elaborate; |
There was a problem hiding this comment.
Please add this back (or something like it), so that the type of config.nixpkgs.hostPlatform is always a platform attrset.
There was a problem hiding this comment.
So, if we do that, then we can't do any of this PR, essentially.
Citing from the commit message:
The description for options.nixpkgs.system already hints at this:
Neither ${opt.system} nor any other option in nixpkgs.* is meant
to be read by modules and configurations.
Use pkgs.stdenv.hostPlatform instead.We can support this goal by not elaborating the systems anymore, forcing
users to go via pkgs.stdenv.
It seems the intent of the apply = elaborate was, that consumers could look at config.nixpkgs.hostPlatform and use it for downstream logic... but the comment cited above already says otherwise.
Also, almost every consumer already depended on stdenv.hostPlatform instead - except for two tests. I think this makes more sense.
What's your reasoning to not do it like this / what am I missing?
| if lib.systems.equals elaborated cfg.hostPlatform then | ||
| cfg.hostPlatform # make identical, so that `==` equality works; see https://github.com/NixOS/nixpkgs/issues/278001 | ||
| else | ||
| elaborated; |
| } | ||
| { | ||
| assertion = | ||
| (opt.hostPlatform.isDefined -> builtins.isAttrs cfg.buildPlatform -> !(cfg.buildPlatform ? parsed)) |
There was a problem hiding this comment.
cfg.buildPlatform must always be attrs.
There was a problem hiding this comment.
Well, nixos/modules/misc/nixpkgs/test.nix says otherwise - it sets nixpkgs.buildPlatform to different strings, too.
There was a problem hiding this comment.
cfg.buildPlatformmust always be attrs.it sets
nixpkgs.buildPlatformto different strings, too.
It could be defined as a string, but previously the apply function would always coerce the final value into attrs.
There was a problem hiding this comment.
I guess what we really need here is a lib.systems.partiallyElaborate, so that the apply function can be restored without violating the purpose of this PR?
There was a problem hiding this comment.
The important part is, that it must be possible to pass the inputs to the module (localSystem, crossSystem, buildPlatform, hostPlatform) onwards unchanged.
No matter what you do as apply, it will always break this premise, as soon as a second layer is involved. I tried adding rawValue in eec2100 to avoid touching apply, but that didn't work out.
But there is really no need to be able to access config.nixpkgs.buildPlatform etc. at all - you can get that information from pkgs.stdenv.buildPlatform etc, right?
There was a problem hiding this comment.
But there is really no need to be able to access
config.nixpkgs.buildPlatformetc. at all - you can get that information frompkgs.stdenv.buildPlatformetc, right?
It seems that up until now, it was intended for users to be able to read those options. But I agree, accessing pkgs.stdenv is better (and is what I do in my configs).
I'm trying to think about whether there's a sane way to enforce that though...
Obviously, the option descriptions should be updated with a note explaining they are write-only, and pkgs.stdenv should be read instead.
Perhaps an apply function could warn or throw when evaluated (similar to mkRemovedOptionModule), but internal usage could bypass the apply by merging the option definitions manually?
# Bypass `apply` function by merging `foo` manually:
(
lib.modules.mergeDefinitions
options.foo.loc
options.foo.type
options.foo.definitionsWithLocations
).mergedValue
|
Oh, you already merged this?? |
Yeah, sorry, didn't know you were going to review. |
|
This broke the |
Ah. I only grepped for |
This seems to work for me: nixpkgs = {
hostPlatform = {
inherit (pkgs.stdenv.hostPlatform) system;
};
};I can open a PR tomorrow, if you don't beat me to it, @nikstur. |
|
@wolfgangwalther Hi, this line seems to be problematic, causing a build failure, when run(not sure if that's a right word as a non-native and not knowing much about Nix) by |
It could very well be that out-of-nixpkgs projects need adjustments, too. I don't specifically know about nix-darwin. @emilazy could you have a look? |
|
To chime in I rather this be iterated on than reverted again if my opinion matters at all :p |
- Pin nixpkgs to work around NixOS/nixpkgs#376988 - Remove custom aerospace HM module since it's now in core
|
Hi, this broke some downstream consumers which access config.nixpkgs.hostPlatform.system, which now no longer exists because the |
|
If the interface change is intended, it should get a release-notes entry. Clearly, this isn't easy to fix, so I would like to see this reverted again and properly thought over. |
It is:
You should be able to replace that with |
|
People make a lot of effort to not break the interface, since it causes trouble for all users and downstream developers when their configs simply error out on evaluation. I appreciate the goal here, but this could have been accomplished without breaking the interface, by leaving config.nixpkgs.hostPlatform be an elaborated system config but simply adding a second option next to it, which inherits the definitions from options.nixpkgs.hostPlatform without an "apply", and use that to instantiate the pkgs. |
|
Yep, this PR should never have been merged without a release note. No matter how trivial, if it requires manual interference, then it should have notes. My flake broke in 3 different lines, and I would have to use GitHub 💩-search to know why if I wasn't pinged. The assertion isn't enough, I saw people in random groups asking where did that error come from (since the backtrace points to a random place). Searching the assertion message in GH points to this PR right now, but in the future it will get lost between the high number of issues+PRs within this repo. |
|
I have prepared a revert to limit the disruption As said, that's not a value judgement; just balancing the interests of users. To summarize our findings and requirements so far:
These are competing requirements, but not mutually exclusive. |
|
Currently taking a short break from Nixpkgs, but since we were affected by this in nix-darwin: I’m afraid I do think a revert is the best option here. I’m generally not all that fussed about backwards compatibility, but in this case we’ve broken basically every third‐party user of the module system that instantiates Nixpkgs without any warning period or even a helpful error message. Over 50 people were confused by their nix-darwin systems suddenly breaking with an obscure error message over the past couple days, and I believe Home Manager is still broken. Breaking changes are often necessary and we’re happy to embrace them on the nix-darwin end, but this one wasn’t very graceful. I do agree that the whole system elaboration procedure conflates inputs and outputs in a problematic way. I would like to see a world where we have a clear delineation between stuff the Nixpkgs user supplies to specify the desired platform and derived properties that are inferred from that specification. I’m not sure if this quite solves that problem, though; I’m worried about the idea that we should be reducing platforms to their double string when reading and adjusting them, which seems like it might get in the way of respecting platforms with the same system double string but more obscure settings changed and even reduce compositionality as a result?
I’m also a bit concerned that this incorrect replacement is being suggested: I think the correct construction is |
|
First of all, I'd like to apologize to everyone who was affected by this PR. I hope to learn from this for the future. Many things went wrong, but the biggest thing for me was, that I didn't know about those downstream copies of Most other things that went wrong.. I should have already known better.
@roberth thank you for making the call. In the last couple of days, I wasn't able to convince, and was still torn on it, myself. I still believe the change itself is right and it's essentially impossible to avoid breakage, but I do accept that the way this has happened was not OK.
Agreed. I somehow had the impression that release notes would become mostly relevant for.. the next release, so I had assumed there to be time to write them - but clearly people are reading release notes while tracking unstable already.
Agreed.
I think it is and always was the case, right?
That's the right approach, yes.
Those will be very hard to achieve. In fact, I already found fixing My conclusion regarding composability of package sets: I won't pursue this again for now, because the base on the NixOS side is just not there, yet. We can't do it.
Reducing things to their system double is actually not the idea. I somehow proposed that, because it seems like a good-enough fix for most cases, especially on the NixOS-side: When running NixOS, you are much less likely to run obscure settings compared to when compiling via The proper way to do it: You'll need to collect the inputs exactly and pass them forward. Not only the system double. That's the very reason why I am so opposed to Passing on the inputs is also what I did in the reverted main commit of this PR, about the package sets. This worked very well and fixed all the package sets eventually. One other thing that I proposed was this:
This is just wrong. There's two cases we need to look at:
My generic suggestion in the comment above was even worse, because it wasn't even clear which of the two cases applied here. |
The problem is actually even worse: most of the time when we want the system/platform "inputs", we don't necessarily want the platforms configured via the nixpkgs module. Rather we want the inputs passed to whatever nixpkgs instance happens to be in use by the module configuration. In the strictest sense, the nixpkgs instance is the module arg config._module.specialArgs.pkgs
or config._module.args.pkgsTo clarify: there is no guarantee that the On that basis, perhaps reading those options should produce a warning, clarifying that users probably want to read Additionally, if we need access to the un-elaborated input platform args, then these should be exposed as attrs on the nixpkgs instance, so that we could do something like: pkgs.stdenv.hostPlatformInputOr pkgs.input.hostPlatformOr something along those lines. I'm more familiar with the nixpkgs module and I'm unfamiliar with pkgs/top-level, so I don't know the best approach to implementing something like that. Finally, the issue of warning when an elaborated platform is assigned to the platform module options and/or asserting that a non-elaborated platform is passed to the top-level nixpkgs constructor. I wonder if the best approach here is to have |
I don’t know about NixVim, but you can think of (a subset of) nix-darwin as being a port of (a subset of) NixOS to macOS. A good number of our modules are copied from NixOS, adjusted to work on macOS and with the rest of our modules. In the case of the Nixpkgs module, we could probably import it directly from NixOS, but in other cases this is often not directly possible (e.g. porting things from systemd to launchd) and even in the case of the Nixpkgs module there are some differences (we never had the deprecated In that context, I think the copy‐paste is understandable; although the Nixpkgs module is one of the very few (or the only?) we could potentially reuse from NixOS wholesale, almost every other module that derives from NixOS code requires adjusting to work with a different OS that is not even entirely managed by nix-darwin.
There is also the Breaking changes announcement for unstable Discourse thread, FWIW.
I hope you don’t get discouraged from working to make fundamental improvements. Not hesitating to roll back changes that cause problems shouldn’t lead us into stagnation and conservatism, because it means we have a good backup plan if something goes wrong. In general I think we should be more proactive to revert and reassess when changes cause problems, because otherwise we hold back evolution until we’re overly confident, precisely because we think we won’t quickly revert if it goes wrong. (Though I realize that in this case we’d already been through a cycle of that before it hit the channels and broke things on a wider scale, and that can be demoralizing.)
Right. By “the inputs”, do you mean the raw value of Perhaps we want three layers: a convenience form for input, a “canonical” input form that has a consistent structure but contains values that can be varied freely and are as decoupled as possible, and the output form that is only for consumption rather than modification. (FWIW, it may be that every time we want to “tweak the platform” we’re actually making a mistake, as @Ericson2314 alluded to; it’s a handy operation but perhaps not always a sensible one. The top‐level package sets do have the feeling of being a bit of a convenience hack to me – nice for quick use of cross‐compilation or alternative toolchains/libraries on the command‐line, but hard to define in a principled way – and they’re costly to instantiate which means we have to discourage their use within Nixpkgs. Unfortunately i don’t know what the right solution to all of this is. I’d really like it if we could use cross‐compilation more freely in Nixpkgs. I feel like for that case you usually want to wipe the slate mostly clean – e.g. it makes no sense to try and preserve the preference of Musl when instantiating a Nixpkgs with a macOS host platform – though you can also point to examples where the intuition points the opposite way.)
My feeling is that it might be nice if we can arrange it so that |
I am still motivated to fix the package sets, I just didn't see a way to fix the NixOS side to make this work... ... but that might change with my idea based on what @MattSturgeon proposed here:
I'm currently thinking about whether we could add those input platform args to the elaborated system itself. This would kind of turn everything we discussed in this PR upside down. If I was able to reconstruct the input args from the elaborated system itself... would all other pieces fall magically into place? Everything I dealt with for package set composability and everything we discussed here for nixpkgs arguments... was all under the premise that this is about "input to nixpkgs". What if we treat this as "input to elaborate" instead? Assume:
The status-quo is, that pkgs/top-level doesn't really want to be passed elaborated systems, but the nixos/nixpkgs module did so anyway. I was dead set on adjusting nixos/nixpkgs to stop doing that. But could the above allow the opposite i.e. make pkgs/top-level work with elaborated input, because the "original input" is preserved inside of that? Only an idea, so far, but with the test-suite I added for package set composition, this should be easy to verify for correctness, once implemented. Edit2: One thing that would need to be changed first... all of |
For nixvim, the main reasons we copy-paste the module is to have custom documentation (descriptions, defaults, examples), and to avoid needing a
I had the same thought earlier. It could be non-circular with some hacks:
Makes sense to me.
Interesting 👀 I like this, conceptually.
From my perspective losing referential equality may be worth it. We could retain it, but at the cost of losing the input values for |
Ah, right. But if the systems otherwise are considered equal, would we really lose by setting |
|
I think fixing all uses of |
That's reasonable. Would it be acceptable to lose Otherwise I think we'd need to expose/preserve the inputs separately from the elaborated platforms. Which will be more difficult and uglier.
Retaining only one set of inputs when the outputs are identical also sounds reasonable to me, but idk if I'm missing any edge-cases. If it is sufficient, it'll definitely simplify things. |
13 participants
This was merged in #303849 and reverted in #376899, because of #303849 (comment).
Turns out the nixos/nixpkgs module still wasn't happy with what I tried in eec2100. Thus, I removed that commit out of the reapplication and am trying again:
The composability of nixpkgs depends on not passing elaborated systems into
localSystemandcrossSystem. Once you pass in an elaborated system, you can't use any of the package sets likepkgsStaticanymore. This is due to howlib.systems.elaborateworks - it can't handle arbitrary fields properly when overriding and will return garbage, i.e. certain fields out of sync. For example, you can quickly end up withlibc == musl, butisMusl == falseetc.My first attempt on the nixos side was to store the original arguments for localSystem/crossSystem etc. and pass those on. After investigating more, however, I think a cleaner approach is to just not elaborate the systems via
applyfor the nixos module anymore. The module had self-contradictory statements in this regard:nixpkgs/nixos/modules/misc/nixpkgs.nix
Lines 202 to 204 in f706899
and
nixpkgs/nixos/modules/misc/nixpkgs.nix
Lines 310 to 313 in f706899
In the first commit here, I am getting rid of the elaboration, making it easier to pass those values on to new incarnations of nixos without accidentally passing on elaborated systems.
I would appreciate a review of the nixos/nixpkgs module side. Also touching the nixpkgs/read-only module, which @roberth introduced.
Things done
Couldn't figure out how to run e.g. all tests from nixos/release-small locally.
How do I do that?
Add a 👍 reaction to pull requests you find important.