Automated rollback of commit 006b4972b433bda280e6982d297da6071b84b88d.
*** Reason for rollback ***
Breaks a number of targets in the nightly.
*** Original change description ***
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: 334579512
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 55c5ab4..7c2f416 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,18 +102,6 @@
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.
@@ -177,8 +165,6 @@
*/
public static FileValue value(
ImmutableList<RootedPath> logicalChainDuringResolution,
- ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain,
- ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain,
RootedPath originalRootedPath,
FileStateValue fileStateValueFromAncestors,
RootedPath realRootedPath,
@@ -219,36 +205,17 @@
if (fileStateValueFromAncestors.getType() == FileStateType.SYMLINK) {
PathFragment symlinkTarget = fileStateValueFromAncestors.getSymlinkTarget();
- 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 SymlinkFileValueWithStoredChain(
+ realRootedPath, realFileStateValue, logicalChainDuringResolution, symlinkTarget)
+ : new SymlinkFileValueWithoutStoredChain(
+ realRootedPath, realFileStateValue, symlinkTarget);
}
+
+ return shouldStoreChain
+ ? new DifferentRealPathFileValueWithStoredChain(
+ realRootedPath, realFileStateValue, logicalChainDuringResolution)
+ : new DifferentRealPathFileValueWithoutStoredChain(realRootedPath, realFileStateValue);
}
/**
@@ -274,16 +241,6 @@
}
@Override
- public ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain() {
- return null;
- }
-
- @Override
- public ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain() {
- return null;
- }
-
- @Override
public RootedPath realRootedPath() {
return rootedPath;
}
@@ -317,84 +274,6 @@
}
/**
- * 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.
@@ -431,16 +310,6 @@
}
@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;
@@ -501,16 +370,6 @@
}
@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;
@@ -537,93 +396,12 @@
}
}
- /**
- * 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 class SymlinkFileValueWithStoredChain
+ public static final class SymlinkFileValueWithStoredChain
extends DifferentRealPathFileValueWithStoredChain {
- protected final PathFragment linkTarget;
+ private 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 8062c16..fa674c4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -1057,7 +1057,6 @@
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",
@@ -1525,8 +1524,6 @@
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",
@@ -2023,8 +2020,6 @@
":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",
@@ -2100,8 +2095,6 @@
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 7e0668a..bae2c18 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,7 +66,11 @@
this.nonexistentFileReceiver = nonexistentFileReceiver;
}
- private static class SymlinkResolutionState {
+ @Override
+ public FileValue compute(SkyKey skyKey, Environment env)
+ throws FileFunctionException, InterruptedException {
+ RootedPath rootedPath = (RootedPath) skyKey.argument();
+
// 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
@@ -87,18 +91,6 @@
// 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).
@@ -107,7 +99,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, symlinkResolutionState, env);
+ resolveFromAncestors(rootedPath, sortedLogicalChain, logicalChain, env);
if (resolveFromAncestorsResult == null) {
return null;
}
@@ -115,9 +107,7 @@
FileStateValue fileStateValueFromAncestors = resolveFromAncestorsResult.fileStateValue;
if (fileStateValueFromAncestors.getType() == FileStateType.NONEXISTENT) {
return FileValue.value(
- ImmutableList.copyOf(symlinkResolutionState.logicalChain),
- symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain,
- symlinkResolutionState.unboundedAncestorSymlinkExpansionChain,
+ ImmutableList.copyOf(logicalChain),
rootedPath,
FileStateValue.NONEXISTENT_FILE_STATE_NODE,
rootedPathFromAncestors,
@@ -130,7 +120,11 @@
while (realFileStateValue.getType().isSymlink()) {
PartialResolutionResult getSymlinkTargetRootedPathResult =
getSymlinkTargetRootedPath(
- realRootedPath, realFileStateValue.getSymlinkTarget(), symlinkResolutionState, env);
+ realRootedPath,
+ realFileStateValue.getSymlinkTarget(),
+ sortedLogicalChain,
+ logicalChain,
+ env);
if (getSymlinkTargetRootedPathResult == null) {
return null;
}
@@ -139,9 +133,7 @@
}
return FileValue.value(
- ImmutableList.copyOf(symlinkResolutionState.logicalChain),
- symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain,
- symlinkResolutionState.unboundedAncestorSymlinkExpansionChain,
+ ImmutableList.copyOf(logicalChain),
rootedPath,
// TODO(b/123922036): This is a bug. Should be 'fileStateValueFromAncestors'.
fileStateValueFromAncestors,
@@ -164,19 +156,24 @@
*/
@Nullable
private PartialResolutionResult resolveFromAncestors(
- RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
+ RootedPath rootedPath,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
+ Environment env)
throws InterruptedException, FileFunctionException {
RootedPath parentRootedPath = rootedPath.getParentDirectory();
return parentRootedPath != null
- ? resolveFromAncestorsWithParent(rootedPath, parentRootedPath, symlinkResolutionState, env)
- : resolveFromAncestorsNoParent(rootedPath, symlinkResolutionState, env);
+ ? resolveFromAncestorsWithParent(
+ rootedPath, parentRootedPath, sortedLogicalChain, logicalChain, env)
+ : resolveFromAncestorsNoParent(rootedPath, sortedLogicalChain, logicalChain, env);
}
@Nullable
private PartialResolutionResult resolveFromAncestorsWithParent(
RootedPath rootedPath,
RootedPath parentRootedPath,
- SymlinkResolutionState symlinkResolutionState,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
Environment env)
throws InterruptedException, FileFunctionException {
PathFragment relativePath = rootedPath.getRootRelativePath();
@@ -200,7 +197,7 @@
for (RootedPath parentPartialRootedPath : parentFileValue.logicalChainDuringResolution()) {
checkAndNotePathSeenDuringPartialResolution(
- getChild(parentPartialRootedPath, baseName), symlinkResolutionState, env);
+ getChild(parentPartialRootedPath, baseName), sortedLogicalChain, logicalChain, env);
if (env.valuesMissing()) {
return null;
}
@@ -217,9 +214,12 @@
@Nullable
private PartialResolutionResult resolveFromAncestorsNoParent(
- RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
+ RootedPath rootedPath,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
+ Environment env)
throws InterruptedException, FileFunctionException {
- checkAndNotePathSeenDuringPartialResolution(rootedPath, symlinkResolutionState, env);
+ checkAndNotePathSeenDuringPartialResolution(rootedPath, sortedLogicalChain, logicalChain, env);
if (env.valuesMissing()) {
return null;
}
@@ -249,7 +249,8 @@
private PartialResolutionResult getSymlinkTargetRootedPath(
RootedPath rootedPath,
PathFragment symlinkTarget,
- SymlinkResolutionState symlinkResolutionState,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
Environment env)
throws FileFunctionException, InterruptedException {
Path path = rootedPath.asPath();
@@ -264,35 +265,44 @@
: path.getRelative(symlinkTarget);
}
RootedPath symlinkTargetRootedPath = toRootedPath(symlinkTargetPath);
- checkPathSeenDuringPartialResolution(symlinkTargetRootedPath, symlinkResolutionState, env);
+ checkPathSeenDuringPartialResolution(
+ symlinkTargetRootedPath, sortedLogicalChain, logicalChain, 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, symlinkResolutionState, env);
+ return resolveFromAncestors(symlinkTargetRootedPath, sortedLogicalChain, logicalChain, env);
}
private void checkAndNotePathSeenDuringPartialResolution(
- RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
+ RootedPath rootedPath,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
+ Environment env)
throws FileFunctionException, InterruptedException {
Path path = rootedPath.asPath();
- checkPathSeenDuringPartialResolutionInternal(rootedPath, path, symlinkResolutionState, env);
- symlinkResolutionState.sortedLogicalChain.add(path);
- symlinkResolutionState.logicalChain.add(rootedPath);
+ checkPathSeenDuringPartialResolutionInternal(
+ rootedPath, path, sortedLogicalChain, logicalChain, env);
+ sortedLogicalChain.add(path);
+ logicalChain.add(rootedPath);
}
private void checkPathSeenDuringPartialResolution(
- RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env)
+ RootedPath rootedPath,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
+ Environment env)
throws FileFunctionException, InterruptedException {
checkPathSeenDuringPartialResolutionInternal(
- rootedPath, rootedPath.asPath(), symlinkResolutionState, env);
+ rootedPath, rootedPath.asPath(), sortedLogicalChain, logicalChain, env);
}
private void checkPathSeenDuringPartialResolutionInternal(
RootedPath rootedPath,
Path path,
- SymlinkResolutionState symlinkResolutionState,
+ TreeSet<Path> sortedLogicalChain,
+ ArrayList<RootedPath> logicalChain,
Environment env)
throws FileFunctionException, InterruptedException {
// We are about to perform another step of partial real path resolution. 'logicalChain' is the
@@ -319,13 +329,12 @@
// candidate p for (ii) and (iii).
SkyKey uniquenessKey = null;
FileSymlinkException fse = null;
- Path seenFloorPath = symlinkResolutionState.sortedLogicalChain.floor(path);
- Path seenCeilingPath = symlinkResolutionState.sortedLogicalChain.ceiling(path);
- if (symlinkResolutionState.sortedLogicalChain.contains(path)) {
+ Path seenFloorPath = sortedLogicalChain.floor(path);
+ Path seenCeilingPath = sortedLogicalChain.ceiling(path);
+ if (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), symlinkResolutionState.logicalChain);
+ CycleUtils.splitIntoPathAndChain(isPathPredicate(path), logicalChain);
FileSymlinkCycleException fsce =
new FileSymlinkCycleException(pathAndChain.getFirst(), pathAndChain.getSecond());
uniquenessKey = FileSymlinkCycleUniquenessFunction.key(fsce.getCycle());
@@ -336,26 +345,21 @@
Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain =
CycleUtils.splitIntoPathAndChain(
isPathPredicate(seenFloorPath),
- ImmutableList.copyOf(
- Iterables.concat(
- symlinkResolutionState.logicalChain, ImmutableList.of(rootedPath))));
+ ImmutableList.copyOf(Iterables.concat(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).
- 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();
- }
+ 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 (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 8aee295..b832485 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,28 +235,6 @@
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()) {
@@ -436,9 +414,5 @@
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 c614c18..0b93eea 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,23 +96,6 @@
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 3bba228..8b27387 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,19 +338,6 @@
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 4e0ac4c..dc8113f 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,16 +459,7 @@
@Test
public void testRecursiveNestingSymlink() throws Exception {
symlink("a/a", "../a");
- 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"));
+ assertError("a/a/b");
}
@Test
@@ -1112,7 +1103,7 @@
public void testSymlinkToPackagePathBoundary() throws Exception {
Path path = path("this/is/a/path");
FileSystemUtils.ensureSymbolicLink(path, pkgRoot.asPath());
- assertNoError("this/is/a/path");
+ assertError("this/is/a/path");
}
private void runTestSimpleInfiniteSymlinkExpansion(
@@ -1173,24 +1164,20 @@
.setEventHandler(eventHandler)
.build();
EvaluationResult<FileValue> result = driver.evaluate(keys, evaluationContext);
- 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");
+ 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");
}
@Test
@@ -1219,13 +1206,15 @@
@Test
public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor() throws Exception {
symlink("d", "a");
- directory("a/b");
+ Path abPath = directory("a/b");
+ Path abcPath = abPath.getChild("c");
symlink("a/b/c", "../../d/b");
- symlink("e", "a/b/c");
- Path fPath = symlink("f", "e");
- RootedPath rootedPathF = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(fPath));
- SkyKey keyF = FileValue.key(rootedPathF);
+ 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);
StoredEventHandler eventHandler = new StoredEventHandler();
SequentialBuildDriver driver = makeDriver();
@@ -1235,16 +1224,25 @@
.setNumThreads(DEFAULT_THREAD_COUNT)
.setEventHandler(eventHandler)
.build();
- EvaluationResult<FileValue> result = driver.evaluate(ImmutableList.of(keyF), evaluationContext);
+ EvaluationResult<FileValue> result =
+ driver.evaluate(ImmutableList.of(keyABC), evaluationContext);
- assertThatEvaluationResult(result).hasNoError();
- FileValue e = result.get(keyF);
- assertThat(e.pathToUnboundedAncestorSymlinkExpansionChain())
- .containsExactly(rootedPath("f"), rootedPath("e"))
+ 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)
.inOrder();
- assertThat(e.unboundedAncestorSymlinkExpansionChain())
- .containsExactly(rootedPath("a/b/c"), rootedPath("d/b"), rootedPath("a/b"))
- .inOrder();
+
+ assertThat(eventHandler.getEvents()).hasSize(1);
+ assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage())
+ .contains("infinite symlink expansion detected");
}
@Test
@@ -1255,6 +1253,10 @@
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();
@@ -1268,7 +1270,22 @@
EvaluationResult<FileValue> result =
driver.evaluate(ImmutableList.of(keyDir1AB), evaluationContext);
- assertThatEvaluationResult(result).hasNoError();
+ 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");
}
@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 9a93bd31..1978e0f 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,8 +684,6 @@
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 0aa644e..f082350 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/infinitesymlinkpkg/subdir"));
+ symlink("foo/infinitesymlinkpkg", path("foo"));
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 6113c30..83b6727 100755
--- a/src/test/shell/bazel/bazelignore_test.sh
+++ b/src/test/shell/bazel/bazelignore_test.sh
@@ -98,22 +98,18 @@
|| fail "directory mentioned in .bazelignore not ignored as it should"
}
-test_symlink_cycle_ignored() {
+test_symlink_loop_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
-
- # 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"
+ bazel build ... && fail "Expected failure" || :
echo; echo
echo ignoreme > .bazelignore
- bazel build ... >& $TEST_log || fail "Expected success"
- expect_not_log "Infinite symlink expansion"
+ bazel build ... \
+ || fail "directory mentioned in .bazelignore not ignored as it should"
}
test_build_specific_target() {
