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

Fix diskutil deleteVolume command in Uninstall guide #11371

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

secobarbital
Copy link
Contributor

Motivation

The uninstall guide had an incorrect command to delete the Nix Store volume on MacOS. It is correct in one place but wrong in the other.

The guide also suggests to copy the backup copies of the system shell rc files if they had not been modified since the Nix install, but there is no way to know if that is the case, so I think it is better to just have the user remove the daemon.sh lines.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@secobarbital secobarbital marked this pull request as ready for review August 26, 2024 03:36
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Of course there's a way to check if something changed: diff the backup file with the current file and check if only the Nix block stands out. :)

The new state of the document would not mention the backup files any more and probably lead to users keeping them around.

If you make a separate PR for the apfs command fix, I can merge that immediately. Thanks for catching that! The other part needs a bit more thought.

doc/manual/src/installation/uninstall.md Outdated Show resolved Hide resolved
@secobarbital
Copy link
Contributor Author

Of course there's a way to check if something changed: diff the backup file with the current file and check if only the Nix block stands out. :)

The new state of the document would not mention the backup files any more and probably lead to users keeping them around.

I took out the backup file parts so we can merge the apfs change with this PR.

My problem with the rc files instruction is that Apple changes them on Mac OS upgrades so people may not realize that they changed and blindly overwrite them with the backup files.

I wonder if the instruction was targeted at people uninstalling after trying nix out for a few hours vs I want to change it to avoid confusing my colleagues who I had convinced a while back to install nix for our dev env and now uninstalling to use the Determinate Systems Installer but the latter fails on the Nox Store volume.

I’m sure we can come up with wording to satisfy both.

@fricklerhandwerk
Copy link
Contributor

The instructions were written before the whole store volume thing came up. Much of the material all around is older than you think. Would be happy to review or discuss proposals for improving this stuff. Maybe it could be as easy as running diff or a some other rather simple commands to check for changes.

@fricklerhandwerk fricklerhandwerk enabled auto-merge (squash) August 27, 2024 23:44
@fricklerhandwerk fricklerhandwerk merged commit 8e63dc4 into NixOS:master Aug 28, 2024
10 of 11 checks passed
@secobarbital secobarbital deleted the patch-1 branch August 28, 2024 16:14
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.

2 participants