Provide descriptive error messages on external mutable source files encountered during a build

Currently when evaluating a file or symlink leading to an external mutable object, Blaze throws an exception with unclear messages. The message does not contain the actual path but rather [/]/[] instead. This change updates FileFunction to allow bubbling up the error with the accurate path.

--
MOS_MIGRATED_REVID=118381323

1 files changed, 59 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
index da6b9d989a..6eb962114c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.TreeSet;
 import java.util.concurrent.atomic.AtomicReference;
@@ -62,8 +63,19 @@ public class FileFunction implements SkyFunction {
     // symlink cycle, we want to detect that quickly as it gives a more informative error message
     // than we'd get doing bogus filesystem operations.
     if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) {
-      Pair<RootedPath, FileStateValue> resolvedState =
-          resolveFromAncestors(rootedPath, env);
+      Pair<RootedPath, FileStateValue> resolvedState = null;
+
+      try {
+        resolvedState = resolveFromAncestors(rootedPath, env);
+      } catch (FileOutsidePackageRootsException e) {
+        // When getting a FileOutsidePackageRootsException caused by an external file symlink
+        // somewhere in this file's path, rethrow an exception with this file's path, so the error
+        // message mentions this file instead of the first ancestor path where the external file
+        // error is observed
+        throw new FileFunctionException(
+            new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT);
+      }
+
       if (resolvedState == null) {
         return null;
       }
@@ -71,7 +83,18 @@ public class FileFunction implements SkyFunction {
       realFileStateValue = resolvedState.getSecond();
     }
 
-    FileStateValue fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath));
+    FileStateValue fileStateValue = null;
+
+    try {
+      fileStateValue =
+          (FileStateValue)
+              env.getValueOrThrow(
+                  FileStateValue.key(rootedPath), FileOutsidePackageRootsException.class);
+    } catch (FileOutsidePackageRootsException e) {
+      throw new FileFunctionException(
+          new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT);
+    }
+
     if (fileStateValue == null) {
       return null;
     }
@@ -109,15 +132,21 @@ public class FileFunction implements SkyFunction {
    * {@code null} if there was a missing dep.
    */
   @Nullable
-  private Pair<RootedPath, FileStateValue> resolveFromAncestors(RootedPath rootedPath,
-      Environment env) throws FileFunctionException {
+  private Pair<RootedPath, FileStateValue> resolveFromAncestors(
+      RootedPath rootedPath, Environment env)
+      throws FileFunctionException, FileOutsidePackageRootsException {
     PathFragment relativePath = rootedPath.getRelativePath();
     RootedPath realRootedPath = rootedPath;
     FileValue parentFileValue = null;
     if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) {
       RootedPath parentRootedPath = RootedPath.toRootedPath(rootedPath.getRoot(),
           relativePath.getParentDirectory());
-      parentFileValue = (FileValue) env.getValue(FileValue.key(parentRootedPath));
+
+      parentFileValue =
+          (FileValue)
+              env.getValueOrThrow(
+                  FileValue.key(parentRootedPath), FileOutsidePackageRootsException.class);
+
       if (parentFileValue == null) {
         return null;
       }
@@ -125,13 +154,17 @@ public class FileFunction implements SkyFunction {
       RootedPath parentRealRootedPath = parentFileValue.realRootedPath();
       realRootedPath = RootedPath.toRootedPath(parentRealRootedPath.getRoot(),
           parentRealRootedPath.getRelativePath().getRelative(baseName));
+
       if (!parentFileValue.exists()) {
         return Pair.<RootedPath, FileStateValue>of(
             realRootedPath, FileStateValue.NONEXISTENT_FILE_STATE_NODE);
       }
     }
     FileStateValue realFileStateValue =
-        (FileStateValue) env.getValue(FileStateValue.key(realRootedPath));
+        (FileStateValue)
+            env.getValueOrThrow(
+                FileStateValue.key(realRootedPath), FileOutsidePackageRootsException.class);
+
     if (realFileStateValue == null) {
       return null;
     }
@@ -230,8 +263,9 @@ public class FileFunction implements SkyFunction {
               ImmutableList.copyOf(
                   Iterables.concat(symlinkChain, ImmutableList.of(symlinkTargetRootedPath))));
       uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond());
-      fse = new FileSymlinkInfiniteExpansionException(
-          pathAndChain.getFirst(), pathAndChain.getSecond());
+      fse =
+          new FileSymlinkInfiniteExpansionException(
+              pathAndChain.getFirst(), pathAndChain.getSecond());
     }
     if (uniquenessKey != null) {
       if (env.getValue(uniquenessKey) == null) {
@@ -241,7 +275,18 @@ public class FileFunction implements SkyFunction {
       }
       throw new FileFunctionException(Preconditions.checkNotNull(fse, rootedPath));
     }
-    return resolveFromAncestors(symlinkTargetRootedPath, env);
+
+    try {
+      return resolveFromAncestors(symlinkTargetRootedPath, env);
+    } catch (FileOutsidePackageRootsException e) {
+      // At this point we know this file node is a symlink leading to an external file. Mark the
+      // exception to be a specific SymlinkOutsidePackageRootsException. The error will be bubbled
+      // up further but no path information will be updated again. This allows preserving the
+      // information about the symlink crossing the internal/external boundary.
+      throw new FileFunctionException(
+          new SymlinkOutsidePackageRootsException(rootedPath, symlinkTargetRootedPath),
+          Transience.PERSISTENT);
+    }
   }
 
   private static final Predicate<RootedPath> isPathPredicate(final Path path) {
@@ -272,5 +317,9 @@ public class FileFunction implements SkyFunction {
     public FileFunctionException(FileSymlinkException e) {
       super(e, Transience.PERSISTENT);
     }
+
+    public FileFunctionException(IOException e, Transience transience) {
+      super(e, transience);
+    }
   }
 }