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

Fix honouring of nixpkgs.config #135

Closed
wants to merge 1 commit into from

Conversation

blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Oct 2, 2020

This should honour modules that declare:

{
  nixpkgs.config.allowUnfree = true;
}

lazy evaluation is like magic

@blaggacao
Copy link
Contributor Author

/cc @adamtulinius @johanot @srhb a quick ping to get discussion going, if you have time. 🎐 (see also the alternative PRs)

@johanot
Copy link
Contributor

johanot commented Oct 3, 2020

@blaggacao First of all, thank you for your interest in morph and for the PR!
Your suggestion doesn't work for me, since network.pkgs is supposed to be a pre-instantiated version of nixpkgs, e.g.

network.pkgs = (import <nixpkgs> {});

on your branch this yields: error: attempt to call something which is not a function but a set, at /run/user/1000/morph-243146745/morph/eval-machines.nix:39:57

That said; I understand what you are trying to do. Turns out there are a huge amount of permutations when considering both the nixpkgs.* options and "nixpkgs args", and it is difficult to improve the current behavior in one area, without breaking something else. If we choose to change how the nixpkgs.* options play together with nixpkgs args, we'll have to step carefully and test thoroughly with different deployment layouts. :)

Perhaps this is not actually documented in morph README, but in general I would advice against using the nixpkgs.* options, except nixpkgs.pkgs. Mainly because all other nixpkgs.* options are ignored if nixpkgs.pkgs is set (see the upstream docs for nixpkgs.config for example), and as you know, morph mkDefault's nixpkgs.pkgs to network.pkgs if the latter is set.

I've created two examples that works with morph master, which (I hope) achieves what you want: https://gist.github.com/johanot/ec2e314e01d54951753b057f75f65f5b and https://gist.github.com/johanot/164ace22de35ccad8259edfcae26ec07 .
Both use nixpkgs arg config instead of the nixpkgs.config module to set allowUnfree at per-host granularity.

Notes on reproducibility with morph:

Not setting network.pkgs causes eval-config.nix, lib and runCommand to "leak in" from the environment through NIX_PATH. For the same reason, it's recommended to always set the config arg to a nixpkgs invocation, since the it will fallback to what is set in the build environment (see https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/impure.nix#L27-L37).

@blaggacao
Copy link
Contributor Author

@johanot Thank you for the context.

I ended up setting only evalConfig as morph dependency to the pinned nixpkgs version. Akin to what you suggested in this gist.

I bet ./lib does not leak (neither — hopefully — would pkgs/build-support or the new pkgs/pkgs-lib). They should (as in imperative) be properly encapsulated. If they are not, this might be considered a bug in NixOS/nixpkgs. — I'm happy for this claim to get falsified.

What absolutely strikes me, though, is that eval-config seems to be leaky. And I kind of think this is a bug in nixos/lib/eval-config: I've opened a discourse discussion about it: https://discourse.nixos.org/t/eval-config-nix-waging-war-against-nixpkgs-pkgs/9277 which unfortunately is a bit TL;DR for a non involved knowledgeable third party.

Conclusion: It suffices to — I would say — co-pin eval-config together with the modularized pinning of nixpkgs.

IMO, having the two conflicting entry points definitely indicates a bug somewhere buried deep in nixpkgs. Or bad code design, which almost is a bug equivalent depending on one's standards.

Btw, thanks for the educative discussion!

@DavHau
Copy link

DavHau commented Oct 4, 2020

Perhaps this is not actually documented in morph README, but in general I would advice against using the nixpkgs.* options, except nixpkgs.pkgs. Mainly because all other nixpkgs.* options are ignored if nixpkgs.pkgs is set (see the upstream docs for nixpkgs.config for example), and as you know, morph mkDefault's nixpkgs.pkgs to network.pkgs if the latter is set.

@johanot
For exactly the reason you described, I would not use nixpkgs.pkgs in morph. Out of different ways to to pass pkgs, nixpgs.pkgs is the only one which breaks other stuff like overlays or config

I think the fix for that should look somewhat like this PR: #136
If I'm not mistaken, this will keep the current functionality, but without breaking nixpkgs.config, nixpkgs.overlays etc.

EDIT: Just noticed that it doesn't break nixpkgs.overlays. Still the nixpkgs.pkgs option looks like something which is meant as last-resort option, to break out of everything previously defined. I don't think that this should be the first choice for a deployment management tool like morph.

@blaggacao
Copy link
Contributor Author

blaggacao commented Oct 4, 2020

If I'm not mistaken, this will keep the current functionality, ...

This is not entirely true.

Modularily setting nixpkgs.pkgs via lib.mkDefault opens up the field to override this value in specific machine configurations. Except, it doesn't work properly for eval-config's leaking — which is rather a bug than a feature in my opinion.

Furthermore, there is this comment from 7 years ago which seems to promote my own conclusions that eval-config should not be passed a pkgs argument while at the same time it also should not be leaking, since that means in effect a duplicate (conflicting) input of the same type to a single evaluation. That's a recipe for conflict by any means.

I opened a discourse question on the abstract reasoning behind this very PR: https://discourse.nixos.org/t/configured-nixpkgs-vs-modularily-configured-nixpkgs/9300 — seems like a question for someone more familiar with the reasons, the git blame history is not particularly telling (or even seems outdated).

@srhb
Copy link
Contributor

srhb commented Apr 29, 2021

Closing this for now. It's not really clear to me whether it's safe to do anything like this, and certainly not without extensive testing. This is the sort of thing that feels like the abstraction regarding nixpkgs.config is a bit weird, hopefully it'll improve in the future and pave a more clear path for us.

@srhb srhb closed this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants