Make Bazel not raise an error when a symlink points to its own ancestor.
This is only an error when the symlink is to be traversed recursively. E.g. `a/a/a/b` is perfectly acceptable if `a/a` is a symlink to `.`, except if we want to recursively traverse into it during a glob or recursive target pattern expansion.
RELNOTES: Blaze now allows symbolic links that point to their own ancestor unless they are traversed recursively by e.g. a //... recursive target pattern or a recursive glob.
PiperOrigin-RevId: 334982640
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java
index 7c2f416..55c5ab4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java
@@ -102,6 +102,18 @@
public abstract ImmutableList<RootedPath> logicalChainDuringResolution();
/**
+ * If a symlink pointing back to its own ancestor was encountered during the resolution of this
+ * {@link FileValue}, returns the path to it. Otherwise, returns null.
+ */
+ public abstract ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain();
+
+ /**
+ * If a symlink pointing back to its own ancestor was encountered during the resolution of this
+ * {@link FileValue}, returns the symlinks in the cycle. Otherwise, returns null.
+ */
+ public abstract ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain();
+
+ /**
* Returns the real rooted path of the file, taking ancestor symlinks into account. For example,
* the rooted path ['root']/['a/b'] is really ['root']/['c/b'] if 'a' is a symlink to 'c'. Note
* that ancestor symlinks outside the root boundary are not taken into consideration.
@@ -165,6 +177,8 @@
*/
public static FileValue value(
ImmutableList<RootedPath> logicalChainDuringResolution,
+ ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain,
+ ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain,
RootedPath originalRootedPath,
FileStateValue fileStateValueFromAncestors,
RootedPath realRootedPath,
@@ -205,17 +219,36 @@
if (fileStateValueFromAncestors.getType() == FileStateType.SYMLINK) {
PathFragment symlinkTarget = fileStateValueFromAncestors.getSymlinkTarget();
- return shouldStoreChain
- ? new SymlinkFileValueWithStoredChain(
- realRootedPath, realFileStateValue, logicalChainDuringResolution, symlinkTarget)
- : new SymlinkFileValueWithoutStoredChain(
- realRootedPath, realFileStateValue, symlinkTarget);
+ if (pathToUnboundedAncestorSymlinkExpansionChain != null) {
+ return new SymlinkFileValueWithSymlinkCycle(
+ realRootedPath,
+ realFileStateValue,
+ logicalChainDuringResolution,
+ symlinkTarget,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ } else if (shouldStoreChain) {
+ return new SymlinkFileValueWithStoredChain(
+ realRootedPath, realFileStateValue, logicalChainDuringResolution, symlinkTarget);
+ } else {
+ return new SymlinkFileValueWithoutStoredChain(
+ realRootedPath, realFileStateValue, symlinkTarget);
+ }
+ } else {
+ if (pathToUnboundedAncestorSymlinkExpansionChain != null) {
+ return new DifferentRealPathFileValueWithSymlinkCycle(
+ realRootedPath,
+ realFileStateValue,
+ logicalChainDuringResolution,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ } else if (shouldStoreChain) {
+ return new DifferentRealPathFileValueWithStoredChain(
+ realRootedPath, realFileStateValue, logicalChainDuringResolution);
+ } else {
+ return new DifferentRealPathFileValueWithoutStoredChain(realRootedPath, realFileStateValue);
+ }
}
-
- return shouldStoreChain
- ? new DifferentRealPathFileValueWithStoredChain(
- realRootedPath, realFileStateValue, logicalChainDuringResolution)
- : new DifferentRealPathFileValueWithoutStoredChain(realRootedPath, realFileStateValue);
}
/**
@@ -241,6 +274,16 @@
}
@Override
+ public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
public RootedPath realRootedPath() {
return rootedPath;
}
@@ -274,6 +317,84 @@
}
/**
+ * A {@link FileValue} whose resolution required traversing a symlink chain caused by a symlink
+ * pointing to its own ancestor but which eventually points to a real file.
+ */
+ @AutoCodec.VisibleForSerialization
+ @AutoCodec
+ public static class DifferentRealPathFileValueWithSymlinkCycle
+ extends DifferentRealPathFileValueWithStoredChain {
+ // We can't store an exception here because this needs to be serialized, AutoCodec chokes on
+ // object cycles and FilesystemInfiniteSymlinkCycleException somehow sets its cause to itself
+ protected final ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain;
+ protected final ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain;
+
+ public DifferentRealPathFileValueWithSymlinkCycle(
+ RootedPath realRootedPath,
+ FileStateValue realFileStateValue,
+ ImmutableList<RootedPath> logicalChainDuringResolution,
+ ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain,
+ ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain) {
+ super(realRootedPath, realFileStateValue, logicalChainDuringResolution);
+ this.pathToUnboundedAncestorSymlinkExpansionChain =
+ pathToUnboundedAncestorSymlinkExpansionChain;
+ this.unboundedAncestorSymlinkExpansionChain = unboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
+ return pathToUnboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
+ return unboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(
+ realRootedPath,
+ realFileStateValue,
+ logicalChainDuringResolution,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == null) {
+ return false;
+ }
+
+ if (obj.getClass() != DifferentRealPathFileValueWithSymlinkCycle.class) {
+ return false;
+ }
+
+ DifferentRealPathFileValueWithSymlinkCycle other =
+ (DifferentRealPathFileValueWithSymlinkCycle) obj;
+ return realRootedPath.equals(other.realRootedPath)
+ && realFileStateValue.equals(other.realFileStateValue)
+ && logicalChainDuringResolution.equals(other.logicalChainDuringResolution)
+ && pathToUnboundedAncestorSymlinkExpansionChain.equals(
+ other.pathToUnboundedAncestorSymlinkExpansionChain)
+ && unboundedAncestorSymlinkExpansionChain.equals(
+ other.unboundedAncestorSymlinkExpansionChain);
+ }
+
+ @Override
+ public String toString() {
+ return String.format(
+ "symlink ancestor (real_path=%s, real_state=%s, chain=%s, path=%s, cycle=%s)",
+ realRootedPath,
+ realFileStateValue,
+ logicalChainDuringResolution,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ }
+ }
+
+ /**
* Implementation of {@link FileValue} for paths whose fully resolved path is different than the
* requested path, but the path itself is not a symlink. For example, this is the case for the
* path "foo/bar/baz" if at least one of {'foo', 'foo/bar'} is a symlink but 'foo/bar/baz' not.
@@ -310,6 +431,16 @@
}
@Override
+ public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
public boolean equals(Object obj) {
if (obj == null) {
return false;
@@ -370,6 +501,16 @@
}
@Override
+ public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
+ return null;
+ }
+
+ @Override
public boolean equals(Object obj) {
if (obj == null) {
return false;
@@ -396,12 +537,93 @@
}
}
+ /**
+ * A {@link FileValue} whose resolution required traversing a symlink chain caused by a symlink
+ * pointing to its own ancestor and which eventually points to a symlink.
+ */
+ @AutoCodec.VisibleForSerialization
+ @AutoCodec
+ public static final class SymlinkFileValueWithSymlinkCycle
+ extends SymlinkFileValueWithStoredChain {
+ // We can't store an exception here because this needs to be serialized, AutoCodec chokes on
+ // object cycles and FilesystemInfiniteSymlinkCycleException somehow sets its cause to itself
+ private final ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain;
+ private final ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain;
+
+ public SymlinkFileValueWithSymlinkCycle(
+ RootedPath realRootedPath,
+ FileStateValue realFileStateValue,
+ ImmutableList<RootedPath> logicalChainDuringResolution,
+ PathFragment linkTarget,
+ ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain,
+ ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain) {
+ super(realRootedPath, realFileStateValue, logicalChainDuringResolution, linkTarget);
+ this.pathToUnboundedAncestorSymlinkExpansionChain =
+ pathToUnboundedAncestorSymlinkExpansionChain;
+ this.unboundedAncestorSymlinkExpansionChain = unboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
+ return pathToUnboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
+ return unboundedAncestorSymlinkExpansionChain;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(
+ realRootedPath,
+ realFileStateValue,
+ logicalChainDuringResolution,
+ linkTarget,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == null) {
+ return false;
+ }
+
+ if (obj.getClass() != SymlinkFileValueWithSymlinkCycle.class) {
+ return false;
+ }
+
+ SymlinkFileValueWithSymlinkCycle other = (SymlinkFileValueWithSymlinkCycle) obj;
+ return realRootedPath.equals(other.realRootedPath)
+ && realFileStateValue.equals(other.realFileStateValue)
+ && logicalChainDuringResolution.equals(other.logicalChainDuringResolution)
+ && linkTarget.equals(other.linkTarget)
+ && pathToUnboundedAncestorSymlinkExpansionChain.equals(
+ other.pathToUnboundedAncestorSymlinkExpansionChain)
+ && unboundedAncestorSymlinkExpansionChain.equals(
+ other.unboundedAncestorSymlinkExpansionChain);
+ }
+
+ @Override
+ public String toString() {
+ return String.format(
+ "symlink ancestor (real_path=%s, real_state=%s, target=%s, chain=%s, path=%s, cycle=%s)",
+ realRootedPath,
+ realFileStateValue,
+ linkTarget,
+ logicalChainDuringResolution,
+ pathToUnboundedAncestorSymlinkExpansionChain,
+ unboundedAncestorSymlinkExpansionChain);
+ }
+ }
+
/** Implementation of {@link FileValue} for paths that are themselves symlinks. */
@AutoCodec.VisibleForSerialization
@AutoCodec
- public static final class SymlinkFileValueWithStoredChain
+ public static class SymlinkFileValueWithStoredChain
extends DifferentRealPathFileValueWithStoredChain {
- private final PathFragment linkTarget;
+ protected final PathFragment linkTarget;
@VisibleForTesting
public SymlinkFileValueWithStoredChain(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index fa674c4..8062c16 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -1057,6 +1057,7 @@
name = "collect_packages_under_directory_function",
srcs = ["CollectPackagesUnderDirectoryFunction.java"],
deps = [
+ "process_package_directory",
":collect_packages_under_directory_value",
":recursive_directory_traversal_function",
":recursive_pkg_key",
@@ -1524,6 +1525,8 @@
srcs = ["GlobFunction.java"],
deps = [
":directory_listing_value",
+ ":file_symlink_infinite_expansion_exception",
+ ":file_symlink_infinite_expansion_uniqueness_function",
":glob_descriptor",
":glob_value",
":ignored_package_prefixes_value",
@@ -2020,6 +2023,8 @@
":directory_listing_value",
":dirents",
":file_symlink_exception",
+ ":file_symlink_infinite_expansion_exception",
+ ":file_symlink_infinite_expansion_uniqueness_function",
":package_lookup_value",
":process_package_directory_result",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
@@ -2095,6 +2100,8 @@
deps = [
":action_execution_value",
":directory_listing_value",
+ ":file_symlink_infinite_expansion_exception",
+ ":file_symlink_infinite_expansion_uniqueness_function",
":package_lookup_value",
":sky_functions",
":tree_artifact_value",
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 bae2c18..7e0668a 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
@@ -66,11 +66,7 @@
this.nonexistentFileReceiver = nonexistentFileReceiver;
}
- @Override
- public FileValue compute(SkyKey skyKey, Environment env)
- throws FileFunctionException, InterruptedException {
- RootedPath rootedPath = (RootedPath) skyKey.argument();
-
+ private static class SymlinkResolutionState {
// Suppose we have a path p. One of the goals of FileFunction is to resolve the "real path", if
// any, of p. The basic algorithm is to use the fully resolved path of p's parent directory to
// determine the fully resolved path of p. This is complicated when symlinks are involved, and
@@ -91,6 +87,18 @@
// See the usage in checkPathSeenDuringPartialResolutionInternal.
TreeSet<Path> sortedLogicalChain = Sets.newTreeSet();
+ ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain = null;
+ ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain = null;
+
+ private SymlinkResolutionState() {}
+ }
+
+ @Override
+ public FileValue compute(SkyKey skyKey, Environment env)
+ throws FileFunctionException, InterruptedException {
+ RootedPath rootedPath = (RootedPath) skyKey.argument();
+ SymlinkResolutionState symlinkResolutionState = new SymlinkResolutionState();
+
// Fully resolve the path of the parent directory, but only if the current file is not the
// filesystem root (has no parent) or a package path root (treated opaquely and handled by
// skyframe's DiffAwareness interface).
@@ -99,7 +107,7 @@
// an ancestor is part of a symlink cycle, we want to detect that quickly as it gives a more
// informative error message than we'd get doing bogus filesystem operations.
PartialResolutionResult resolveFromAncestorsResult =
- resolveFromAncestors(rootedPath, sortedLogicalChain, logicalChain, env);
+ resolveFromAncestors(rootedPath, symlinkResolutionState, env);
if (resolveFromAncestorsResult == null) {
return null;
}
@@ -107,7 +115,9 @@
FileStateValue fileStateValueFromAncestors = resolveFromAncestorsResult.fileStateValue;
if (fileStateValueFromAncestors.getType() == FileStateType.NONEXISTENT) {
return FileValue.value(
- ImmutableList.copyOf(logicalChain),
+ ImmutableList.copyOf(symlinkResolutionState.logicalChain),
+ symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain,
+ symlinkResolutionState.unboundedAncestorSymlinkExpansionChain,
rootedPath,
FileStateValue.NONEXISTENT_FILE_STATE_NODE,
rootedPathFromAncestors,
@@ -120,11 +130,7 @@
while (realFileStateValue.getType().isSymlink()) {
PartialResolutionResult getSymlinkTargetRootedPathResult =
getSymlinkTargetRootedPath(
- realRootedPath,
- realFileStateValue.getSymlinkTarget(),
- sortedLogicalChain,
- logicalChain,
- env);
+ realRootedPath, realFileStateValue.getSymlinkTarget(), symlinkResolutionState, env);
if (getSymlinkTargetRootedPathResult == null) {
return null;
}
@@ -133,7 +139,9 @@
}
return FileValue.value(
- ImmutableList.copyOf(logicalChain),
+ ImmutableList.copyOf(symlinkResolutionState.logicalChain),
+ symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain,
+ symlinkResolutionState.unboundedAncestorSymlinkExpansionChain,
rootedPath,
// TODO(b/123922036): This is a bug. Should be 'fileStateValueFromAncestors'.
fileStateValueFromAncestors,
@@ -156,24 +164,19 @@
*/
@Nullable
private PartialResolutionResult resolveFromAncestors(
- RootedPath rootedPath,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
- Environment env)
+ RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
throws InterruptedException, FileFunctionException {
RootedPath parentRootedPath = rootedPath.getParentDirectory();
return parentRootedPath != null
- ? resolveFromAncestorsWithParent(
- rootedPath, parentRootedPath, sortedLogicalChain, logicalChain, env)
- : resolveFromAncestorsNoParent(rootedPath, sortedLogicalChain, logicalChain, env);
+ ? resolveFromAncestorsWithParent(rootedPath, parentRootedPath, symlinkResolutionState, env)
+ : resolveFromAncestorsNoParent(rootedPath, symlinkResolutionState, env);
}
@Nullable
private PartialResolutionResult resolveFromAncestorsWithParent(
RootedPath rootedPath,
RootedPath parentRootedPath,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
+ SymlinkResolutionState symlinkResolutionState,
Environment env)
throws InterruptedException, FileFunctionException {
PathFragment relativePath = rootedPath.getRootRelativePath();
@@ -197,7 +200,7 @@
for (RootedPath parentPartialRootedPath : parentFileValue.logicalChainDuringResolution()) {
checkAndNotePathSeenDuringPartialResolution(
- getChild(parentPartialRootedPath, baseName), sortedLogicalChain, logicalChain, env);
+ getChild(parentPartialRootedPath, baseName), symlinkResolutionState, env);
if (env.valuesMissing()) {
return null;
}
@@ -214,12 +217,9 @@
@Nullable
private PartialResolutionResult resolveFromAncestorsNoParent(
- RootedPath rootedPath,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
- Environment env)
+ RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
throws InterruptedException, FileFunctionException {
- checkAndNotePathSeenDuringPartialResolution(rootedPath, sortedLogicalChain, logicalChain, env);
+ checkAndNotePathSeenDuringPartialResolution(rootedPath, symlinkResolutionState, env);
if (env.valuesMissing()) {
return null;
}
@@ -249,8 +249,7 @@
private PartialResolutionResult getSymlinkTargetRootedPath(
RootedPath rootedPath,
PathFragment symlinkTarget,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
+ SymlinkResolutionState symlinkResolutionState,
Environment env)
throws FileFunctionException, InterruptedException {
Path path = rootedPath.asPath();
@@ -265,44 +264,35 @@
: path.getRelative(symlinkTarget);
}
RootedPath symlinkTargetRootedPath = toRootedPath(symlinkTargetPath);
- checkPathSeenDuringPartialResolution(
- symlinkTargetRootedPath, sortedLogicalChain, logicalChain, env);
+ checkPathSeenDuringPartialResolution(symlinkTargetRootedPath, symlinkResolutionState, env);
if (env.valuesMissing()) {
return null;
}
// The symlink target could have a different parent directory, which itself could be a directory
// symlink (or have an ancestor directory symlink)!
- return resolveFromAncestors(symlinkTargetRootedPath, sortedLogicalChain, logicalChain, env);
+ return resolveFromAncestors(symlinkTargetRootedPath, symlinkResolutionState, env);
}
private void checkAndNotePathSeenDuringPartialResolution(
- RootedPath rootedPath,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
- Environment env)
+ RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
throws FileFunctionException, InterruptedException {
Path path = rootedPath.asPath();
- checkPathSeenDuringPartialResolutionInternal(
- rootedPath, path, sortedLogicalChain, logicalChain, env);
- sortedLogicalChain.add(path);
- logicalChain.add(rootedPath);
+ checkPathSeenDuringPartialResolutionInternal(rootedPath, path, symlinkResolutionState, env);
+ symlinkResolutionState.sortedLogicalChain.add(path);
+ symlinkResolutionState.logicalChain.add(rootedPath);
}
private void checkPathSeenDuringPartialResolution(
- RootedPath rootedPath,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
- Environment env)
+ RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
throws FileFunctionException, InterruptedException {
checkPathSeenDuringPartialResolutionInternal(
- rootedPath, rootedPath.asPath(), sortedLogicalChain, logicalChain, env);
+ rootedPath, rootedPath.asPath(), symlinkResolutionState, env);
}
private void checkPathSeenDuringPartialResolutionInternal(
RootedPath rootedPath,
Path path,
- TreeSet<Path> sortedLogicalChain,
- ArrayList<RootedPath> logicalChain,
+ SymlinkResolutionState symlinkResolutionState,
Environment env)
throws FileFunctionException, InterruptedException {
// We are about to perform another step of partial real path resolution. 'logicalChain' is the
@@ -329,12 +319,13 @@
// candidate p for (ii) and (iii).
SkyKey uniquenessKey = null;
FileSymlinkException fse = null;
- Path seenFloorPath = sortedLogicalChain.floor(path);
- Path seenCeilingPath = sortedLogicalChain.ceiling(path);
- if (sortedLogicalChain.contains(path)) {
+ Path seenFloorPath = symlinkResolutionState.sortedLogicalChain.floor(path);
+ Path seenCeilingPath = symlinkResolutionState.sortedLogicalChain.ceiling(path);
+ if (symlinkResolutionState.sortedLogicalChain.contains(path)) {
// 'rootedPath' is [transitively] a symlink to a previous element in the symlink chain (i).
Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain =
- CycleUtils.splitIntoPathAndChain(isPathPredicate(path), logicalChain);
+ CycleUtils.splitIntoPathAndChain(
+ isPathPredicate(path), symlinkResolutionState.logicalChain);
FileSymlinkCycleException fsce =
new FileSymlinkCycleException(pathAndChain.getFirst(), pathAndChain.getSecond());
uniquenessKey = FileSymlinkCycleUniquenessFunction.key(fsce.getCycle());
@@ -345,21 +336,26 @@
Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain =
CycleUtils.splitIntoPathAndChain(
isPathPredicate(seenFloorPath),
- ImmutableList.copyOf(Iterables.concat(logicalChain, ImmutableList.of(rootedPath))));
+ ImmutableList.copyOf(
+ Iterables.concat(
+ symlinkResolutionState.logicalChain, ImmutableList.of(rootedPath))));
uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond());
fse = new FileSymlinkInfiniteExpansionException(
pathAndChain.getFirst(), pathAndChain.getSecond());
} else if (seenCeilingPath != null && seenCeilingPath.startsWith(path)) {
// 'rootedPath' is [transitively] a symlink to an ancestor of a previous element in the
// symlink chain (iii).
- Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain =
- CycleUtils.splitIntoPathAndChain(
- isPathPredicate(seenCeilingPath),
- ImmutableList.copyOf(Iterables.concat(logicalChain, ImmutableList.of(rootedPath))));
- uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond());
- fse =
- new FileSymlinkInfiniteExpansionException(
- pathAndChain.getFirst(), pathAndChain.getSecond());
+ if (symlinkResolutionState.unboundedAncestorSymlinkExpansionChain == null) {
+ Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain =
+ CycleUtils.splitIntoPathAndChain(
+ isPathPredicate(seenCeilingPath),
+ ImmutableList.copyOf(
+ Iterables.concat(
+ symlinkResolutionState.logicalChain, ImmutableList.of(rootedPath))));
+ symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain =
+ pathAndChain.getFirst();
+ symlinkResolutionState.unboundedAncestorSymlinkExpansionChain = pathAndChain.getSecond();
+ }
}
if (uniquenessKey != null) {
// Note that this dependency is merely to ensure that each unique symlink error gets
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
index b832485..8aee295 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
@@ -235,6 +235,28 @@
if (!symlinkFileValue.exists()) {
continue;
}
+
+ // This check is more strict than necessary: we raise an error if globbing traverses into
+ // a directory for any reason, even though it's only necessary if that reason was the
+ // resolution of a recursive glob ("**"). Fixing this would require plumbing the ancestor
+ // symlink information through DirectoryListingValue.
+ if (symlinkFileValue.isDirectory()
+ && symlinkFileValue.unboundedAncestorSymlinkExpansionChain() != null) {
+ SkyKey uniquenessKey =
+ FileSymlinkInfiniteExpansionUniquenessFunction.key(
+ symlinkFileValue.unboundedAncestorSymlinkExpansionChain());
+ env.getValue(uniquenessKey);
+ if (env.valuesMissing()) {
+ return null;
+ }
+
+ FileSymlinkInfiniteExpansionException symlinkException =
+ new FileSymlinkInfiniteExpansionException(
+ symlinkFileValue.pathToUnboundedAncestorSymlinkExpansionChain(),
+ symlinkFileValue.unboundedAncestorSymlinkExpansionChain());
+ throw new GlobFunctionException(symlinkException, Transience.PERSISTENT);
+ }
+
Dirent dirent = symlinkFileMap.get(lookedUpKeyAndValue.getKey());
String fileName = dirent.getName();
if (symlinkFileValue.isDirectory()) {
@@ -414,5 +436,9 @@
public GlobFunctionException(InconsistentFilesystemException e, Transience transience) {
super(e, transience);
}
+
+ public GlobFunctionException(FileSymlinkInfiniteExpansionException e, Transience transience) {
+ super(e, transience);
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
index 0b93eea..c614c18 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
@@ -96,6 +96,23 @@
return ProcessPackageDirectoryResult.EMPTY_RESULT;
}
+ if (fileValue.unboundedAncestorSymlinkExpansionChain() != null) {
+ SkyKey uniquenessKey =
+ FileSymlinkInfiniteExpansionUniquenessFunction.key(
+ fileValue.unboundedAncestorSymlinkExpansionChain());
+ env.getValue(uniquenessKey);
+ if (env.valuesMissing()) {
+ return null;
+ }
+
+ FileSymlinkInfiniteExpansionException symlinkException =
+ new FileSymlinkInfiniteExpansionException(
+ fileValue.pathToUnboundedAncestorSymlinkExpansionChain(),
+ fileValue.unboundedAncestorSymlinkExpansionChain());
+ return reportErrorAndReturn(
+ symlinkException.getMessage(), symlinkException, rootRelativePath, env.getListener());
+ }
+
PackageIdentifier packageId = PackageIdentifier.create(repositoryName, rootRelativePath);
if ((packageId.getRepository().isDefault() || packageId.getRepository().isMain())
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
index 8b27387..3bba228 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -338,6 +338,19 @@
if (env.valuesMissing()) {
throw new MissingDepException();
}
+ if (fileValue.unboundedAncestorSymlinkExpansionChain() != null) {
+ SkyKey uniquenessKey =
+ FileSymlinkInfiniteExpansionUniquenessFunction.key(
+ fileValue.unboundedAncestorSymlinkExpansionChain());
+ env.getValue(uniquenessKey);
+ if (env.valuesMissing()) {
+ throw new MissingDepException();
+ }
+
+ throw new FileSymlinkInfiniteExpansionException(
+ fileValue.pathToUnboundedAncestorSymlinkExpansionChain(),
+ fileValue.unboundedAncestorSymlinkExpansionChain());
+ }
if (fileValue.exists()) {
// If it exists, it may either be a symlink or a file/directory.
PathFragment unresolvedLinkTarget = null;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index dc8113f..4e0ac4c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -459,7 +459,16 @@
@Test
public void testRecursiveNestingSymlink() throws Exception {
symlink("a/a", "../a");
- assertError("a/a/b");
+ file("b");
+ assertNoError("a/a/a/a/b");
+ }
+
+ @Test
+ public void testSimpleUnboundedAncestorSymlinkExpansionChainReported() throws Exception {
+ symlink("a/a", "../a");
+ FileValue v = valueForPath(path("a/a"));
+ assertThat(v.unboundedAncestorSymlinkExpansionChain())
+ .containsExactly(rootedPath("a/a"), rootedPath("a"));
}
@Test
@@ -1103,7 +1112,7 @@
public void testSymlinkToPackagePathBoundary() throws Exception {
Path path = path("this/is/a/path");
FileSystemUtils.ensureSymbolicLink(path, pkgRoot.asPath());
- assertError("this/is/a/path");
+ assertNoError("this/is/a/path");
}
private void runTestSimpleInfiniteSymlinkExpansion(
@@ -1164,20 +1173,24 @@
.setEventHandler(eventHandler)
.build();
EvaluationResult<FileValue> result = driver.evaluate(keys, evaluationContext);
- assertThat(result.hasError()).isTrue();
- for (SkyKey key : errorKeys) {
- ErrorInfo errorInfo = result.getError(key);
- // FileFunction detects infinite symlink expansion explicitly.
- assertThat(errorInfo.getCycleInfo()).isEmpty();
- FileSymlinkInfiniteExpansionException fsiee =
- (FileSymlinkInfiniteExpansionException) errorInfo.getException();
- assertThat(fsiee).hasMessageThat().contains("Infinite symlink expansion");
- assertThat(fsiee.getChain()).containsExactlyElementsIn(expectedChain).inOrder();
+ if (symlinkToAncestor) {
+ assertThat(result.hasError()).isFalse();
+ } else {
+ assertThat(result.hasError()).isTrue();
+ for (SkyKey key : errorKeys) {
+ ErrorInfo errorInfo = result.getError(key);
+ // FileFunction detects infinite symlink expansion explicitly.
+ assertThat(errorInfo.getCycleInfo()).isEmpty();
+ FileSymlinkInfiniteExpansionException fsiee =
+ (FileSymlinkInfiniteExpansionException) errorInfo.getException();
+ assertThat(fsiee).hasMessageThat().contains("Infinite symlink expansion");
+ assertThat(fsiee.getChain()).containsExactlyElementsIn(expectedChain).inOrder();
+ }
+ // Check that the unique symlink expansion error was reported exactly once.
+ assertThat(eventHandler.getEvents()).hasSize(1);
+ assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage())
+ .contains("infinite symlink expansion detected");
}
- // Check that the unique symlink expansion error was reported exactly once.
- assertThat(eventHandler.getEvents()).hasSize(1);
- assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage())
- .contains("infinite symlink expansion detected");
}
@Test
@@ -1206,15 +1219,13 @@
@Test
public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor() throws Exception {
symlink("d", "a");
- Path abPath = directory("a/b");
- Path abcPath = abPath.getChild("c");
+ directory("a/b");
symlink("a/b/c", "../../d/b");
+ symlink("e", "a/b/c");
+ Path fPath = symlink("f", "e");
- RootedPath rootedPathABC = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(abcPath));
- RootedPath rootedPathAB = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(abPath));
- RootedPath rootedPathDB = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("d/b")));
-
- SkyKey keyABC = FileValue.key(rootedPathABC);
+ RootedPath rootedPathF = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(fPath));
+ SkyKey keyF = FileValue.key(rootedPathF);
StoredEventHandler eventHandler = new StoredEventHandler();
SequentialBuildDriver driver = makeDriver();
@@ -1224,25 +1235,16 @@
.setNumThreads(DEFAULT_THREAD_COUNT)
.setEventHandler(eventHandler)
.build();
- EvaluationResult<FileValue> result =
- driver.evaluate(ImmutableList.of(keyABC), evaluationContext);
+ EvaluationResult<FileValue> result = driver.evaluate(ImmutableList.of(keyF), evaluationContext);
- assertThatEvaluationResult(result).hasErrorEntryForKeyThat(keyABC).isNotTransient();
- assertThatEvaluationResult(result)
- .hasErrorEntryForKeyThat(keyABC)
- .hasExceptionThat()
- .isInstanceOf(FileSymlinkInfiniteExpansionException.class);
- FileSymlinkInfiniteExpansionException fiee =
- (FileSymlinkInfiniteExpansionException) result.getError(keyABC).getException();
- assertThat(fiee).hasMessageThat().contains("Infinite symlink expansion");
- assertThat(fiee.getPathToChain()).isEmpty();
- assertThat(fiee.getChain())
- .containsExactly(rootedPathABC, rootedPathDB, rootedPathAB)
+ assertThatEvaluationResult(result).hasNoError();
+ FileValue e = result.get(keyF);
+ assertThat(e.pathToUnboundedAncestorSymlinkExpansionChain())
+ .containsExactly(rootedPath("f"), rootedPath("e"))
.inOrder();
-
- assertThat(eventHandler.getEvents()).hasSize(1);
- assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage())
- .contains("infinite symlink expansion detected");
+ assertThat(e.unboundedAncestorSymlinkExpansionChain())
+ .containsExactly(rootedPath("a/b/c"), rootedPath("d/b"), rootedPath("a/b"))
+ .inOrder();
}
@Test
@@ -1253,10 +1255,6 @@
RootedPath rootedPathDir1AB =
RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir1/a/b")));
- RootedPath rootedPathDir2B =
- RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir2/b")));
- RootedPath rootedPathDir1 = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir1")));
-
SkyKey keyDir1AB = FileValue.key(rootedPathDir1AB);
StoredEventHandler eventHandler = new StoredEventHandler();
@@ -1270,22 +1268,7 @@
EvaluationResult<FileValue> result =
driver.evaluate(ImmutableList.of(keyDir1AB), evaluationContext);
- assertThatEvaluationResult(result).hasErrorEntryForKeyThat(keyDir1AB).isNotTransient();
- assertThatEvaluationResult(result)
- .hasErrorEntryForKeyThat(keyDir1AB)
- .hasExceptionThat()
- .isInstanceOf(FileSymlinkInfiniteExpansionException.class);
- FileSymlinkInfiniteExpansionException fiee =
- (FileSymlinkInfiniteExpansionException) result.getError(keyDir1AB).getException();
- assertThat(fiee).hasMessageThat().contains("Infinite symlink expansion");
- assertThat(fiee.getPathToChain()).isEmpty();
- assertThat(fiee.getChain())
- .containsExactly(rootedPathDir1AB, rootedPathDir2B, rootedPathDir1)
- .inOrder();
-
- assertThat(eventHandler.getEvents()).hasSize(1);
- assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage())
- .contains("infinite symlink expansion detected");
+ assertThatEvaluationResult(result).hasNoError();
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
index 1978e0f..9a93bd31 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -684,6 +684,8 @@
FileValue pkgDirValue =
FileValue.value(
ImmutableList.of(pkgRootedPath),
+ null,
+ null,
pkgRootedPath,
pkgDirFileStateValue,
pkgRootedPath,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java
index f082350..0aa644e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java
@@ -226,7 +226,7 @@
@Test
public void testNonPackageEventsReported() throws Exception {
path("foo").createDirectoryAndParents();
- symlink("foo/infinitesymlinkpkg", path("foo"));
+ symlink("foo/infinitesymlinkpkg", path("foo/infinitesymlinkpkg/subdir"));
PackageIdentifier pkgId = PackageIdentifier.createInMainRepo("foo/infinitesymlinkpkg");
PackageLoader.Result result;
try (PackageLoader pkgLoader = newPackageLoader()) {
diff --git a/src/test/shell/bazel/bazelignore_test.sh b/src/test/shell/bazel/bazelignore_test.sh
index 83b6727..6113c30 100755
--- a/src/test/shell/bazel/bazelignore_test.sh
+++ b/src/test/shell/bazel/bazelignore_test.sh
@@ -98,18 +98,22 @@
|| fail "directory mentioned in .bazelignore not ignored as it should"
}
-test_symlink_loop_ignored() {
+test_symlink_cycle_ignored() {
rm -rf work && mkdir work && cd work
create_workspace_with_default_repos WORKSPACE
mkdir -p ignoreme/deep
(cd ignoreme/deep && ln -s . loop)
touch BUILD
- bazel build ... && fail "Expected failure" || :
+
+ # This should really fail, but it does not:
+ # https://github.com/bazelbuild/bazel/issues/12148
+ bazel build ... >& $TEST_log || fail "Expected success"
+ expect_log "Infinite symlink expansion"
echo; echo
echo ignoreme > .bazelignore
- bazel build ... \
- || fail "directory mentioned in .bazelignore not ignored as it should"
+ bazel build ... >& $TEST_log || fail "Expected success"
+ expect_not_log "Infinite symlink expansion"
}
test_build_specific_target() {
