Skip to content

noBrokenSymlinks: check for unreadable symlinks#380683

Merged
ConnorBaker merged 8 commits intoNixOS:stagingfrom
Rhys-T:fix-no-broken-symlinks-hook
Mar 8, 2025
Merged

noBrokenSymlinks: check for unreadable symlinks#380683
ConnorBaker merged 8 commits intoNixOS:stagingfrom
Rhys-T:fix-no-broken-symlinks-hook

Conversation

@Rhys-T
Copy link
Copy Markdown
Contributor

@Rhys-T Rhys-T commented Feb 9, 2025 •
edited
Loading

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:
  • 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
Copy link
Copy Markdown
Contributor Author

Rhys-T commented Feb 9, 2025

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.

@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Feb 9, 2025
wolfgangwalther
wolfgangwalther previously requested changes Feb 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

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

@Rhys-T Rhys-T force-pushed the fix-no-broken-symlinks-hook branch from 5fd24fd to 42b0ac3 Compare February 21, 2025 21:35
@Rhys-T
Copy link
Copy Markdown
Contributor Author

Rhys-T commented Feb 21, 2025

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…

@Rhys-T Rhys-T force-pushed the fix-no-broken-symlinks-hook branch from 42b0ac3 to 76a3e75 Compare February 21, 2025 21:45
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: vim Advanced text editor 6.topic: stdenv Standard environment 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: php PHP is a general-purpose scripting language geared towards web development. labels Feb 21, 2025
@Rhys-T Rhys-T changed the base branch from master to staging February 21, 2025 21:46
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: vim Advanced text editor 6.topic: stdenv Standard environment 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: php PHP is a general-purpose scripting language geared towards web development. labels Feb 21, 2025
@Rhys-T Rhys-T force-pushed the fix-no-broken-symlinks-hook branch from 98838cc to 32617d7 Compare March 5, 2025 20:46
@Rhys-T
noBrokenSymlinks: apply lib.recurseIntoAttrs to set of tests
0f17943 
For consistency with the outer `tests.stdenv.hooks` attrset.
@Rhys-T Rhys-T force-pushed the fix-no-broken-symlinks-hook branch from 32617d7 to 0f17943 Compare March 5, 2025 20:49
@Rhys-T
Copy link
Copy Markdown
Contributor Author

Rhys-T commented Mar 6, 2025 •
edited
Loading

Working on running the tests for this latest version now, but I think I've finally got it ready at this point…?

Edit: Thought it had failed again, but it's just the Cachix push step choking on the (intentionally) broken symlinks. I think the hook tests themselves are working now!

@ConnorBaker
Copy link
Copy Markdown
Contributor

ConnorBaker commented Mar 8, 2025

Wonderful! I was able to verify it works on x86_64-linux, aarch64-darwin, and x86_64-darwin.

Thank you so much for taking this on!

@ConnorBaker ConnorBaker merged commit bc725a0 into NixOS:staging Mar 8, 2025
27 checks passed
@ConnorBaker ConnorBaker mentioned this pull request Mar 9, 2025
Closed
3 tasks
pwaller added a commit to pwaller/nixpkgs that referenced this pull request Jun 16, 2025
@pwaller
llvmPackages.libunwind: condition doFakeLibgcc on !stdenv.hostPlatfor…
8f10f4f 
…m.isStatic

Since NixOS#380683, broken symlinks prevent the package from building on
pkgsStatic.pkgsLLVM.

So far as I know, isStatic builds don't have shared objects, so don't
emit these broken symlinks for them.

Signed-off-by: Peter Waller <p@pwaller.net>
@pwaller pwaller mentioned this pull request Jun 16, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ConnorBaker ConnorBaker ConnorBaker approved these changes

@philiptaron philiptaron Awaiting requested review from philiptaron

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@emilazy emilazy Awaiting requested review from emilazy

@reckenrode reckenrode Awaiting requested review from reckenrode

@Artturin Artturin Awaiting requested review from Artturin

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

@wolfgangwalther wolfgangwalther Awaiting requested review from wolfgangwalther

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

stdenv: noBrokenSymlinks hook exits silently if readlink fails (e.g. Darwin symlink permissions)

5 participants

@Rhys-T @wolfgangwalther @ConnorBaker @reckenrode @wegank