diff options
Diffstat (limited to 'src')
4 files changed, 134 insertions, 9 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Globber.java b/src/main/java/com/google/devtools/build/lib/packages/Globber.java index cd337a1f27..64d61796d3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Globber.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Globber.java @@ -19,10 +19,10 @@ import java.util.List; /** Interface for evaluating globs during package loading. */ public interface Globber { /** An opaque token for fetching the result of a glob computation. */ - abstract static class Token {} + abstract class Token {} /** Used to indicate an invalid glob pattern. */ - static class BadGlobException extends Exception { + class BadGlobException extends Exception { public BadGlobException(String message) { super(message); } @@ -38,7 +38,11 @@ public interface Globber { Token runAsync(List<String> includes, List<String> excludes, boolean excludeDirs) throws BadGlobException; - /** Fetches the result of a previously started glob computation. */ + /** + * Fetches the result of a previously started glob computation. The returned list must be ordered + * deterministically. For more obvious correctness, implementations should generally sort the list + * they return. + */ List<String> fetch(Token token) throws IOException, InterruptedException; /** Should be called when the globber is about to be discarded due to an interrupt. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java index 5f8eaa90ea..cd4dd85f34 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java @@ -47,7 +47,8 @@ public final class GlobValue implements SkyValue { } /** - * Returns glob matches. + * Returns glob matches. The matches will be in a deterministic but unspecified order. If a + * particular order is required, the returned iterable should be sorted. */ public NestedSet<PathFragment> getMatches() { return matches; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 5d68e3f0dc..ad2f501612 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -68,6 +68,7 @@ import com.google.devtools.build.skyframe.ValueOrException4; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -1079,28 +1080,44 @@ public class PackageFunction implements SkyFunction { private List<String> resolve(Globber delegate) throws IOException, InterruptedException { LinkedHashSet<String> matches = Sets.newLinkedHashSet(); + // Skyframe globbing does not sort the list of results alphabetically, while legacy globbing + // does. To avoid non-deterministic sorting, we sort the result if it has any skyframe + // globs. We must also sort if merging legacy and Skyframe globs, since the end result + // should be deterministically ordered. + boolean needsSorting = false; for (SkyKey includeGlobKey : includesGlobKeys) { // TODO(bazel-team): NestedSet expansion here is suboptimal. for (PathFragment match : getGlobMatches(includeGlobKey, globValueMap)) { + needsSorting = true; matches.add(match.getPathString()); } } matches.addAll(delegate.fetch(delegateIncludesToken)); for (SkyKey excludeGlobKey : excludesGlobKeys) { for (PathFragment match : getGlobMatches(excludeGlobKey, globValueMap)) { + needsSorting = true; matches.remove(match.getPathString()); } } for (String delegateExcludeMatch : delegate.fetch(delegateExcludesToken)) { matches.remove(delegateExcludeMatch); } - return Lists.newArrayList(matches); + List<String> result = new ArrayList<>(matches); + if (needsSorting) { + Collections.sort(result); + } + return result; } - private Iterable<PathFragment> getGlobMatches(SkyKey globKey, - Map<SkyKey, ValueOrException4<IOException, BuildFileNotFoundException, - FileSymlinkCycleException, InconsistentFilesystemException>> globValueMap) - throws IOException { + private static Iterable<PathFragment> getGlobMatches( + SkyKey globKey, + Map< + SkyKey, + ValueOrException4< + IOException, BuildFileNotFoundException, FileSymlinkCycleException, + InconsistentFilesystemException>> + globValueMap) + throws IOException { ValueOrException4<IOException, BuildFileNotFoundException, FileSymlinkCycleException, InconsistentFilesystemException> valueOrException = Preconditions.checkNotNull(globValueMap.get(globKey), "%s should not be missing", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index cf95adb297..119205af5d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java @@ -330,6 +330,109 @@ public class PackageFunctionTest extends BuildViewTestCase { Label.parseAbsolute("//bar:a"), Label.parseAbsolute("//baz:c")); } + @SuppressWarnings("unchecked") // Cast of srcs attribute to Iterable<Label>. + @Test + public void testGlobOrderStable() throws Exception { + scratch.file("foo/BUILD", "sh_library(name = 'foo', srcs = glob(['**/*.txt']))"); + scratch.file("foo/b.txt"); + scratch.file("foo/c/c.txt"); + preparePackageLoading(rootDirectory); + SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); + PackageValue value = validPackage(skyKey); + assertThat( + (Iterable<Label>) + value + .getPackage() + .getTarget("foo") + .getAssociatedRule() + .getAttributeContainer() + .getAttr("srcs")) + .containsExactly( + Label.parseAbsoluteUnchecked("//foo:b.txt"), + Label.parseAbsoluteUnchecked("//foo:c/c.txt")) + .inOrder(); + scratch.file("foo/d.txt"); + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(new PathFragment("foo/d.txt")).build(), + rootDirectory); + value = validPackage(skyKey); + assertThat( + (Iterable<Label>) + value + .getPackage() + .getTarget("foo") + .getAssociatedRule() + .getAttributeContainer() + .getAttr("srcs")) + .containsExactly( + Label.parseAbsoluteUnchecked("//foo:b.txt"), + Label.parseAbsoluteUnchecked("//foo:c/c.txt"), + Label.parseAbsoluteUnchecked("//foo:d.txt")) + .inOrder(); + } + + @SuppressWarnings("unchecked") // Cast of srcs attribute to Iterable<Label>. + @Test + public void testGlobOrderStableWithLegacyAndSkyframeComponents() throws Exception { + scratch.file("foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.txt']))"); + scratch.file("foo/b.txt"); + scratch.file("foo/a.config"); + preparePackageLoading(rootDirectory); + SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); + PackageValue value = validPackage(skyKey); + assertThat( + (Iterable<Label>) + value + .getPackage() + .getTarget("foo") + .getAssociatedRule() + .getAttributeContainer() + .getAttr("srcs")) + .containsExactly(Label.parseAbsoluteUnchecked("//foo:b.txt")); + scratch.overwriteFile( + "foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.txt', '*.config']))"); + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(new PathFragment("foo/BUILD")).build(), + rootDirectory); + value = validPackage(skyKey); + assertThat( + (Iterable<Label>) + value + .getPackage() + .getTarget("foo") + .getAssociatedRule() + .getAttributeContainer() + .getAttr("srcs")) + .containsExactly( + Label.parseAbsoluteUnchecked("//foo:a.config"), + Label.parseAbsoluteUnchecked("//foo:b.txt")) + .inOrder(); + scratch.overwriteFile( + "foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.txt', '*.config'])) # comment"); + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(new PathFragment("foo/BUILD")).build(), + rootDirectory); + value = validPackage(skyKey); + assertThat( + (Iterable<Label>) + value + .getPackage() + .getTarget("foo") + .getAssociatedRule() + .getAttributeContainer() + .getAttr("srcs")) + .containsExactly( + Label.parseAbsoluteUnchecked("//foo:a.config"), + Label.parseAbsoluteUnchecked("//foo:b.txt")) + .inOrder(); + } + @Test public void testIncludeInMainAndDefaultRepository() throws Exception { scratch.file("foo/BUILD", |