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
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 da6b9d9..6eb9621 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.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 @@
// 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 @@
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 @@
* {@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 @@
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 @@
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 @@
}
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 FileFunctionException(FileSymlinkException e) {
super(e, Transience.PERSISTENT);
}
+
+ public FileFunctionException(IOException e, Transience transience) {
+ super(e, transience);
+ }
}
}
