stdenv: add no-broken-symlinks hook

[ConnorBaker]

Adds a setup hook which checks for broken symlinks; a symlink is considered broken if it's dangling (target doesn't exist) or reflexive (it refers to itself).

In my experience, outputs with broken symlinks are generally errors, and so we should provide a hook which can catch and report them.

The hook can be disabled by setting dontCheckForBrokenSymlinks.

Future work

Fixup PRs:

• apple-sdk: set dontCheckForBrokenSymlinks #376043
• cc-wrapper: set dontCheckForBrokenSymlinks #376202
• bluez: set dontCheckForBrokenSymlinks #376210 (superseded by no-broken-symlinks: restrict checks to symlinks pointing inside the store #376261)
• systemd: set dontCheckForBrokenSymlinks #376218 (superseded by no-broken-symlinks: restrict checks to symlinks pointing inside the store #376261)
• no-broken-symlinks: restrict checks to symlinks pointing inside the store #376261

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[K900]

Conceptually LGTM, bash looks roughly OK and I don't trust myself to comment any further than that. Has anyone tried running the data on the number of broken symlinks in the current Hydra outputs?

[philiptaron]

As with @K900, this looks good to me in theory and I support the idea that a dangling or reflexive symlink is more often than not a bug.

I'd love a test. Could you write a few using testBuildFailure, showing:

1. Reflexive symlink fails
2. Dangling symlink fails
3. They don't fail if dontCheckForBrokenSymlinks is specified.

[nix-owners]

The PR's base branch is set to staging, but 4 commits from the master branch are included. Make sure you know the right base branch for your changes, then:

• If the changes should go to the master branch, change the base branch to master
• If the changes should go to the staging branch, rebase your PR onto the merge base with the staging branch:

  # git rebase --onto $(git merge-base upstream/staging HEAD) $(git merge-base upstream/master HEAD)
  git rebase --onto eac5f2caf93e16315194c6227cab4937af4b74e3 db0f0fb937b73c4585444925e17446726836a405
  git push --force-with-lease

[ConnorBaker]

Added docs and tests, force-pushed and rebased!

@K900 and @philiptaron would you mind taking another look?

[bjornfor]

Hi. Now there's lots of pkg: fix noBrokenSymlinks PRs all over nixpkgs, and they just nuke paths without any check whether the path being removed is problematic. I see that as a long-term issue.

Example:

Why is path $out/foo removed from this package? [looking in git history] Oh, at some point in the past it contained a broken symlink, but it now it (should) contain a valid file and we didn't notice because there was no build time check for it.

I'd suggest to

1. Revert the hook.
2. Extend the hook to add option(s) to delete the erronous paths, via some filter/glob/list/bool.
3. Re-enable the hook.

[K900]

Generally the right thing to do if upstream ships broken symlinks is to just disable the check.

[bjornfor]

Generally the right thing to do if upstream ships broken symlinks is to just disable the check.

But then, what value does the hook bring? But yes, I guess that'd be preferable over nuking the paths, because then we have a semantic connection between the two (hook and workaround/fix).

[K900]

The value of the hook is to catch bad symlinks that are created by our packaging.

[FKouhai]

I guess that's fair, although I think over time more people would just disable the hook

[bjornfor]

The value of the hook is to catch bad symlinks that are created by our packaging.

But if the response to it finding a bad symlink is to disable the check (if we agree that's preferable to blindly deleting files in $out without a build time check/connection to the underlying setup hook), then the hook doesn't provide much value.

Anyway, PRs that break a lot of packages should be reverted, right?

[bjornfor]

Btw, I like the idea behind the hook, and would like to see it re-applied, but without the breakage and "mass PRs that work around the hook in non-obvious ways".

[wolfgangwalther]

But if the response to it finding a bad symlink is to disable the check (if we agree that's preferable to blindly deleting files in $out without a build time check/connection to the underlying setup hook), then the hook doesn't provide much value.

It does still provide value. You can then easily grep for dontCheckForBrokenSymlinks and fix packages 1-by-1 afterwards.

Most importantly, it prevents introducing new broken symlinks.

Anyway, PRs that break a lot of packages should be reverted, right?

You can't easily revert this PR, because it would need to go through staging. So it wouldn't really help short-term to fix those packages, right?

[bjornfor]

But if the response to it finding a bad symlink is to disable the check (if we agree that's preferable to blindly deleting files in $out without a build time check/connection to the underlying setup hook), then the hook doesn't provide much value.

It does still provide value. You can then easily grep for dontCheckForBrokenSymlinks and fix packages 1-by-1 afterwards.

Good. But some PRs I found just rm $out/...

Most importantly, it prevents introducing new broken symlinks.

👍

Anyway, PRs that break a lot of packages should be reverted, right?

You can't easily revert this PR, because it would need to go through staging. So it wouldn't really help short-term to fix those packages, right?

Oh, I guess not.

[wolfgangwalther]

So I guess the general guideline should be here:

• If nuking any dangling symlinks, add code comments to explain why that's the right thing to do.
• If in doubt, disable the check via dontCheckForBrokenSymlinks temporarily to unbreak the build.
• When adding dontCheckForBrokenSymlinks still add a comment explaining what you've found out already, whether it's temporary or whether there's a really good reason to do it permanently etc.
• If you're not getting attention for the respective PR, comment here.

[bjornfor]

I'd suggest to
[...]
2. Extend the hook to add option(s) to delete the erronous paths, via some filter/glob/list/bool.

The hook could also export a bash function to remove given paths, iff they are problematic symlinks. That way we'd get build time check for when the invariant is broken, unlike rm $out/....

[FKouhai]

it could even be a bash one liner using find to search for the broken symlinks and then removing them or just printing them out to have the maintainers removing them if needed