noBrokenSymlinks: check for unreadable symlinks

[Rhys-T]

Resolves #380681.

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.

[Rhys-T]

I haven't tested this locally yet because it wants to Rebuild The Everything (well, it is a stdenv change) and I don't have that much free space.

[wolfgangwalther]

Please add a test for this in pkgs/test/stdenv/no-broken-symlinks.nix and target staging.

[Rhys-T]

Hang on, that was the wrong branch, and now I've messed something up on my local repo. (I don't do rebases often enough.) Give me a sec, I'll fix it…

[ConnorBaker]

Should just need to be moved earlier in the list.

[Rhys-T]

Okay, testing it now.

[Rhys-T]

Okay, I've almost got it. The problem now is the pass-*-allowed versions of the test, on macOS/Darwin. The dontCheckForBrokenSymlinks argument successfully skips the noBrokenSymlinks hook… which allows the other hooks to find the unreadable symlink and die during readlink, causing the test to fail. This applies to the unreadable-symlink test and the combined broken-symlinks test, in both the absolute and relative variants. Any suggestions?

[ConnorBaker]

Don't try to read the symlink -- the function should just produce one which is unreadable, not try to read it.

[Rhys-T]

The idea there was to check if I had managed to make it unreadable in the first place - if not, I'm on a platform like Linux that doesn't support permissions for symlinks, and I need to skip this test.

[ConnorBaker]

stdenv.hostPlatform provides booleans like isLinux and isDarwin; I would condition including the check using those rather than having the above in the test.

[ConnorBaker]

Should be good to go after requested changes are addressed.

[ConnorBaker]

I think the function producing the unreadable symlink needs to be reduce so it does exactly what the name says -- it should only produce the symlink, not try to read it.

Broken symlinks are a correctness issue -- it's expected that trying to interact with these broken symlinks should fail, so the tests shouldn't interact with them. The tests only exist to catch and warn about their presence.

Disabling the hook is a way of communicating "I know there are correctness issues with the output(s) but I don't care" so I think the behavior of failing when trying to actually interact with the broken symlinks is valid and that we just need to avoid doing so in the tests.

[Rhys-T]

I think the function producing the unreadable symlink needs to be reduce so it does exactly what the name says -- it should only produce the symlink, not try to read it.

The function may not be the best place for that, but something in the test needs to see whether it's on a platform that supports symlink permissions in the first place - otherwise the Linux version of the test will "fail to fail properly" because noBrokenSymlinks won't have anything to report there (symlinks are always readable).

Disabling the hook is a way of communicating "I know there are correctness issues with the output(s) but I don't care" so I think the behavior of failing when trying to actually interact with the broken symlinks is valid and that we just need to avoid doing so in the tests.

Trying to make sure I'm understanding correctly: Should I remove the pass-*-allowed test for unreadable symlinks (and the corresponding part of the combined broken-symlinks test) because it can't actually succeed, or should it be changed to expect an error, just a different error than the normal fail version? Or is there some other option that I'm missing?

[ConnorBaker]

The function may not be the best place for that, but something in the test needs to see whether it's on a platform that supports symlink permissions in the first place - otherwise the Linux version of the test will "fail to fail properly" because noBrokenSymlinks won't have anything to report there (symlinks are always readable).

See my comment on stdenv.hostPlatform.isLinux, which you can use to conditionally add the test for unreadable symlinks.

Trying to make sure I'm understanding correctly: Should I remove the pass-*-allowed test for unreadable symlinks (and the corresponding part of the combined broken-symlinks test) because it can't actually succeed, or should it be changed to expect an error, just a different error than the normal fail version? Or is there some other option that I'm missing?

The pass-*-allowed tests should succeed because the tests should only verify behavior of the hook: in the case where the hook is disabled and broken symlinks exist, we should expect success (because the hook does not run and so the build does not fail).

[Rhys-T]

See my comment on stdenv.hostPlatform.isLinux, which you can use to conditionally add the test for unreadable symlinks.

Got it. So far it sounds like Darwin and the *BSDs are the ones where I need to worry about symlink permissions, so I'll set it up to run on those for now.

The pass-*-allowed tests should succeed because the tests should only verify behavior of the hook: in the case where the hook is disabled and broken symlinks exist, we should expect success (because the hook does not run and so the build does not fail).

Well, that's the thing: the pass-*-allowed tests should succeed, but they aren't. The test only disables the noBrokenSymlinks hook, not any of the others - so those other hooks end up getting readlink errors.

[ConnorBaker]

Ah, in that case I’d recommends disabling or marking them broken on non-Linux systems.

[Rhys-T]

Is there any point in even having the pass one on Linux? There can never be unreadable symlinks to ignore there, so there's not much for it to test. I guess it at least confirms that the hook isn't breaking things just by being there, though…

Also, for the combined broken-symlinks tests: The current versions in Nixpkgs test the case of having both dangling and reflexive symlinks in the same build, and work on both platforms. I had added a third, unreadable link into that test case. What should I do about the pass versions on non-Linux systems? Should I mark them broken like the unreadable-only ones? Simplify them back to just dangling+reflexive on those platforms, so it can at least do as much testing as it does now? Leave them as just dangling+reflexive everywhere and not worry about unreadables for the combo test?

Sorry for all the questions - I just really want to make sure I'm doing this right, since I'm changing a fairly major component of Nixpkgs.

[ConnorBaker]

No worries! Happy to try to answer them :)

To correct something I wrote earlier -- instead of marking a test as broken, we should instead be populating meta.badPlatforms. So for instance, if you have a negative test case where you make an unreadable symlink and expect it to fail, you would set meta.badPlatforms = platforms.linux.

I'd say keep the pass test for Linux just to make sure the hook isn't breaking in some scary way.

For the combined tests, rather than adding the unreadable symlink to them, I'd recommend making a new combined test which includes the unreadable symlink and setting meta.badPlatforms = platforms.linux -- that way, the current combined tests stay unchanged and non-linux systems have a new test.

Does that help?

[Rhys-T]

I think so, thanks. So to summarize what I'm getting from this:

Symlink type	Test type	Platform	Action
dangling, reflexive	fail, pass	any	Run - leave these doing what they do now
unreadable	fail	Linux	Skip (badPlatforms) since I can't make a symlink fail that way there
unreadable	pass	Linux	Run to make sure the hook isn't totally broken (it won't have anything to report, though)
unreadable	fail	Darwin/*BSD	Run to make sure we can catch unreadable symlinks
unreadable	pass	Darwin/*BSD	Skip (badPlatforms) because even though this hook will be okay, later ones will error out
broken	fail, pass	any	Run - but leave it only testing dangling and reflexive, like it does now
New: all-broken (or something)	fail, pass	any	Run/Skip in the same cases as unreadable, and test for all three types of broken symlinks

Does that sound right?

[ConnorBaker]

Perfect! And thank you for making the table!

Would you mind adding a comment to the test cases similar to what you did in the Action column of your table?
I think it would be helpful for posterity to have some explanation of why we set badPlatforms.

[Rhys-T]

Does this look right? I couldn't find any other example of using meta.badPlatforms on tests. I did see some meta.brokens in pkgs/test/cc-wrapper/hardening.nix, so as long as everything is using lib.meta.availableOn or something else like that, I guess meta.badPlatforms should work too. Not entirely clear on how to test it though. It looks like the cc-wrapper tests have a supported test derivation that pulls in just the non-broken tests in that group as dependencies. Do I need to add something similar for no-broken-symlinks?

[Rhys-T]

(Oops… sorry, I didn't realize it was going to request another batch of reviewers each time I marked it ready.)