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

Proposal: Changes to eval-machines #96

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

johanot
Copy link
Contributor

@johanot johanot commented Dec 7, 2019

Motivation

#67 and #70 did nice groundwork for per-machine nixpkgs args.

However the various changes have lead to a situation where nixpkgs and sometimes nixpkgs overlays get evaluated more than once, due to the modifications and merging of nixpkgs args morph eval-machines currently does. These cases are difficult to debug and non-trivial to fix, due to the custom way that morph merges and modifies the nixpkgs.* options while evaluating.

Also, it is sometimes just non-intuitive how nixpkgs args and nixpkgs options work with morph and users become unsure whether the latter work as described in the NixOS manual, when using morph.

In short, we want to not evaluate nixpkgs or overlays more than once - unless we have to. And we want to preserve the expected behavior of all the available nixpkgs.* options.

Proposed new behavior

  1. Let network.pkgs be optional.

  2. If network.pkgs is set, then nixpkgs.pkgs = mkDefault network.pkgs.

  3. Introduce network.lib, network.runCommand and network.evalConfig allowing for override of morph eval-machines internals.

  4. Get lib, runCommand and evalConfig from the network attrset, if unset then from network.pkgs, if unset then fallback to <nixpkgs>.

The consequence of 1 and 2, is that network.pkgs works exactly as nixpkgs.pkgs, still honoring nixpkgs.overlays, but disabling all other nixpkgs options.

3 and 4 allows for injection of all internal morph build dependencies, moreover preventing evaluation of multiple nixpkgs sets, even when network.pkgs is unset.

Feedback

@Shados I'd much appreciate your feedback on this. Especially, how does this solution affect cross-compilation with morph from your point of view? And what are your thoughts in general?

1) Let `network.pkgs` be optional.

2) If `network.pkgs` is set, then `nixpkgs.pkgs = mkDefault network.pkgs`.

3) Introduce `network.lib`, `network.runCommand` and `network.evalConfig` allowing for override of morph eval-machines internals.

4) Get lib, runCommand and evalConfig from the network attrset, if unset then from network.pkgs, if unset then fallback to `<nixpkgs>`.
@srhb srhb self-requested a review December 7, 2019 14:17
@johanot
Copy link
Contributor Author

johanot commented Dec 13, 2019

@Shados will you get a chance to look at this during the coming week? (no pressure) :-) Don't know who else could have an opinion here. @delroth ?

@Shados
Copy link
Contributor

Shados commented Dec 14, 2019

Huh. Weirdly I didn't get a notification for the initial ping. I'm probably not going to have time to really have a play with this, but it basically looks OK; people can still manually merge things if they want. I'm not using morph with any cross-compiled systems, so don't really have anything for you on that front.

@johanot
Copy link
Contributor Author

johanot commented Dec 22, 2019

@flokli This is not a requirement for releasing at all (and I haven't forgotten you) :-). But.. Perhaps you have an opinion about this change as well? Feedback is much appreciated.

@flokli
Copy link

flokli commented Dec 23, 2019

@johanot LGTM in general, but I haven't yet wrapped my head around whether this makes cross more difficult or not. I'll pass that question to @lheckemann ;-)

@lheckemann
Copy link

I'm not familiar with morph at all, haven't tried it before. But this looks to me as if it would make cross easier by introducing more flexibility.

@johanot johanot merged commit 0630d6b into DBCDK:master Jan 7, 2020
@johanot johanot deleted the optional-network-pkgs branch January 7, 2020 14:21
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

5 participants