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

darwin installer: remove the file before installing new one #3532

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

domenkozar
Copy link
Member

Otherwise results into:

cp: /Library/LaunchDaemons/org.nixos.nix-daemon.plist and /nix/var/nix/profiles/default/Library/LaunchDaemons/org.nixos.nix-daemon.plist are identical (not copied).

@domenkozar
Copy link
Member Author

Not sure this fixes it: /Library/LaunchDaemons/org.nixos.nix-daemon.plist: service already loaded

@domenkozar
Copy link
Member Author

Although I'd argue that should be a separate check at installation

@LnL7
Copy link
Member

LnL7 commented Apr 24, 2020

If we want to support this it's probably best to also ensure that whatever has been loaded by launchd is also removed first.

sudo launchctl remove org.nixos.nix-daemon

@domenkozar
Copy link
Member Author

I think the script should be idempotent, @grahamc came quite far in #1551

Copy link

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

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

seems 0644 is needed

@@ -39,7 +39,7 @@ EOF

poly_configure_nix_daemon_service() {
_sudo "to set up the nix-daemon as a LaunchDaemon" \
cp -f "/nix/var/nix/profiles/default$PLIST_DEST" "$PLIST_DEST"
install -m 0664 "/nix/var/nix/profiles/default$PLIST_DEST" "$PLIST_DEST"

Choose a reason for hiding this comment

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

Compare:

brick:~ blast$ launchctl stop org.nixos.nix-daemon && launchctl unload /Library/LaunchDaemons/org.nixos.nix-daemon.plist
brick:~ blast$ sudo install -m 0664 /nix/var/nix/profiles/default/Library/LaunchDaemons/org.nixos.nix-daemon.plist /Library/LaunchDaemons/org.nixos.nix-daemon.plist
brick:~ blast$ launchctl load /Library/LaunchDaemons/org.nixos.nix-daemon.plist && launchctl start org.nixos.nix-daemon
/Library/LaunchDaemons/org.nixos.nix-daemon.plist: Path had bad ownership/permissions

with

brick:~ blast$ launchctl stop org.nixos.nix-daemon && launchctl unload /Library/LaunchDaemons/org.nixos.nix-daemon.plist
brick:~ blast$ sudo install -m 0644 /nix/var/nix/profiles/default/Library/LaunchDaemons/org.nixos.nix-daemon.plist /Library/LaunchDaemons/org.nixos.nix-daemon.plist
brick:~ blast$ launchctl load /Library/LaunchDaemons/org.nixos.nix-daemon.plist && launchctl start org.nixos.nix-daemon

@stale
Copy link

stale bot commented Jun 2, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 2, 2021
@fricklerhandwerk
Copy link
Contributor

Still relevant.

@stale stale bot removed the stale label Feb 21, 2022
@fricklerhandwerk fricklerhandwerk added macos Nix on macOS, aka OS X, aka darwin installer labels Sep 9, 2022
@Ericson2314 Ericson2314 force-pushed the darwin-install-idempotent branch from b038a04 to db2dc7c Compare May 19, 2023 12:40
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner May 19, 2023 12:40
@Ericson2314
Copy link
Member

I rebased this and changed the mode as @blast-hardcheese accepted. I also made it symbolic because that is much more readable/maintainable.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I don't really think there is a good way to test this between CI and just....deploying to the wild --- at least I don't know of a good manual test plan.

I did some manual copying to my home directory with cp and install. I could not reproduce the original issue --- I thought cp -f was supposed to be idempotent?! --- but I could confirm that the new thing is at least idempotent too.

The bottom line is that install is...meant to be used in installers. Being explicit about the mode and not trusting unpacking is also good. Trying to be idempotent is always laudable. The theory is sound. I say we finally merge this tiny one line change, and see what happens. Users will let us know if something unpredictable goes wrong :).

Otherwise results into:

cp: /Library/LaunchDaemons/org.nixos.nix-daemon.plist and /nix/var/nix/profiles/default/Library/LaunchDaemons/org.nixos.nix-daemon.plist are identical (not copied).
@Ericson2314 Ericson2314 force-pushed the darwin-install-idempotent branch from db2dc7c to c73daea Compare June 14, 2023 16:28
@Ericson2314 Ericson2314 enabled auto-merge June 14, 2023 16:28
@Ericson2314 Ericson2314 merged commit 63dc8fb into NixOS:master Jun 14, 2023
@abathur
Copy link
Member

abathur commented Feb 12, 2024

I'm unsure why it would take so long to manifest as user reports, but it doesn't look like the mode part of this change works, and I'm starting to suspect it's causing some people to turn up with Bootstrap failed: 5: Input/output error errors during service init on reinstalls.

$ touch inst1
$ /usr/bin/install -m -rw-r--r-- inst1 inst2
$ ls -la inst*
-rw-r--r-- 1 abathur _lpoperator      0 Feb 12 12:19 inst1
---------- 1 abathur _lpoperator      0 Feb 12 12:21 inst2

FWIW I see the same behavior with the nix-installed coreutils implementation of install.

User reports I've spotted so far:

Won't be able to start on anything myself until this evening, but I imagine this will cause enough ongoing grief until all of the wild installs with these perms age out that it's worth getting a point release together, and maybe an issue with a lucid explanation and a clear workaround to pin.

If someone happens to pull together a PR with installer tests configured, I have a spare x86_64-darwin system that I can use to confirm it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installer macos Nix on macOS, aka OS X, aka darwin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants