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

stdenv: add no-broken-symlinks hook #370750

Merged

Conversation

ConnorBaker
Copy link
Contributor

@ConnorBaker ConnorBaker commented Jan 3, 2025

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:

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.

Copy link
Contributor

@K900 K900 left a comment

Choose a reason for hiding this comment

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

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?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 6, 2025
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 19, 2025
@philiptaron
Copy link
Contributor

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.

@ConnorBaker ConnorBaker force-pushed the feat/stdenv-no-broken-symlinks-hook branch from 865fa50 to 7aedd5a Compare January 22, 2025 01:33
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jan 22, 2025
@nix-owners
Copy link

nix-owners bot commented Jan 22, 2025

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 ConnorBaker force-pushed the feat/stdenv-no-broken-symlinks-hook branch from 7aedd5a to 229fdf0 Compare January 22, 2025 01:35
@ConnorBaker
Copy link
Contributor Author

Added docs and tests, force-pushed and rebased!

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

@ConnorBaker ConnorBaker requested a review from K900 January 22, 2025 01:37
@nix-owners nix-owners bot removed the request for review from K900 January 22, 2025 01:37
@nix-owners nix-owners bot requested a review from MattSturgeon January 22, 2025 01:51
@K900
Copy link
Contributor

K900 commented Jan 22, 2025

LGTM, should we get this a Hydra jobset, or do we just send it to staging? CC @vcunat.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Requesting change for the tests of relative symlinks, especially the error case due to interpolation.

The rest of the suggestions and thoughts are not blocking.

With regard to merge target, I favor staging rather than the work of setting up a special Hydra jobset, just on the grounds that we think these are generally bugs. If we get a swath of brokenness, we can revert this and we'll be fine, as I see it.

pkgs/test/stdenv/no-broken-symlinks.nix Outdated Show resolved Hide resolved
pkgs/test/stdenv/no-broken-symlinks.nix Outdated Show resolved Hide resolved
pkgs/test/stdenv/no-broken-symlinks.nix Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/no-broken-symlinks.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/no-broken-symlinks.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/no-broken-symlinks.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/no-broken-symlinks.sh Outdated Show resolved Hide resolved
@ConnorBaker
Copy link
Contributor Author

Quick tip for testing:

( PREFIX="$PWD#legacyPackages.x86_64-linux.tests.stdenv.hooks.no-broken-symlinks"; nix eval --json "$PREFIX" | jq --arg prefix "$PREFIX" -cr 'paths(strings) | join(".") | $prefix + "." + .' | nix build --keep-going --no-link --print-out-paths --stdin )

@ConnorBaker
Copy link
Contributor Author

All feedback addressed! @philiptaron if you're happy, please feel free to merge :)

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Looks good, let's see how it tastes in staging. (All tests.stdenv pass.)

@philiptaron philiptaron merged commit 9e56333 into NixOS:staging Jan 23, 2025
23 of 27 checks passed
@vcunat
Copy link
Member

vcunat commented Jan 23, 2025

Not even darwin stdenv passes:
https://hydra.nixos.org/build/286340056

@ConnorBaker
Copy link
Contributor Author

@vcunat any recommendations for where fixes should go? That is, should I add them to this branch and open new PRs, or have separate branches/PRs for fixes?

Additionally, any way to test such changes on Darwin? I have a MacBook Pro laptop, but its beyond its capacity to rebuild staging.

@K900
Copy link
Contributor

K900 commented Jan 23, 2025

Just create new PRs against staging, and ideally post them in the staging room on Matrix so they can get merged quickly.

@vcunat
Copy link
Member

vcunat commented Jan 23, 2025

I'd think that a MacBook Pro would be able to build stdenv from scratch. It would just take some time. Hydra runs just on Mac Minis after all. But I or OfBorg or others could run some builds, too, once there's a commit.

@nix-owners nix-owners bot requested a review from toonn January 23, 2025 08:37
@trofi
Copy link
Contributor

trofi commented Jan 23, 2025

Bisect says aa1405b stdenv: add no-broken-symlinks hook broke systemdMinimal on x86_64-linux staging:

$ nix build --no-link -f. -L systemdMinimal
...
systemd-minimal> auto-patchelf: 0 dependencies could not be satisfied
systemd-minimal> ERROR: noBrokenSymlinks: the symlink /nix/store/ap7bdzzd6lmjs2ll5p5ahs5i61h0mws5-systemd-minimal-257.2/lib/environment.d/99-environment.conf points to a missing target /etc/environment
systemd-minimal> ERROR: noBrokenSymlinks: found 1 dangling symlinks and 0 reflexive symlinks

@trofi
Copy link
Contributor

trofi commented Jan 23, 2025

firefox dependency pkgsCross.wasm32-unknown-none.stdenv.cc is also broken on staging on x86_64-linux as:

# Sorry, don't know a better way to refer it directly:
$ NIXPKGS_ALLOW_UNSUPPORTED_SYSTEM=1 nix build --no-link -f. pkgsCross.wasm32-unknown-none.stdenv.cc
...
wasm32-unknown-none-clang-wrapper> substituteStream() in derivation wasm32-unknown-none-clang-wrapper-19.1.6: WARNING: '--replace' is deprecated, use --replace-{fail,warn,quiet}. (file '/nix/store/n827sk63y2ivpzbmvaknqzvhg1dwr2gh-wasm32-unknown-none-clang-wrapper-19.1.6/nix-support/cc-cflags')
wasm32-unknown-none-clang-wrapper> ERROR: noBrokenSymlinks: the symlink /nix/store/n827sk63y2ivpzbmvaknqzvhg1dwr2gh-wasm32-unknown-none-clang-wrapper-19.1.6/resource-root/share points to a missing target /nix/store/y00382p3x89ycrdri0fqrpwdw8h08mwx-compiler-rt-wasm32-unknown-none-19.1.6/share
wasm32-unknown-none-clang-wrapper> ERROR: noBrokenSymlinks: found 1 dangling symlinks and 0 reflexive symlinks

@philiptaron
Copy link
Contributor

@ConnorBaker, is it backout time or fast-fix time? I'm at work today and can't participate in the fix-fest.

@ConnorBaker
Copy link
Contributor Author

I'd say it's fast-fix time!

@jmbaur has a patch for systemd he's testing right now (one of their Meson build files was missing a conditional guard) and I'm free to fix things right now.

@philiptaron in the interest of unbreaking stuff, would it be reasonable to create PRs with fixes or disable the hook and issues for followup so the additions don't get lost (because we consider breakage under the hook to be caused by bugs)?

@K900
Copy link
Contributor

K900 commented Jan 23, 2025

There is no rush for this staging cycle. Let's just take our time. We will have to eat this elephant eventually anyway.

@ConnorBaker
Copy link
Contributor Author

@vcunat the failure in clang-wrapper you posted here #376043 (comment) is similar to the failure posted here #370750 (comment); I'm going to make a PR to disable it for the wrapper to triage.

@trofi
Copy link
Contributor

trofi commented Jan 23, 2025

bluez is also broken on x86_64-linux staging as:

$ nix build --no-link -f. -L bluez
...
bluez> stripping (with command strip and flags -S -p) in  /nix/store/pa3rqk9i6mj7jvdjf9afpvaa9hdg1q77-bluez-5.79-test/bin
bluez> ERROR: noBrokenSymlinks: the symlink /nix/store/smnbszll1x0hdj8sg11f9mkyfj0h8r5z-bluez-5.79/etc/bluetooth/main.conf points to a missing target /etc/bluetooth/main.conf
bluez> ERROR: noBrokenSymlinks: the symlink /nix/store/smnbszll1x0hdj8sg11f9mkyfj0h8r5z-bluez-5.79/etc/bluetooth/input.conf points to a missing target /etc/bluetooth/input.conf
bluez> ERROR: noBrokenSymlinks: the symlink /nix/store/smnbszll1x0hdj8sg11f9mkyfj0h8r5z-bluez-5.79/etc/bluetooth/network.conf points to a missing target /etc/bluetooth/network.conf
bluez> ERROR: noBrokenSymlinks: found 3 dangling symlinks and 0 reflexive symlinks

@vcunat
Copy link
Member

vcunat commented Jan 27, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants