Skip to content

Commit

Permalink
Revert "nixos: remove rsync from base install and add explicit path i…
Browse files Browse the repository at this point in the history 
…n nixos-install"

This reverts commit 582313b.

Removing rsync is actually pointless because nixos-install depends on
it. So if it's part of the system closure, we may as well provide it
to users.

Probably with the next Nix release we can drop the use of rsync and
use "nix copy" instead.
  • Loading branch information
@edolstra
edolstra committed Sep 5, 2016
1 parent 8295089 commit 0aa7520
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 2 deletions.
1 change: 1 addition & 0 deletions nixos/modules/config/system-path.nix
View file
Expand Up @@ -34,6 +34,7 @@ let
config.programs.ssh.package
pkgs.perl
pkgs.procps
pkgs.rsync
pkgs.strace
pkgs.su
pkgs.time
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/installer/tools/nixos-install.sh
View file
Expand Up @@ -169,7 +169,7 @@ if ! NIX_DB_DIR=$mountPoint/nix/var/nix/db nix-store --check-validity @nix@ 2> /
for i in $(@perl@/bin/perl @pathsFromGraph@ @nixClosure@); do
echo " $i"
chattr -R -i $mountPoint/$i 2> /dev/null || true # clear immutable bit
@rsync@/bin/rsync -a $i $mountPoint/nix/store/
rsync -a $i $mountPoint/nix/store/
done

# Register the paths in the Nix closure as valid. This is necessary
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/installer/tools/tools.nix
View file
Expand Up @@ -21,7 +21,7 @@ let
name = "nixos-install";
src = ./nixos-install.sh;

inherit (pkgs) perl pathsFromGraph rsync;
inherit (pkgs) perl pathsFromGraph;
nix = config.nix.package.out;
cacert = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt";
root_uid = config.ids.uids.root;
Expand Down

10 comments on commit 0aa7520

@obadz
Copy link
Contributor

@obadz obadz commented on 0aa7520 Sep 6, 2016

Choose a reason for hiding this comment

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

I personally disagree that everything that's installed should necessarily be in the path.

More importantly, I think explicit paths in scripts are better than relying on stuff being installed (nixos-install can also run on non-NixOS systems) and this revert is now causing the ova test failure: http://hydra.nixos.org/build/39960547

I'll leave rsync in nixos/modules/config/system-path.nix since that is your wish but I'll make the paths explicit again to fix test failure if that's ok.

@obadz
Copy link
Contributor

@obadz obadz commented on 0aa7520 Sep 6, 2016

Choose a reason for hiding this comment

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

Btw one argument to remove rsync from path now is so that nobody is surprised when we drop it when nix copy comes around.

Also fyi 16.09 was forked without rsync.

See partial revert 3f1ceae

@copumpkin
Copy link
Member

Choose a reason for hiding this comment

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

I honestly wish we'd go in the other direction here, and remove things from the global system path replacing them with more precise links in scripts.

@edolstra
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, I didn't notice that this commit did more than just remove rsync from the system path.

@vcunat
Copy link
Member

@vcunat vcunat commented on 0aa7520 Sep 28, 2016

Choose a reason for hiding this comment

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

I put rsync back to to 16.09 as well 5e86b8a. If we remove it (and I would be fine with it), master should go first and we should put it into release notes.

@obadz
Copy link
Contributor

@obadz obadz commented on 0aa7520 Feb 26, 2017

Choose a reason for hiding this comment

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

Shall we remove rsync from PATH prior to 17.03 branch-off or leave as be?
(Like @copumpkin I feel like just because we depend on it somewhere doesn't mean it should be in system PATH)

cc @globin @edolstra @vcunat

@vcunat
Copy link
Member

@vcunat vcunat commented on 0aa7520 Feb 26, 2017

Choose a reason for hiding this comment

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

I've got no strong opinion on this point. Including it does not prevent anyone from using more precise paths. (There's less push to do so, but I'm not convinced that's bad.)

@copumpkin
Copy link
Member

@copumpkin copumpkin commented on 0aa7520 Feb 26, 2017 •
edited

Choose a reason for hiding this comment

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

@vcunat not saying it prevents it, just that it's a crutch that enables bad habits. If for example your module that is usually enabled (but not always) adds something to systemPackages, I, a newbie module writer, might write my module assuming that your package is in scope by default on NixOS. Then someone customizes their configuration a bit, turns off your thing, and it breaks my service. Yes, I'm technically in the wrong for not being more careful about my dependencies, but it's really just a problem with global state in general; it obscures and makes it harder to reason about dependencies.

Personally, I'd rather abolish environment.systemPackages altogether, because I don't think it follows that just because the system has postgresql installed, every user on the system should want its tooling in their $PATH by default. And given how people write modules, everyone looks at existing modules as inspiration for new ones, and they see that most existing modules add packages to systemPackages so assume that's what we need to do, so we get more and more of this behavior over time.

Anyway, it really just feels very un-functional. I know we have the power to set up global state like this, but it seems silly for us to rely on it all over the place, especially when it's almost never necessary.

@copumpkin
Copy link
Member

Choose a reason for hiding this comment

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

As an example, firewall.enable defaults to true, but we can disable it. It adds iptables to our systemPackages. I just ran a quick search and found this module that assumes iptables is in scope, thanks to firewall setting systemPackages. If someone were to run miniupnpd with a firewall disabled, it would fail. This is obviously just an example; we can fix individual packages that break this way, but I'm more interested in systematic fixes for issues like this, not one-offs.

@vcunat
Copy link
Member

@vcunat vcunat commented on 0aa7520 Feb 26, 2017

Choose a reason for hiding this comment

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

Hmm, I think services shouldn't be allowed to just see general systemPackages (during runtime); perhaps some other default set.

Please sign in to comment.