home-manager
home-manager copied to clipboard
files: avoid surprises when linking files
In the unlikely (but possible) event that a directory is created in place of a target file after checkLinkTargets but before linkGeneration, currently the link is created inside the directory, which is surprising (see https://github.com/nix-community/home-manager/pull/3017 for context).
To avoid this, use ln -T and bail if it fails.
Also bail if backing up the target fails.
Ideally, checkLinkTargets and linkGeneration should be more tightly coupled.
Checklist
-
[X] Change is backwards compatible.
-
[X] Code formatted with
./format. -
[X] Code tested through
nix-shell --pure tests -A run.all. -
[ ] Test cases updated/added. See example.
-
[X] Commit messages are formatted like
{component}: {description} {long description}See CONTRIBUTING for more information and recent commit messages for examples.
Do you know whether the GNU coreutil package is in the activation script's $PATH? If so, my previous comment can be ignored but a note should be left.
Yep. I'm not sure we want to add a comment in the code every time we rely on this though, it seems like a fine "ambient assumption".
Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.
If you are the original author of the PR
- GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
- If it is unfinished but you plan to finish it, please mark it as a draft.
- If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
- To get things rolling again, rebase the PR against the target branch and address valid comments.
If you are not the original author of the PR
- If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.
cc @rycee, I still think this change is good
I've removed the other change, added a comment and rebased on master.
Also note that we're already using ln -T: https://github.com/nix-community/home-manager/blob/c1a830c8fabb13f95f51ecf48552f0a794d8718a/modules/files.nix#L274