New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/activation: propagate system to nested configurations #81241
Conversation
The current behavior lets `system` default to `builtins.currentSystem`. The system value specified to `eval-config.nix` has very low precedence, so this should compose properly. Fixes NixOS#80806
Can you be more specific about what problem this is fixing? |
I somehow omitted the issue on first posting. I've added it now. It's aimed at #80806. |
This is amazing, I've never heard of |
Hehe, it's very useful to have an escape hatch configuration. |
@GrahamcOfBorg test nesting |
I also tested i686-linux. I have no idea how this fixed it: anyone care to explain it? |
NixOS evaluation starts by calling The problem was that the nested calls to the entry point did not set the There's an added complication in that the system values are encoded in the To prevent |
Thank you, that was a very clear explanation.
Right, this change could cause some subtle problem, though I think the use of
That is because when |
Yeah. |
@thefloweringash can we go forward the internal option solution or do you have other plans? |
I've pushed an implementation of using the internal option |
@GrahamcOfBorg test nesting nginx gitea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me but I'm not an expert on this matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good to me.
At one moment I was wondering whether the nested.clone
got the correct nixpkgs.system
value, but as it is inheriting the same modules, it should be overwritten the same way.
Thanks!
(PS: I would not have noticed this issue without seeing the notification on IRC.)
@nbp Thank you for the review! @thefloweringash could you squash the last two commit? |
This avoids a possible surprise if the user is using `nixpkgs.system` and `nesting.children`. `nesting.children` is expected to ignore all parent configuration so we shouldn't propagate the user-facing option `nixpkgs.system`. To avoid doing so, we introduce a new internal option for holding the value passed to eval-config.nix, and use that when recursing for nesting.
f267b48
to
ce41677
Compare
@GrahamcOfBorg test nesting nginx gitea |
Motivation for this change
Fix the nesting tests for non-x86_64 on hydra, and the other tests that depend on nesting like nginx.
I'm not confident that this composes correctly with
nixpkgs.localSystem
andnixpkgs.crossSystem
, so reviews and tests appreciated.Fixes #80806
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)