Skip to content
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

Merged
merged 3 commits into from Mar 13, 2020

Conversation

thefloweringash
Copy link
Member

@thefloweringash thefloweringash commented Feb 28, 2020

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 and nixpkgs.crossSystem, so reviews and tests appreciated.

Fixes #80806

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

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
@grahamc
Copy link
Member

grahamc commented Feb 28, 2020

Can you be more specific about what problem this is fixing?

@thefloweringash
Copy link
Member Author

I somehow omitted the issue on first posting. I've added it now. It's aimed at #80806.

@grahamc
Copy link
Member

grahamc commented Feb 28, 2020

This is amazing, I've never heard of nesting.

@worldofpeace
Copy link
Contributor

This is amazing, I've never heard of nesting.

Hehe, it's very useful to have an escape hatch configuration.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg test nesting

@worldofpeace worldofpeace added this to the 20.03 milestone Feb 28, 2020
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 28, 2020

I also tested i686-linux. I have no idea how this fixed it: anyone care to explain it?

@thefloweringash
Copy link
Member Author

NixOS evaluation starts by calling nixos/lib/eval-config.nix. This takes a system parameter that defaults to builtins.currentSystem. The implementation of nesting.clone and nesting.children is based on calling this entry point again to evaluate an entirely separate NixOS, and then linking the result into the parent.

The problem was that the nested calls to the entry point did not set the system argument when calling eval-config.nix, which caused it to default to builtins.currentSystem. All nested configurations were built for the same system as the evaluating system. To fix this, it was sufficient to set the system to the same value as the top-level eval-config.nix, which is stored in config.nixpkgs.system.

There's an added complication in that the system values are encoded in the pkgs argument, which is derived from config.nixpkgs.system, config.nixpkgs.localSystem, config.nixpkgs.crossSystem, and config.nixpkgs.pkgs (for details, see nixos/modules/nixpkgs.nix). Since there's no actual place where the value of system argument to eval-config.nix value is preserved, it's possible that some configurations will be surprising. In particular, config.nixpkgs.system will be inherited by all nested configurations, but the other parameters will not. This distinction likely only applies to nesting.children, which try not to inherit the parent configuration.

To prevent config.nixpkgs.system from being inherited unexpectedly, we could add a new internal option that stores the initial system passed to eval-config.nix, and use that for the nested calls.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 29, 2020

Thank you, that was a very clear explanation.

This distinction likely only applies to nesting.children, which try not to inherit the parent configuration.

Right, this change could cause some subtle problem, though I think the use of nesting.children is fairly uncommon.

the initial system passed to eval-config.nix, and use that for the nested calls.

That is because when top-level.nix is being evaluated the system argument stored in config.nixpkgs.system could have been override by the user configuration.nix, right?

@thefloweringash
Copy link
Member Author

the initial system passed to eval-config.nix, and use that for the nested calls.

That is because when top-level.nix is being evaluated the system argument stored in config.nixpkgs.system could have been override by the user configuration.nix, right?

Yeah.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 4, 2020

@thefloweringash can we go forward the internal option solution or do you have other plans?

@thefloweringash
Copy link
Member Author

@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

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 4, 2020

@GrahamcOfBorg test nesting nginx gitea

Copy link
Contributor

@rnhmjoj rnhmjoj left a 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.

Copy link
Member

@nbp nbp left a 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.)

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 5, 2020

@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.
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 13, 2020

@GrahamcOfBorg test nesting nginx gitea

@rnhmjoj rnhmjoj merged commit 7b15d6c into NixOS:master Mar 13, 2020
@thefloweringash thefloweringash deleted the nesting-system branch March 13, 2020 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nesting.clone is broken on non-x86_64
5 participants