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

Jump to bottom

nixos-install: add support for flakes #95194

Merged
merged 2 commits into from Sep 5, 2020
Merged

nixos-install: add support for flakes #95194

merged 2 commits into from Sep 5, 2020

Conversation

ju1m
Copy link
Contributor

@ju1m ju1m commented Aug 11, 2020 •
edited

Motivation for this change

Use nixos-install with a flake.nix.

Things done

I've just copied code from nixos-rebuild, not sure whether all this is correct.

  • Add support for --flake and other options like in nixos-rebuild.
  • Quote variables to please ShellCheck (I hope I've not done any mistake in doing so).
  • Pass pkgs.nixFlakes's nix prepended on PATH, to be used only when --flake is involved. This avoids adding all nixUnstable's closure to nixos-install.
  • Pass --experimental-features 'nix-command flakes' to nix when using --flake in nixos-install and nixos-rebuild.
  • Document --flake.
  • Fix documentation by using the URI fragment syntax.
  • 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.

@Ma27 Ma27 self-requested a review August 11, 2020 17:33
@worldofpeace
Copy link
Contributor

I just want to note to nixos devs to please not merge this PR without the documentation. I hope that doesn't sound rigid, but I think it's more likely, if this isn't documented in the same PR, to take a significantly longer time to be documented.

@vlaci
Copy link
Contributor

vlaci commented Aug 30, 2020

IMHO jq should be added to PATH as it had been done for nixos-rebuild

@worldofpeace
Copy link
Contributor

@vlaci tools.nix puts jq into PATH and nixos-install is an output of this.

@vlaci
Copy link
Contributor

vlaci commented Aug 30, 2020

@worldofpeace are you sure? my installer iso built from unstable does not have it by default, and I only see it scoped to nixos-rebuild in tools.nix

@worldofpeace
Copy link
Contributor

@worldofpeace are you sure? my installer iso built from unstable does not have it by default, and I only see it scoped to nixos-rebuild in tools.nix

Ooh, I thought you meant for the wrapped PATH for nixos-install, but actually you mean as a utility available in nixos by default. I think that's excessive, and we certainly wouldn't want to undo the wrapping for the tools as that would make them non-hermetic. So I would consider adding jq out of scope for this particular change as jq isn't really a necessary utility outside of these programs that need them.

@vlaci
Copy link
Contributor

vlaci commented Aug 30, 2020 •
edited

@worldofpeace are you sure? my installer iso built from unstable does not have it by default, and I only see it scoped to nixos-rebuild in tools.nix

Ooh, I thought you meant for the wrapped PATH for nixos-install, but actually you mean as a utility available in nixos by default. I think that's excessive, and we certainly wouldn't want to undo the wrapping for the tools as that would make them non-hermetic. So I would consider adding jq out of scope for this particular change as jq isn't really a necessary utility outside of these programs that need them.

No, I have meant nixos-install in the first place then I have misunderstood you :)
Anyway, this PR does not add jq in the path for nixos-install and it isn't already there. What am I missing?
It is set for nixos-rebuild in https://github.com/ju1m/nixpkgs/blob/nixos-install/nixos/modules/installer/tools/tools.nix#L34
but is not for nixos-install at https://github.com/ju1m/nixpkgs/blob/nixos-install/nixos/modules/installer/tools/tools.nix#L23

Sorry I have meant to put the links in the first comment but somehow missed it.
Also see the commit that introduced it for nixos-install I thought that this PR should mirror that change too.

@worldofpeace
Copy link
Contributor

Oh, lol @vlaci understood.

worldofpeace
worldofpeace previously requested changes Aug 30, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

@ju1m Can you add pkgs.jq to https://github.com/ju1m/nixpkgs/blob/nixos-install/nixos/modules/installer/tools/tools.nix#L23

Ma27
Ma27 reviewed Aug 30, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

First of all thanks a lot for your effort!

  • Tested with tests.installer.simple and on a local tmpfs mounted to /mnt and this seems to work fine overall 👍
  • I think that it should be somehow ensured that we're actually using a nixUnstable for nixos-install in the case of flake support.
  • I'd like to have some kind of default behavior in case of flakes (e.g. nixos-install --flake path:/mnt/etc/nixos#default as default behavior if flake.nix exists).
  • hardware-configuration.nix imports <nixpkgs/nixos/modules/installer/scan/not-detected.nix>. I'm not sure how, but we should somehow take care of this when using flakes.

nixos/modules/installer/tools/nixos-install.sh Show resolved Hide resolved
nixos/modules/installer/tools/nixos-install.sh Show resolved Hide resolved
@xaverdh
Copy link
Contributor

xaverdh commented Aug 31, 2020 •
edited

hardware-configuration.nix imports <nixpkgs/nixos/modules/installer/scan/not-detected.nix>. I'm not sure how, but we should somehow take care of this when using flakes.

By having a module which does whatever detected.nix / not-detected.nix do (which is not a lot currently) depending on a boolean value which nixos-generate-configshould write inhardware-configuration.nix ?

**edit**: or by dropping detected.nix / not-detected.nix all together and just flipping hardware.enableRedistributableFirmwareinnixos-generate-config ..

Ok never mind, it uses modulesPath anyway these days. You probably meant the case where people already have a generated hardware-configuration.nix.. I guess they can just regenerate it then?

@worldofpeace
Copy link
Contributor

For hardware-configuration not detected and anything that wants to use <nixpkgs> we'll have to conditionally change the syntax to like inputs.nixpkgs.nixosModules.notDetected. IIRC at a glance there's a lot of code that would have to be different.

@Ma27
Copy link
Member

Ma27 commented Aug 31, 2020

Ok never mind, it uses modulesPath anyway these days. You probably meant the case where people already have a generated hardware-configuration.nix.. I guess they can just regenerate it then?

You're absolutely right! I actually generated a fresh config in my temporary /mnt mount, but I used the default nixos-generate-config from my 20.03 system for that purpose, so never mind then :)

@worldofpeace
Copy link
Contributor

worldofpeace commented Aug 31, 2020 •
edited

@Ma27 Oh awesome c45295d#diff-14ed97ea0350115b36b0b8d884db5667 no action needed ✨

@ju1m
Copy link
Contributor Author

ju1m commented Sep 2, 2020

I've tried to incorporate your feedbacks as I could. Please let me know if something is missing or wrong.

@ju1m ju1m force-pushed the nixos-install branch 4 times, most recently from d694851 to 4962dd3 Compare September 4, 2020 04:17
cole-h
cole-h approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Diff LGTM (did all of my reviewing on IRC).

(Note that a slight difference from previous revisions of this PR is the addition of --experimental-features flags where necessary, so that we don't rely on a nix.conf with the proper settings enabled.)

@worldofpeace
Copy link
Contributor

I think there's tests to run here for nixos-install to check nothing broke

@cole-h
Copy link
Member

cole-h commented Sep 5, 2020

@ofborg test installer.simple

(Feel free to check additional installer tests, but I believe this is the simplest (heh) of the bunch that tests nixos-install.)

@worldofpeace worldofpeace dismissed their stale review September 5, 2020 19:25

stale, see above

@worldofpeace
Copy link
Contributor

So @cole-h cleared up on IRC that my reading skills aren't that great today 🤣

worldofpeace: btw we went over that yesterday -- in a trap, single quotes don't prevent expansion unless the variable is single-quoted inside those single-quotes (iirc)

@worldofpeace worldofpeace merged commit d0972c9 into NixOS:master Sep 5, 2020
@xaverdh
Copy link
Contributor

xaverdh commented Sep 11, 2020

I think jq is still missing from

nixpkgs/nixos/modules/installer/tools/tools.nix

Line 25 in 607f5a6

path = makeBinPath [ pkgs.nixUnstable nixos-enter ];
(just stumbled upon this, as jq is not in my default PATH)

@worldofpeace
Copy link
Contributor

I think jq is still missing from

nixpkgs/nixos/modules/installer/tools/tools.nix

Line 25 in 607f5a6

path = makeBinPath [ pkgs.nixUnstable nixos-enter ];

(just stumbled upon this, as jq is not in my default PATH)

You're right. I think it just disappeared from the diff at some point.

@worldofpeace
Copy link
Contributor

fixed in a39ad85

@worldofpeace
Copy link
Contributor

backported to 20.09 281c899

@ncfavier
Copy link
Member

ncfavier commented Oct 3, 2020

nixpkgs/nixos/modules/installer/tools/nixos-install.sh

Lines 155 to 157 in c8ccfdb

nix "${flakeFlags[@]}" build "$flake#$flakeAttr.config.system.build.toplevel" \
--extra-substituters "$sub" "${verbosity[@]}" \
"${extraBuildFlags[@]}" "${lockFlags[@]}" --out-link "$outLink"

This builds the flake in /nix instead of /mnt/nix, which makes nixos-install run out of memory for me on a live CD.

It doesn't seem like nix build has something like a --store flag, should I open a separate issue?

@cole-h
Copy link
Member

cole-h commented Oct 3, 2020 •
edited

Actually, all nix commands have a --store flag, because --store ... is shorthand for --option store .... I'll have a PR ready shortly.

EDIT: PR opened: #99493

@cole-h cole-h mentioned this pull request Oct 3, 2020
Merged
10 tasks
@chkno chkno mentioned this pull request Jun 10, 2021
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@ncfavier ncfavier ncfavier left review comments

@Ma27 Ma27 Ma27 left review comments

@cole-h cole-h cole-h approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ju1m @worldofpeace @vlaci @xaverdh @Ma27 @cole-h @ncfavier @edolstra