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

Create a profile suitable for multi-user installs (for master) #1455

Closed
wants to merge 1 commit into from

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Jul 9, 2017

(cherry picked from commit 3e0a503)

#1454

@grahamc grahamc changed the title Create a profile suitable for multi-user installs Create a profile suitable for multi-user installs (for master) Jul 9, 2017

# Set up the per-user profile.
mkdir -m 0755 -p $NIX_USER_PROFILE_DIR
if test "$(stat -f '%u' $NIX_USER_PROFILE_DIR)" != "$(id -u)"; then
Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember from the last attempt to have a multi-user-capable nix-profile that stat had issues on some platforms. But maybe we don't need to care if we're only going to use this on Linux and macOS.

Copy link
Member

Choose a reason for hiding this comment

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

That was here: aac1422

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm inclined to go ahead with it and as-is, and work on cross-platform support if someone becomes interested in it. Since indeed this will only be used (by default) on macOS, I think it is safe.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, if you have coreutils installed (even on Mac) it will give you:

stat: cannot read file system information for '%u': No such file or directory
WARNING: bad ownership on /nix/var/nix/profiles/per-user/...
stat: cannot read file system information for '%u': No such file or directory
WARNING: bad ownership on /nix/var/nix/gcroots/per-user/...

Maybe we should hardcode "/usr/bin/stat" to it, so that we get the MacOS/BSD version of stat?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, this works on all platforms:

"$(ls -ld "$NIX_USER_PROFILE_DIR" | awk '{print $3}')" != "$USER"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of this option, as it strictly is the UID we care about, not the username.

What do you think about thisoption:

if [ -n "$(find "$NIX_USER_PROFILE_DIR" -mindepth 0 -maxdepth 0 -type d -not -uid $(id -u))" ]

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Another option might be:

test -O "$NIX_USER_PROFILE_DIR"

Copy link

Choose a reason for hiding this comment

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

I tested ! test -O "$NIX_USER_PROFILE_DIR" and it does fix the problems discussed in issue #1496, including nix-shell -p bash --run bash and nested nix-shell invocations. It's also simpler and shorter.

However, it looks like it's not in the POSIX standard.


# Set up the per-user profile.
mkdir -m 0755 -p $NIX_USER_PROFILE_DIR
if test "$(stat -f '%u' $NIX_USER_PROFILE_DIR)" != "$(id -u)"; then
Copy link
Member

Choose a reason for hiding this comment

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

Yep, if you have coreutils installed (even on Mac) it will give you:

stat: cannot read file system information for '%u': No such file or directory
WARNING: bad ownership on /nix/var/nix/profiles/per-user/...
stat: cannot read file system information for '%u': No such file or directory
WARNING: bad ownership on /nix/var/nix/gcroots/per-user/...

Maybe we should hardcode "/usr/bin/stat" to it, so that we get the MacOS/BSD version of stat?


# Set up the per-user profile.
mkdir -m 0755 -p $NIX_USER_PROFILE_DIR
if test "$(stat -f '%u' $NIX_USER_PROFILE_DIR)" != "$(id -u)"; then
Copy link
Member

Choose a reason for hiding this comment

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

BTW, this works on all platforms:

"$(ls -ld "$NIX_USER_PROFILE_DIR" | awk '{print $3}')" != "$USER"

fi
fi

export NIX_SSL_CERT_FILE="@localstatedir@/nix/profiles/default/etc/ssl/certs/ca-bundle.crt"
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed with recent SSL changes to Nix?

echo "WARNING: bad ownership on $NIX_USER_PROFILE_DIR" >&2
fi

if test -w $HOME; then
Copy link
Member

Choose a reason for hiding this comment

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

Why use test here when you could just use "[" and "]" ?

rm -f $HOME/.nix-defexpr
mkdir -p $HOME/.nix-defexpr
if [ "$USER" != root ]; then
ln -s @localstatedir@/nix/profiles/per-user/root/channels $HOME/.nix-defexpr/channels_root
Copy link
Member

Choose a reason for hiding this comment

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

I believe @localstatedir@/nix/profiles/per-user/root/channels won't exist until you run nix-channel --update, maybe that should be added?

@@ -0,0 +1,54 @@
# Only execute this file once per shell.
if [ -n "$__ETC_PROFILE_NIX_SOURCED" ]; then return; fi
__ETC_PROFILE_NIX_SOURCED=1
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary. None of the below actions have too many side effects. This could make things confusing to someone who has for instance modified their NIX_PATH value and wants to "reset" it. They have to unset __ETC_PROFILE_NIX_SOURCED for that to work.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I guess this is because nix-daemon.sh is put in multiple places?

Copy link

Choose a reason for hiding this comment

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

There are lots of side effects below, although most of them do look to me like they are written to be idempotent.

Setting the PATH is not idempotent, however, as it prepends to ${PATH}, so running this multiple times would result in a PATH with multiple copies of the directories added by this script.

$(d)/nix-reduce-build

noinst-scripts += $(nix_noinst_scripts)

profiledir = $(sysconfdir)/profile.d

$(eval $(call install-file-as, $(d)/nix-profile.sh, $(profiledir)/nix.sh, 0644))
$(eval $(call install-file-as, $(d)/nix-profile-daemon.sh, $(profiledir)/nix-daemon.sh, 0644))
Copy link
Member

Choose a reason for hiding this comment

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

I've got the following code in my environment script:

if [ -d @out@/etc/profile.d ]; then
  for i in @out@/etc/profile.d/*.sh; do
    if [ -r $i ]; then
      . $i
    fi
  done
fi

Because of the ordering nix.sh will be called after nix-daemon.sh. I'm not sure of a fix but it seems worth mentioning and a legitimate use case.

@grahamc
Copy link
Member Author

grahamc commented Aug 4, 2017 via email

fi

export NIX_SSL_CERT_FILE="@localstatedir@/nix/profiles/default/etc/ssl/certs/ca-bundle.crt"
export NIX_PATH="@localstatedir@/nix/profiles/per-user/root/channels"
Copy link
Member

Choose a reason for hiding this comment

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

Can we also get a backup if NIX_PATH has already been set? (like in PATH below)

@FRidh
Copy link
Member

FRidh commented Dec 21, 2017

@grahamc close this because 6a037a7 went in?

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

7 participants