no-broken-symlinks: provide only dontCheckForBrokenSymlinks and test …

…against absolute symlinks
NixOS · Jan 22, 2025 · 51b2764 · 51b2764
1 parent 229fdf0
commit 51b2764
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 61 deletions.
diff --git a/doc/stdenv/stdenv.chapter.md b/doc/stdenv/stdenv.chapter.md
@@ -1375,9 +1375,11 @@ This hook only runs when compiling for Linux.
 
 This setup hook checks for, reports, and (by default) fails builds when "broken" symlinks are found. A symlink is considered "broken" if it's dangling (the target doesn't exist) or reflexive (it refers to itself).
 
-By setting `allowDanglingSymlinks` or `allowReflexiveSymlinks` the hook can be configured to allow symlinks with non-existent targets or symlinks which are reflexive, respectively.
+This hook can be disabled by setting `dontCheckForBrokenSymlinks`.
 
-This hook can be disabled entirely by setting `dontCheckForBrokenSymlinks`.
+::: {.note}
+The check for reflexivity is direct and does not account for transitivity, so this hook will not prevent cycles in symlinks.
+:::
 
 ### `set-source-date-epoch-to-latest.sh` {#set-source-date-epoch-to-latest.sh}
 

diff --git a/pkgs/build-support/setup-hooks/no-broken-symlinks.sh b/pkgs/build-support/setup-hooks/no-broken-symlinks.sh
@@ -19,11 +19,10 @@ noBrokenSymlinks() {
   local path
   local pathParent
   local symlinkTarget
-  local errorMessage
   local -i numDanglingSymlinks=0
   local -i numReflexiveSymlinks=0
 
-  # TODO(@connorbaker): This hook doesn't check for cycles in symlinks.
+  # NOTE(@connorbaker): This hook doesn't check for cycles in symlinks.
 
   if [[ ! -e $output ]]; then
     nixWarnLog "skipping non-existent output $output"
@@ -52,28 +51,13 @@ noBrokenSymlinks() {
     fi
 
     if [[ $path == "$symlinkTarget" ]]; then
-      # symlinkTarget is reflexive
-      errorMessage="the symlink $path is reflexive $symlinkTarget"
-      if [[ -z ${allowReflexiveSymlinks-} ]]; then
-        nixErrorLog "$errorMessage"
-        numReflexiveSymlinks+=1
-      else
-        nixInfoLog "$errorMessage"
-      fi
-
+      nixErrorLog "the symlink $path is reflexive $symlinkTarget"
+      numReflexiveSymlinks+=1
     elif [[ ! -e $symlinkTarget ]]; then
-      # symlinkTarget does not exist
-      errorMessage="the symlink $path points to a missing target $symlinkTarget"
-      if [[ -z ${allowDanglingSymlinks-} ]]; then
-        nixErrorLog "$errorMessage"
-        numDanglingSymlinks+=1
-      else
-        nixInfoLog "$errorMessage"
-      fi
-
+      nixErrorLog "the symlink $path points to a missing target $symlinkTarget"
+      numDanglingSymlinks+=1
     else
-      # symlinkTarget exists and is irreflexive
-      nixInfoLog "the symlink $path is irreflexive and points to a target which exists"
+      nixDebugLog "the symlink $path is irreflexive and points to a target which exists"
     fi
   done < <(find "$output" -type l -print0)
 

diff --git a/pkgs/test/stdenv/no-broken-symlinks.nix b/pkgs/test/stdenv/no-broken-symlinks.nix
@@ -9,17 +9,17 @@ let
   inherit (pkgs) runCommand;
   inherit (pkgs.testers) testBuildFailure;
 
-  mkDanglingSymlink = ''
-    ln -sr "$out/dangling" "$out/dangling-symlink"
+  mkDanglingSymlink = absolute: ''
+    ln -s${if absolute then "r" else ""} "$out/dangling" "$out/dangling-symlink"
   '';
 
-  mkReflexiveSymlink = ''
-    ln -sr "$out/reflexive-symlink" "$out/reflexive-symlink"
+  mkReflexiveSymlink = absolute: ''
+    ln -s${if absolute then "r" else ""} "$out/reflexive-symlink" "$out/reflexive-symlink"
   '';
 
-  mkValidSymlink = ''
+  mkValidSymlink = absolute: ''
     touch "$out/valid"
-    ln -sr "$out/valid" "$out/valid-symlink"
+    ln -s${if absolute then "r" else ""} "$out/valid" "$out/valid-symlink"
   '';
 
   testBuilder =
@@ -47,13 +47,12 @@ let
     );
 in
 {
-  # Dangling symlinks (allowDanglingSymlinks)
-  fail-dangling-symlink =
-    runCommand "fail-dangling-symlink"
+  fail-dangling-symlink-relative =
+    runCommand "fail-dangling-symlink-relative"
       {
         failed = testBuildFailure (testBuilder {
-          name = "fail-dangling-symlink-inner";
-          commands = [ mkDanglingSymlink ];
+          name = "fail-dangling-symlink-relative-inner";
+          commands = [ (mkDanglingSymlink false) ];
         });
       }
       ''
@@ -62,19 +61,38 @@ in
         touch $out
       '';
 
-  pass-dangling-symlink-allowed = testBuilder {
-    name = "pass-symlink-dangling-allowed";
-    commands = [ mkDanglingSymlink ];
-    derivationArgs.allowDanglingSymlinks = true;
+  pass-dangling-symlink-relative-allowed = testBuilder {
+    name = "pass-dangling-symlink-relative-allowed";
+    commands = [ (mkDanglingSymlink false) ];
+    derivationArgs.dontCheckForBrokenSymlinks = true;
+  };
+
+  fail-dangling-symlink-absolute =
+    runCommand "fail-dangling-symlink-absolute"
+      {
+        failed = testBuildFailure (testBuilder {
+          name = "fail-dangling-symlink-absolute-inner";
+          commands = [ (mkDanglingSymlink true) ];
+        });
+      }
+      ''
+        (( 1 == "$(cat "$failed/testBuildFailure.exit")" ))
+        grep -F 'found 1 dangling symlinks and 0 reflexive symlinks' "$failed/testBuildFailure.log"
+        touch $out
+      '';
+
+  pass-dangling-symlink-absolute-allowed = testBuilder {
+    name = "pass-dangling-symlink-absolute-allowed";
+    commands = [ (mkDanglingSymlink true) ];
+    derivationArgs.dontCheckForBrokenSymlinks = true;
   };
 
-  # Reflexive symlinks (allowReflexiveSymlinks)
-  fail-reflexive-symlink =
-    runCommand "fail-reflexive-symlink"
+  fail-reflexive-symlink-relative =
+    runCommand "fail-reflexive-symlink-relative"
       {
         failed = testBuildFailure (testBuilder {
-          name = "fail-reflexive-symlink-inner";
-          commands = [ mkReflexiveSymlink ];
+          name = "fail-reflexive-symlink-relative-inner";
+          commands = [ (mkReflexiveSymlink false) ];
         });
       }
       ''
@@ -83,21 +101,40 @@ in
         touch $out
       '';
 
-  pass-reflexive-symlink-allowed = testBuilder {
-    name = "pass-reflexive-symlink-allowed";
-    commands = [ mkReflexiveSymlink ];
-    derivationArgs.allowReflexiveSymlinks = true;
+  pass-reflexive-symlink-relative-allowed = testBuilder {
+    name = "pass-reflexive-symlink-relative-allowed";
+    commands = [ (mkReflexiveSymlink false) ];
+    derivationArgs.dontCheckForBrokenSymlinks = true;
   };
 
-  # Global (dontCheckForBrokenSymlinks)
-  fail-broken-symlinks =
-    runCommand "fail-broken-symlinks"
+  fail-reflexive-symlink-absolute =
+    runCommand "fail-reflexive-symlink-absolute"
       {
         failed = testBuildFailure (testBuilder {
-          name = "fail-broken-symlinks-inner";
+          name = "fail-reflexive-symlink-absolute-inner";
+          commands = [ (mkReflexiveSymlink true) ];
+        });
+      }
+      ''
+        (( 1 == "$(cat "$failed/testBuildFailure.exit")" ))
+        grep -F 'found 0 dangling symlinks and 1 reflexive symlinks' "$failed/testBuildFailure.log"
+        touch $out
+      '';
+
+  pass-reflexive-symlink-absolute-allowed = testBuilder {
+    name = "pass-reflexive-symlink-absolute-allowed";
+    commands = [ (mkReflexiveSymlink true) ];
+    derivationArgs.dontCheckForBrokenSymlinks = true;
+  };
+
+  fail-broken-symlinks-relative =
+    runCommand "fail-broken-symlinks-relative"
+      {
+        failed = testBuildFailure (testBuilder {
+          name = "fail-broken-symlinks-relative-inner";
           commands = [
-            mkDanglingSymlink
-            mkReflexiveSymlink
+            (mkDanglingSymlink false)
+            (mkReflexiveSymlink false)
           ];
         });
       }
@@ -107,17 +144,48 @@ in
         touch $out
       '';
 
-  pass-broken-symlinks-allowed = testBuilder {
-    name = "fail-broken-symlinks-allowed";
+  pass-broken-symlinks-relative-allowed = testBuilder {
+    name = "pass-broken-symlinks-relative-allowed";
     commands = [
-      mkDanglingSymlink
-      mkReflexiveSymlink
+      (mkDanglingSymlink false)
+      (mkReflexiveSymlink false)
     ];
     derivationArgs.dontCheckForBrokenSymlinks = true;
   };
 
-  pass-valid-symlink = testBuilder {
-    name = "pass-valid-symlink";
-    commands = [ mkValidSymlink ];
+  fail-broken-symlinks-absolute =
+    runCommand "fail-broken-symlinks-absolute"
+      {
+        failed = testBuildFailure (testBuilder {
+          name = "fail-broken-symlinks-absolute-inner";
+          commands = [
+            (mkDanglingSymlink true)
+            (mkReflexiveSymlink true)
+          ];
+        });
+      }
+      ''
+        (( 1 == "$(cat "$failed/testBuildFailure.exit")" ))
+        grep -F 'found 1 dangling symlinks and 1 reflexive symlinks' "$failed/testBuildFailure.log"
+        touch $out
+      '';
+
+  pass-broken-symlinks-absolute-allowed = testBuilder {
+    name = "pass-broken-symlinks-absolute-allowed";
+    commands = [
+      (mkDanglingSymlink true)
+      (mkReflexiveSymlink true)
+    ];
+    derivationArgs.dontCheckForBrokenSymlinks = true;
+  };
+
+  pass-valid-symlink-relative = testBuilder {
+    name = "pass-valid-symlink-relative";
+    commands = [ (mkValidSymlink false) ];
+  };
+
+  pass-valid-symlink-absolute = testBuilder {
+    name = "pass-valid-symlink-absolute";
+    commands = [ (mkValidSymlink true) ];
   };
 }