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

WIP: installer: allow overriding nix user UID on darwin too #6466

Conversation

bjornfor
Copy link
Contributor

Similar to f4d57aa ("installer: allow overriding nix user GID and
UIDs"), but for darwin.

(There's no corresponding *_GID in this script though, so skipping
that.)

Ref #6153.

Similar to f4d57aa ("installer: allow overriding nix user GID and
UIDs"), but for darwin.

(There's no corresponding *_GID in this script though, so skipping
that.)

Ref NixOS#6153.
@abathur
Copy link
Member

abathur commented Apr 30, 2022

I don't have enough time to look, but I think it may not be this simple. Iirc the variable is already set in the core multiuser script, so I think this would start inheriting that default?

@bjornfor
Copy link
Contributor Author

bjornfor commented May 3, 2022

I don't have enough time to look, but I think it may not be this simple. Iirc the variable is already set in the core multiuser script, so I think this would start inheriting that default?

You're right, landing this PR now would mean effectively changing the darwin default UID from 301 to 30001.

@abathur
Copy link
Member

abathur commented May 3, 2022

#4531 and #4532 have some backstory on how it ended up at 301 (and why it can't be 30001).

@bjornfor bjornfor changed the title installer: allow overriding nix user UID on darwin too WIP: installer: allow overriding nix user UID on darwin too May 4, 2022
@bjornfor bjornfor marked this pull request as draft May 8, 2022 14:24
@stale stale bot added the stale label May 21, 2023
@@ -6,7 +6,7 @@ set -o pipefail
readonly NIX_DAEMON_DEST=/Library/LaunchDaemons/org.nixos.nix-daemon.plist
# create by default; set 0 to DIY, use a symlink, etc.
readonly NIX_VOLUME_CREATE=${NIX_VOLUME_CREATE:-1} # now default
NIX_FIRST_BUILD_UID="301"
NIX_FIRST_BUILD_UID="${NIX_FIRST_BUILD_UID:-301}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use a different var name to avoid it being overridden by the parent script.

Suggested change
NIX_FIRST_BUILD_UID="${NIX_FIRST_BUILD_UID:-301}"
NIX_FIRST_BUILD_UID="${NIX_FIRST_BUILD_UID_DARWIN:-301}"

Copy link
Member

Choose a reason for hiding this comment

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

While it is the simpler change, I think the potential-confusion would be lower if we moved the value-setting out into the ~speciated scripts (i.e., require the platform install scripts to be opinionated on this UID instead of having a default-that-isn't-a-default?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

Something like:

  • replace the default declaration up in the main multiuser script with a comment that says that the speciated ~variant (-darwin-, -linux-) scripts are responsible for setting it
  • move the default declaration out into the -linux- script
  • do roughly what this current PR does for the -darwin- script

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha. yeah, sounds good.

@stale stale bot removed the stale label Dec 12, 2023
@bacchanalia
Copy link
Contributor

I'd really like to get some version of this in. It's been causing me headaches.

@bjornfor
Copy link
Contributor Author

@bacchanalia: if you can, please make a PR.

@bacchanalia
Copy link
Contributor

@bjornfor sure. I just wasn't sure if you were interested in reviving this one or not.

bacchanalia added a commit to awakesecurity/nix that referenced this pull request Dec 18, 2023
because there are often already users in the 300 range and it's painful
to work around.

revives NixOS#6466
@bjornfor
Copy link
Contributor Author

Superseded by #9639.

@bjornfor bjornfor closed this Dec 19, 2023
mergify bot pushed a commit that referenced this pull request Sep 11, 2024
because there are often already users in the 300 range and it's painful
to work around.

revives #6466

(cherry picked from commit fa4bbe5)
mergify bot pushed a commit that referenced this pull request Sep 11, 2024
because there are often already users in the 300 range and it's painful
to work around.

revives #6466

(cherry picked from commit fa4bbe5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants