-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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-install: re-enable --chroot option #28777
Conversation
# Set up some bind mounts we'll want regardless of chroot or not | ||
mount --rbind /dev $mountPoint/dev | ||
mount --rbind /proc $mountPoint/proc | ||
mount --rbind /sys $mountPoint/sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the mountPoint
chosen by users? I wonder if it make sense to quote it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably does. I'll fix that, thanks
rm -rf $mountPoint/var/run | ||
ln -s /run $mountPoint/var/run | ||
for f in /etc/resolv.conf /etc/hosts; do rm -f $mountPoint/$f; [ -f "$f" ] && cp -Lf $f $mountPoint/etc/; done | ||
for f in /etc/passwd /etc/group; do touch $mountPoint/$f; [ -f "$f" ] && mount --rbind -o ro $f $mountPoint/$f; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line means that you cannot chroot into the new installation, and use passwd
to setup passwords, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is basically just copied and pasted from the old installer's --chroot
support.
8e94a9b
to
1241b03
Compare
there was also a weird quick in how |
@cleverca22 I didn't really change the arg parsing at all. Does it look like I messed that up here? |
@copumpkin i dont think you broke it, but that is one quirk you can test for, see if it defaults to a shell, and also allows still running a given command directly |
Yeah, my point is just that I can't actually test it easily right now, because I don't have a Linux box and can't run VM tests, so I'd appreciate help from anyone who has an actual box to run it on 😄 |
# Set up some bind mounts we'll want regardless of chroot or not | ||
mount --rbind /dev "$mountPoint/dev" | ||
mount --rbind /proc "$mountPoint/proc" | ||
mount --rbind /sys "$mountPoint/sys" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of doing a fresh install those mount points don't exist yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only support doing this from some sort of live cd installation, as far as I know, which will have those mount points. I'm just replicating the old installer's behavior here, not trying to add new functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT those don't exist until nixos-prepare-root
is ran later on in the installer, and installation starts off with more or less empty $mountPoint
:
$ nix-build "nixos/release-combined.nix" -A nixos.tests.installer.simple.x86_64-linux
# bunch of noise omitted
machine: must succeed: nixos-install < /dev/null >&2
machine# > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > mount: /mnt/dev: mount point does not exist.
machine: exit status 32
machine: output:
error: command `nixos-install < /dev/null >&2' did not succeed (exit code 32)
command `nixos-install < /dev/null >&2' did not succeed (exit code 32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbogdan that's why I consider this option to be pretty awkward, but my understanding is that you're only expected to run --chroot
on an "installed" (i.e., you've already run install on it once at some point in the past) $mountPoint
. It really doesn't feel to me like this functionality belongs in the installer itself, but rather in an adjoining tool that's called something like "poke-around-post-install.sh", but I didn't want to break the existing interface so that's why it's here.
So at that point, $mountPoint
has the folders. I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for --chroot
the assumption is that you have an existing install somewhere and all of those are present. I'm only trying to point out that as is this change would break the use case of using the installer as, well, installer 😆.
And I agree that it seems like somewhat strange functionality to be included, and recall initially being a bit confused as to what purpose it was supposed to serve in context of an installer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point, sorry for being dense 😄 I'll push up a fix later
@@ -102,16 +102,37 @@ fi | |||
extraBuildFlags+=(--option "build-users-group" "$buildUsersGroup") | |||
|
|||
# Inherit binary caches from the host | |||
# TODO: will this still work with Nix 1.12 now that it has no perl? Probably not... | |||
# TODO: will this still work with Nix 1.12 now that it has no perl? Probably not... | |||
binary_caches="$(@perl@/bin/perl -I @nix@/lib/perl5/site_perl/*/* -e 'use Nix::Config; Nix::Config::readConfig; print $Nix::Config::config{"binary-caches"};')" | |||
extraBuildFlags+=(--option "binary-caches" "$binary_caches") | |||
|
|||
nixpkgs="$(readlink -f "$(nix-instantiate --find-file nixpkgs)")" | |||
export NIX_PATH="nixpkgs=$nixpkgs:nixos-config=$mountPoint/$NIXOS_CONFIG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it matters much but this would propagate "configured-for-installation" NIX_PATH into the chroot environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what it used to do. In case you're missing context, I basically rewrote how nixos-install
worked a couple of months ago, and forgot to support the --chroot
option at the time. This is me just bringing it back to life, not trying to do anything fancy.
I forgot to implement it the first time around. Whoops!
1241b03
to
186c120
Compare
|
||
# These get created in nixos-prepare-root as well, but we want to make sure they're here in case we're | ||
# running with --chroot. TODO: --chroot should just be split into a separate tool. | ||
mkdir -m 0755 -p "$mountPoint/dev" "$mountPoint/proc" "$mountPoint/sys" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbogdan I think this resolves the issue you pointed out, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copumpkin LGTM, checked with nixos.tests.installer.simple.x86_64-linux
and --chroot
on a fresh install and everything is working AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
@globin should I backport this to 17.09 now? Or is that automated somehow? Sorry, I'm pretty clueless about this whole release thing 😄 |
Thank you, and sorry this took so long! I can cherry-pick going forward 😄 |
I forgot to implement it the first time around. Whoops!
Motivation for this change
I broke it, I fix it. Should fix #28251. @cleverca22 do you have an easy harness to check this? It would be nice to add to the installer tests but I can't really engage with any of the VM tests easily since I don't have a machine that can run them right now.