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.

Sign up for GitHub

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

Jump to bottom

Make pkgs.nixos and pkgs.nixosTest pure wrt NIXOS_EXTRA_MODULE_PATH #51852

Closed
wants to merge 2 commits into from
Closed

Make pkgs.nixos and pkgs.nixosTest pure wrt NIXOS_EXTRA_MODULE_PATH #51852

wants to merge 2 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Dec 10, 2018

Motivation for this change

Includes #51850

This avoids the NIXOS_EXTRA_MODULE_PATH impurity in NixOS when
Nixpkgs is invoked 'purely' via pkgs/top-level/default.nix, but
passed the impurity along if invoked impurely via /default.nix
or pkgs/top-level/impure.nix.

pkgs.nixosTests remains impure by necessity until a possible future
optimization that will make it call pkgs.nixosTest, therefore
inheriting its behavior.

This duplicates the default in a number of places because these files are potentially public interfaces.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date (NIXOS_EXTRA_MODULE_PATH has minimal documentation in NixOS manual which remains correct)
  • Fits CONTRIBUTING.md.

@roberth
nixos/lib/eval-config.nix: Add extraModules parameter for opt-in purity
4272188
@roberth
pkgs.nixos, pkgs.nixosTest: Optionally avoid impurity
21b0ca4 
This avoids the NIXOS_EXTRA_MODULE_PATH impurity in NixOS when
Nixpkgs is invoked 'purely' via pkgs/top-level/default.nix, but
passed the impurity along if invoked impurely via /default.nix
or nixpkgs/top-level/impure.nix.

pkgs.nixosTests is impure by necessity until a possible future
optimization that will make it call pkgs.nixosTest, therefore
inheriting its behavior.
grahamc
grahamc reviewed Dec 10, 2018
View reviewed changes
nixos/lib/build-vms.nix
@@ -6,7 +6,8 @@
# Nixpkgs, for qemu, lib and more
, pkgs
, # NixOS configuration to add to the VMs
extraConfigurations ? []
extraConfigurations ? let e = builtins.getEnv "NIXOS_EXTRA_MODULE_PATH";
in if e == "" then [] else [(import e)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, a bit ugly to proliferate this code around. Are there nicer ways to accomplish this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just not put it most places, and then see what breaks :D

Ericson2314
Ericson2314 reviewed Dec 11, 2018
View reviewed changes
pkgs/top-level/impure.nix
@@ -72,15 +72,24 @@ in
else overlays homeOverlaysDir
else []

# An impurity in NixOS to pass on when invoking pkgs.nixos,
# pkgs.nixosTest, pkgs.nixosTests.
, nixosExtraModules ? let e = builtins.getEnv "NIXOS_EXTRA_MODULE_PATH";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe call this nixosTestExtraModules so it doesn't just sound like some layer violation.

@ottidmes
Copy link

I completely agree with the reason for these changes, namely that you should be able to make your builds completely pure and that the choice should not be between impure and less impure, rather it should be between pure and impure. I do have two points of feedback though:

  1. Ideally we should have pure by default and only add impurities explicitly, while these changes make it from always potentially impure to potentially impure by default, so an improvement, but still not what we should aim for. That does not mean that I am against merging these changes, because it is still an improvement in the meantime.

  2. From checking your changes, it seems like you got it covered, but just in case I will ask: is the original use case for adding this environment variable still covered? The ability to change every NixOS evaluation externally with the use of an environment variable. With the exception of the NixOS packages not run through impure.nix, but that is a good thing and does not break the use case.

@mmahut
Copy link
Member

mmahut commented Aug 12, 2019

Are there any updates on this pull request, please?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@roberth
Copy link
Member Author

roberth commented Jul 18, 2021

This PR was too conservative. Nowaday's, we're moving toward a Nix that doesn't read environment variables by default, so we'll probably remove this feature completely at some point in the future.

@roberth roberth closed this Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

@Ericson2314 Ericson2314 Ericson2314 left review comments

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 6.topic: non-nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@roberth @ottidmes @mmahut @grahamc @Ericson2314 @ryantm @GrahamcOfBorg