diff options
11 files changed, 154 insertions, 217 deletions
diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm index 780716cfe1..33e0a4634a 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm @@ -402,7 +402,7 @@ This wildcard can only be used as a complete path segment. For example, and <code>"**.java"</code> are both illegal. No other wildcards are supported. </p> <p> -Glob returns a list of every file in the current package that: +Glob returns a sorted list of every file in the current package that: </p> <ul> <li>Matches at least one pattern in <code>include</code>. </li> diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java index 45e36ed90b..694601d1c4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java +++ b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java @@ -18,7 +18,6 @@ import com.google.common.base.Predicate; import com.google.common.base.Throwables; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; -import com.google.common.collect.Sets; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.ThreadSafety; @@ -32,7 +31,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.LinkedHashSet; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -126,26 +125,26 @@ public class GlobCache { * @throws BadGlobException if the glob was syntactically invalid, or * contained uplevel references. */ - Future<List<Path>> getGlobAsync(String pattern, boolean excludeDirs) + Future<List<Path>> getGlobUnsortedAsync(String pattern, boolean excludeDirs) throws BadGlobException { Future<List<Path>> cached = globCache.get(Pair.of(pattern, excludeDirs)); if (cached == null) { - cached = safeGlob(pattern, excludeDirs); + cached = safeGlobUnsorted(pattern, excludeDirs); setGlobPaths(pattern, excludeDirs, cached); } return cached; } @VisibleForTesting - List<String> getGlob(String pattern) + List<String> getGlobUnsorted(String pattern) throws IOException, BadGlobException, InterruptedException { - return getGlob(pattern, false); + return getGlobUnsorted(pattern, false); } @VisibleForTesting - protected List<String> getGlob(String pattern, boolean excludeDirs) + protected List<String> getGlobUnsorted(String pattern, boolean excludeDirs) throws IOException, BadGlobException, InterruptedException { - Future<List<Path>> futureResult = getGlobAsync(pattern, excludeDirs); + Future<List<Path>> futureResult = getGlobUnsortedAsync(pattern, excludeDirs); List<Path> globPaths = fromFuture(futureResult); // Replace the UnixGlob.GlobFuture with a completed future object, to allow // garbage collection of the GlobFuture and GlobVisitor objects. @@ -168,11 +167,8 @@ public class GlobCache { return result; } - /** - * Adds glob entries to the cache, making sure they are sorted first. - */ - @VisibleForTesting - void setGlobPaths(String pattern, boolean excludeDirectories, Future<List<Path>> result) { + /** Adds glob entries to the cache. */ + private void setGlobPaths(String pattern, boolean excludeDirectories, Future<List<Path>> result) { globCache.put(Pair.of(pattern, excludeDirectories), result); } @@ -181,7 +177,7 @@ public class GlobCache { * getGlob(). */ @VisibleForTesting - Future<List<Path>> safeGlob(String pattern, boolean excludeDirs) throws BadGlobException { + Future<List<Path>> safeGlobUnsorted(String pattern, boolean excludeDirs) throws BadGlobException { // Forbidden patterns: if (pattern.indexOf('?') != -1) { throw new BadGlobException("glob pattern '" + pattern + "' contains forbidden '?' wildcard"); @@ -217,64 +213,28 @@ public class GlobCache { } /** - * Returns true iff all this package's globs are up-to-date. That is, - * re-evaluating the package's BUILD file at this moment would yield an - * equivalent Package instance. (This call requires filesystem I/O to - * re-evaluate the globs.) - */ - public boolean globsUpToDate() throws InterruptedException { - // Start all globs in parallel. - Map<Pair<String, Boolean>, Future<List<Path>>> newGlobs = new HashMap<>(); - try { - for (Map.Entry<Pair<String, Boolean>, Future<List<Path>>> entry : globCache.entrySet()) { - Pair<String, Boolean> key = entry.getKey(); - try { - newGlobs.put(key, safeGlob(key.first, key.second)); - } catch (BadGlobException e) { - return false; - } - } - - for (Map.Entry<Pair<String, Boolean>, Future<List<Path>>> entry : globCache.entrySet()) { - try { - Pair<String, Boolean> key = entry.getKey(); - List<Path> newGlob = fromFuture(newGlobs.get(key)); - List<Path> oldGlob = fromFuture(entry.getValue()); - if (!oldGlob.equals(newGlob)) { - return false; - } - } catch (IOException e) { - return false; - } - } - - return true; - } finally { - finishBackgroundTasks(newGlobs.values()); - } - } - - /** - * Evaluate the build language expression "glob(includes, excludes)" in the + * Helper for evaluating the build language expression "glob(includes, excludes)" in the * context of this package. * * <p>Called by PackageFactory via Package. */ - public List<String> glob(List<String> includes, List<String> excludes, boolean excludeDirs) - throws IOException, BadGlobException, InterruptedException { + public List<String> globUnsorted( + List<String> includes, + List<String> excludes, + boolean excludeDirs) throws IOException, BadGlobException, InterruptedException { // Start globbing all patterns in parallel. The getGlob() calls below will // block on an individual pattern's results, but the other globs can // continue in the background. for (String pattern : Iterables.concat(includes, excludes)) { - getGlobAsync(pattern, excludeDirs); + getGlobUnsortedAsync(pattern, excludeDirs); } - LinkedHashSet<String> results = Sets.newLinkedHashSetWithExpectedSize(includes.size()); + HashSet<String> results = new HashSet<>(); for (String pattern : includes) { - results.addAll(getGlob(pattern, excludeDirs)); + results.addAll(getGlobUnsorted(pattern, excludeDirs)); } for (String pattern : excludes) { - for (String excludeMatch : getGlob(pattern, excludeDirs)) { + for (String excludeMatch : getGlobUnsorted(pattern, excludeDirs)) { results.remove(excludeMatch); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index 9c1a5c8a0c..4b8db490f3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -70,6 +70,7 @@ import com.google.devtools.build.lib.vfs.UnixGlob; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -285,9 +286,11 @@ public final class PackageFactory { /** {@link Globber} that uses the legacy GlobCache. */ public static class LegacyGlobber implements Globber { private final GlobCache globCache; + private final boolean sort; - public LegacyGlobber(GlobCache globCache) { + private LegacyGlobber(GlobCache globCache, boolean sort) { this.globCache = globCache; + this.sort = sort; } private static class Token extends Globber.Token { @@ -306,20 +309,25 @@ public final class PackageFactory { public Token runAsync(List<String> includes, List<String> excludes, boolean excludeDirs) throws BadGlobException { for (String pattern : Iterables.concat(includes, excludes)) { - globCache.getGlobAsync(pattern, excludeDirs); + globCache.getGlobUnsortedAsync(pattern, excludeDirs); } return new Token(includes, excludes, excludeDirs); } @Override public List<String> fetch(Globber.Token token) throws IOException, InterruptedException { + List<String> result; Token legacyToken = (Token) token; try { - return globCache.glob(legacyToken.includes, legacyToken.excludes, + result = globCache.globUnsorted(legacyToken.includes, legacyToken.excludes, legacyToken.excludeDirs); } catch (BadGlobException e) { throw new IllegalStateException(e); } + if (sort) { + Collections.sort(result); + } + return result; } @Override @@ -1428,12 +1436,33 @@ public final class PackageFactory { } } - public LegacyGlobber createLegacyGlobber(Path packageDirectory, PackageIdentifier packageId, + /** Returns a new {@link LegacyGlobber}. */ + public LegacyGlobber createLegacyGlobber( + Path packageDirectory, + PackageIdentifier packageId, CachingPackageLocator locator) { - return new LegacyGlobber(new GlobCache(packageDirectory, packageId, locator, syscalls, + return createLegacyGlobber(new GlobCache(packageDirectory, packageId, locator, syscalls, threadPool)); } + /** Returns a new {@link LegacyGlobber}. */ + public static LegacyGlobber createLegacyGlobber(GlobCache globCache) { + return new LegacyGlobber(globCache, /*sort=*/ true); + } + + /** + * Returns a new {@link LegacyGlobber}, the same as in {@link #createLegacyGlobber}, except that + * the implementation of {@link Globber#fetch} intentionally breaks the contract and doesn't + * return sorted results. + */ + public LegacyGlobber createLegacyGlobberThatDoesntSort( + Path packageDirectory, + PackageIdentifier packageId, + CachingPackageLocator locator) { + return new LegacyGlobber(new GlobCache(packageDirectory, packageId, locator, syscalls, + threadPool), /*sort=*/ false); + } + @Nullable private byte[] maybeGetBuildFileBytes(Path buildFile, EventHandler eventHandler) { try { 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 54edcf52e0..733cf5abd3 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 @@ -25,6 +25,7 @@ import com.google.common.collect.Sets; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.events.StoredEventHandler; @@ -36,6 +37,7 @@ import com.google.devtools.build.lib.packages.InvalidPackageNameException; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.PackageFactory; +import com.google.devtools.build.lib.packages.PackageFactory.LegacyGlobber; import com.google.devtools.build.lib.packages.Preprocessor; import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing; import com.google.devtools.build.lib.packages.RuleVisibility; @@ -69,6 +71,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -151,10 +154,10 @@ public class PackageFunction implements SkyFunction { private final T value; private final Set<SkyKey> globDepKeys; @Nullable - private final Globber legacyGlobber; + private final LegacyGlobber legacyGlobber; private CacheEntryWithGlobDeps(T value, Set<SkyKey> globDepKeys, - @Nullable Globber legacyGlobber) { + @Nullable LegacyGlobber legacyGlobber) { this.value = value; this.globDepKeys = globDepKeys; this.legacyGlobber = legacyGlobber; @@ -899,15 +902,15 @@ public class PackageFunction implements SkyFunction { private final PackageIdentifier packageId; private final Path packageRoot; private final Environment env; - private final Globber delegate; + private final LegacyGlobber legacyGlobber; private final Set<SkyKey> globDepsRequested = Sets.newConcurrentHashSet(); private SkyframeHybridGlobber(PackageIdentifier packageId, Path packageRoot, Environment env, - Globber delegate) { + LegacyGlobber legacyGlobber) { this.packageId = packageId; this.packageRoot = packageRoot; this.env = env; - this.delegate = delegate; + this.legacyGlobber = legacyGlobber; } private Set<SkyKey> getGlobDepsRequested() { @@ -972,15 +975,15 @@ public class PackageFunction implements SkyFunction { excludesKeys.remove(missingKey); } } - Token delegateIncludesToken = - delegate.runAsync(includesToDelegate, ImmutableList.<String>of(), excludeDirs); + Token legacyIncludesToken = + legacyGlobber.runAsync(includesToDelegate, ImmutableList.<String>of(), excludeDirs); // See the HybridToken class-comment for why we pass excludesToDelegate as the includes // parameter here. - Token delegateExcludesToken = - delegate.runAsync(excludesToDelegate, ImmutableList.<String>of(), excludeDirs); + Token legacyExcludesToken = + legacyGlobber.runAsync(excludesToDelegate, ImmutableList.<String>of(), excludeDirs); return new HybridToken(globValueMap, includesKeys, excludesKeys, - delegateIncludesToken, delegateExcludesToken); + legacyIncludesToken, legacyExcludesToken); } private Collection<SkyKey> getMissingKeys(Collection<SkyKey> globKeys, @@ -1008,17 +1011,17 @@ public class PackageFunction implements SkyFunction { @Override public List<String> fetch(Token token) throws IOException, InterruptedException { HybridToken hybridToken = (HybridToken) token; - return hybridToken.resolve(delegate); + return hybridToken.resolve(legacyGlobber); } @Override public void onInterrupt() { - delegate.onInterrupt(); + legacyGlobber.onInterrupt(); } @Override public void onCompletion() { - delegate.onCompletion(); + legacyGlobber.onCompletion(); } /** @@ -1062,9 +1065,9 @@ public class PackageFunction implements SkyFunction { // (this is excludes_sky above). private final Iterable<SkyKey> excludesGlobKeys; // A token for computing includes_leg. - private final Token delegateIncludesToken; + private final Token legacyIncludesToken; // A token for computing excludes_leg. - private final Token delegateExcludesToken; + private final Token legacyExcludesToken; private HybridToken(Map<SkyKey, ValueOrException4<IOException, BuildFileNotFoundException, FileSymlinkCycleException, InconsistentFilesystemException>> globValueMap, @@ -1073,42 +1076,35 @@ public class PackageFunction implements SkyFunction { this.globValueMap = globValueMap; this.includesGlobKeys = includesGlobKeys; this.excludesGlobKeys = excludesGlobKeys; - this.delegateIncludesToken = delegateIncludesToken; - this.delegateExcludesToken = delegateExcludesToken; + this.legacyIncludesToken = delegateIncludesToken; + this.legacyExcludesToken = delegateExcludesToken; } 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; + HashSet<String> matches = new HashSet<>(); 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)); + matches.addAll(delegate.fetch(legacyIncludesToken)); for (SkyKey excludeGlobKey : excludesGlobKeys) { for (PathFragment match : getGlobMatches(excludeGlobKey, globValueMap)) { - needsSorting = true; matches.remove(match.getPathString()); } } - for (String delegateExcludeMatch : delegate.fetch(delegateExcludesToken)) { + for (String delegateExcludeMatch : delegate.fetch(legacyExcludesToken)) { matches.remove(delegateExcludeMatch); } List<String> result = new ArrayList<>(matches); - if (needsSorting) { - Collections.sort(result); - } + // Skyframe glob results are unsorted. And we used a LegacyGlobber that doesn't sort. + // Therefore, we want to unconditionally sort here. + Collections.sort(result); return result; } - private static Iterable<PathFragment> getGlobMatches( + private static NestedSet<PathFragment> getGlobMatches( SkyKey globKey, Map< SkyKey, @@ -1122,7 +1118,6 @@ public class PackageFunction implements SkyFunction { Preconditions.checkNotNull(globValueMap.get(globKey), "%s should not be missing", globKey); try { - // TODO(bazel-team): NestedSet expansion here is suboptimal. return Preconditions.checkNotNull((GlobValue) valueOrException.get(), "%s should not be missing", globKey).getMatches(); } catch (BuildFileNotFoundException | FileSymlinkCycleException @@ -1173,7 +1168,9 @@ public class PackageFunction implements SkyFunction { if (showLoadingProgress.get()) { env.getListener().handle(Event.progress("Loading package: " + packageId)); } - Globber legacyGlobber = packageFactory.createLegacyGlobber( + // We use a LegacyGlobber that doesn't sort the matches for each individual glob pattern, + // since we want to sort the final result anyway. + LegacyGlobber legacyGlobber = packageFactory.createLegacyGlobberThatDoesntSort( buildFilePath.getParentDirectory(), packageId, packageLocator); SkyframeHybridGlobber skyframeGlobber = new SkyframeHybridGlobber(packageId, packageRoot, env, legacyGlobber); @@ -1211,7 +1208,7 @@ public class PackageFunction implements SkyFunction { // legacy globber instance during BUILD file evaluation since the performance argument // below does not apply. Set<SkyKey> globDepsRequested = skyframeGlobber.getGlobDepsRequested(); - Globber legacyGlobberToStore = globDepsRequested.isEmpty() ? null : legacyGlobber; + LegacyGlobber legacyGlobberToStore = globDepsRequested.isEmpty() ? null : legacyGlobber; astCacheEntry = new CacheEntryWithGlobDeps<>( new AstAfterPreprocessing(preprocessingResult, ast, astParsingEventHandler), globDepsRequested, legacyGlobberToStore); @@ -1237,7 +1234,7 @@ public class PackageFunction implements SkyFunction { // If a legacy globber was used to evaluate globs during preprocessing, it's important that // we reuse that globber during BUILD file evaluation for performance, in the case that // globs were fetched lazily during preprocessing. See Preprocessor.Factory#considersGlobs. - Globber legacyGlobber = astCacheEntry.legacyGlobber != null + LegacyGlobber legacyGlobber = astCacheEntry.legacyGlobber != null ? astCacheEntry.legacyGlobber : packageFactory.createLegacyGlobber( buildFilePath.getParentDirectory(), packageId, packageLocator); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java index 6da3925e96..1ea067d20f 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java @@ -23,9 +23,9 @@ import com.google.common.base.Throwables; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; -import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.common.util.concurrent.ForwardingListenableFuture; import com.google.common.util.concurrent.Futures; @@ -55,6 +55,8 @@ import java.util.regex.Pattern; * * <p><code>**</code> gets special treatment in include patterns. If it is used as a complete path * segment it matches the filenames in subdirectories recursively. + * + * <p>Importantly, note that the glob matches are in an unspecified order. */ public final class UnixGlob { private UnixGlob() {} @@ -396,7 +398,7 @@ public final class UnixGlob { } /** - * Executes the glob. + * Executes the glob and returns the result. * * @throws InterruptedException if the thread is interrupted. */ @@ -499,9 +501,10 @@ public final class UnixGlob { } /** - * Performs wildcard globbing: returns the sorted list of filenames that match any of + * Performs wildcard globbing: returns the list of filenames that match any of * {@code patterns} relative to {@code base}. Directories are traversed if and only if they - * match {@code dirPred}. The predicate is also called for the root of the traversal. + * match {@code dirPred}. The predicate is also called for the root of the traversal. The order + * of the returned list is unspecified. * * <p>Patterns may include "*" and "?", but not "[a-z]". * @@ -530,6 +533,10 @@ public final class UnixGlob { return "**".equals(pattern); } + /** + * Same as {@link #glob}, except does so asynchronously and returns a {@link Future} for the + * result. + */ public Future<List<Path>> globAsync(Path base, Collection<String> patterns, boolean excludeDirectories, Predicate<Path> dirPred, FilesystemCalls syscalls) { @@ -635,7 +642,7 @@ public final class UnixGlob { } else if (failure.get() != null) { result.setException(failure.get()); } else { - result.set(Ordering.<Path>natural().immutableSortedCopy(results)); + result.set(ImmutableList.copyOf(results)); } } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java index 11d301f4f8..7285edea34 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java @@ -14,12 +14,9 @@ package com.google.devtools.build.lib.packages; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.common.collect.Lists; -import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.Globber.BadGlobException; import com.google.devtools.build.lib.testutil.Scratch; @@ -115,7 +112,7 @@ public class GlobCacheTest { @Test public void testSafeGlob() throws Exception { - List<Path> paths = cache.safeGlob("*.js", false).get(); + List<Path> paths = cache.safeGlobUnsorted("*.js", false).get(); assertPathsAre(paths, "/workspace/isolated/first.js", "/workspace/isolated/second.js"); } @@ -125,7 +122,7 @@ public class GlobCacheTest { for (String pattern : new String[] { "Foo?.txt", "List{Test}.py", "List(Test).py" }) { try { - cache.safeGlob(pattern, false); + cache.safeGlobUnsorted(pattern, false); fail("Expected pattern " + pattern + " to fail"); } catch (BadGlobException expected) { } @@ -134,13 +131,13 @@ public class GlobCacheTest { @Test public void testGetGlob() throws Exception { - List<String> glob = cache.getGlob("*.js"); + List<String> glob = cache.getGlobUnsorted("*.js"); assertThat(glob).containsExactly("first.js", "second.js"); } @Test public void testGetGlob_subdirectory() throws Exception { - List<String> glob = cache.getGlob("foo/*.js"); + List<String> glob = cache.getGlobUnsorted("foo/*.js"); assertThat(glob).containsExactly("foo/first.js", "foo/second.js"); } @@ -148,148 +145,108 @@ public class GlobCacheTest { public void testGetKeySet() throws Exception { assertThat(cache.getKeySet()).isEmpty(); - cache.getGlob("*.java"); + cache.getGlobUnsorted("*.java"); assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false)); - cache.getGlob("*.java"); + cache.getGlobUnsorted("*.java"); assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false)); - cache.getGlob("*.js"); + cache.getGlobUnsorted("*.js"); assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false)); - cache.getGlob("*.java", true); + cache.getGlobUnsorted("*.java", true); assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false), Pair.of("*.java", true)); try { - cache.getGlob("invalid?"); + cache.getGlobUnsorted("invalid?"); fail("Expected an invalid regex exception"); } catch (BadGlobException expected) { } assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false), Pair.of("*.java", true)); - cache.getGlob("foo/first.*"); + cache.getGlobUnsorted("foo/first.*"); assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.java", true), Pair.of("*.js", false), Pair.of("foo/first.*", false)); } @Test public void testGlob() throws Exception { - assertEmpty(cache.glob(list("*.java"), NONE, false)); + assertEmpty(cache.globUnsorted(list("*.java"), NONE, false)); - assertThat(cache.glob(list("*.*"), NONE, false)).containsExactly("first.js", "first.txt", - "second.js", "second.txt").inOrder(); + assertThat(cache.globUnsorted(list("*.*"), NONE, false)).containsExactly( + "first.js", "first.txt", "second.js", "second.txt"); - assertThat(cache.glob(list("*.*"), list("first.js"), false)).containsExactly("first.txt", - "second.js", "second.txt").inOrder(); + assertThat(cache.globUnsorted(list("*.*"), list("first.js"), false)).containsExactly( + "first.txt", "second.js", "second.txt"); - assertThat(cache.glob(list("*.txt", "first.*"), NONE, false)).containsExactly("first.txt", - "second.txt", "first.js").inOrder(); - } - - @Test - public void testSetGlobPaths() throws Exception { - // This pattern matches no files. - String pattern = "fake*.java"; - assertThat(cache.getKeySet()).doesNotContain(pattern); - - List<String> results = cache.getGlob(pattern, false); - - assertThat(cache.getKeySet()).contains(Pair.of(pattern, false)); - assertThat(results).isEmpty(); - - cache.setGlobPaths(pattern, false, Futures.<List<Path>>immediateFuture(Lists.newArrayList( - scratch.resolve("isolated/fake.txt"), - scratch.resolve("isolated/fake.py")))); - - assertThat(cache.getGlob(pattern, false)).containsExactly("fake.py", "fake.txt"); - } - - @Test - public void testGlobsUpToDate() throws Exception { - assertTrue(cache.globsUpToDate()); - - // Initialize the cache - cache.getGlob("*.txt"); - assertTrue(cache.globsUpToDate()); - - cache.getGlob("*.js"); - assertTrue(cache.globsUpToDate()); - - // Change the filesystem - scratch.file("isolated/third.txt", - "# this is third.txt"); - assertFalse(cache.globsUpToDate()); - - // Fool the cache to observe the method's behavior. - cache.setGlobPaths("*.txt", false, Futures.<List<Path>>immediateFuture(Lists.newArrayList( - scratch.resolve("isolated/first.txt"), - scratch.resolve("isolated/second.txt"), - scratch.resolve("isolated/third.txt")))); - assertTrue(cache.globsUpToDate()); + assertThat(cache.globUnsorted(list("*.txt", "first.*"), NONE, false)).containsExactly( + "first.txt", "second.txt", "first.js"); } @Test public void testRecursiveGlobDoesNotMatchSubpackage() throws Exception { - List<String> glob = cache.getGlob("**/*.js"); + List<String> glob = cache.getGlobUnsorted("**/*.js"); assertThat(glob).containsExactly("first.js", "second.js", "foo/first.js", "bar/first.js", "foo/second.js", "bar/second.js"); } @Test public void testSingleFileExclude_Star() throws Exception { - assertThat(cache.glob(list("*"), list("first.txt"), false)).containsExactly( - "BUILD", "bar", "first.js", "foo", "second.js", "second.txt").inOrder(); + assertThat(cache.globUnsorted(list("*"), list("first.txt"), false)).containsExactly( + "BUILD", "bar", "first.js", "foo", "second.js", "second.txt"); } @Test public void testSingleFileExclude_StarStar() throws Exception { - assertThat(cache.glob(list("**"), list("first.txt"), false)).containsExactly( + assertThat(cache.globUnsorted(list("**"), list("first.txt"), false)).containsExactly( "BUILD", "bar", "bar/first.js", "bar/second.js", "first.js", "foo", "foo/first.js", - "foo/second.js", "second.js", "second.txt").inOrder(); + "foo/second.js", "second.js", "second.txt"); } @Test public void testExcludeAll_Star() throws Exception { - assertThat(cache.glob(list("*"), list("*"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("*"), list("*"), false)).isEmpty(); } @Test public void testExcludeAll_Star_NoMatchesAnyway() throws Exception { - assertThat(cache.glob(list("nope"), list("*"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("nope"), list("*"), false)).isEmpty(); } @Test public void testExcludeAll_StarStar() throws Exception { - assertThat(cache.glob(list("**"), list("**"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("**"), list("**"), false)).isEmpty(); } @Test public void testExcludeAll_Manual() throws Exception { - assertThat(cache.glob(list("**"), list("*", "*/*", "*/*/*"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("**"), list("*", "*/*", "*/*/*"), false)).isEmpty(); } @Test public void testSingleFileExcludeDoesntMatch() throws Exception { - assertThat(cache.glob(list("first.txt"), list("nope.txt"), false)).containsExactly("first.txt"); + assertThat(cache.globUnsorted(list("first.txt"), list("nope.txt"), false)).containsExactly( + "first.txt"); } @Test public void testExcludeDirectory() throws Exception { - assertThat(cache.glob(list("foo/*"), NONE, true)).containsExactly( + assertThat(cache.globUnsorted(list("foo/*"), NONE, true)).containsExactly( "foo/first.js", "foo/second.js"); - assertThat(cache.glob(list("foo/*"), list("foo"), false)).containsExactly( + assertThat(cache.globUnsorted(list("foo/*"), list("foo"), false)).containsExactly( "foo/first.js", "foo/second.js"); } @Test public void testChildGlobWithChildExclude() throws Exception { - assertThat(cache.glob(list("foo/*"), list("foo/*"), false)).isEmpty(); - assertThat(cache.glob(list("foo/first.js", "foo/second.js"), list("foo/*"), false)).isEmpty(); - assertThat(cache.glob(list("foo/first.js"), list("foo/first.js"), false)).isEmpty(); - assertThat(cache.glob(list("foo/first.js"), list("*/first.js"), false)).isEmpty(); - assertThat(cache.glob(list("foo/first.js"), list("*/*"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("foo/*"), list("foo/*"), false)).isEmpty(); + assertThat( + cache.globUnsorted(list("foo/first.js", "foo/second.js"), list("foo/*"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("foo/first.js"), list("foo/first.js"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("foo/first.js"), list("*/first.js"), false)).isEmpty(); + assertThat(cache.globUnsorted(list("foo/first.js"), list("*/*"), false)).isEmpty(); } private void assertEmpty(Collection<?> glob) { diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java index b3740c8f8c..e53690e20c 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java @@ -121,7 +121,7 @@ public class PackageFactoryApparatus { getPackageLocator(), null, TestUtils.getPool()); - LegacyGlobber globber = new LegacyGlobber(globCache); + LegacyGlobber globber = PackageFactory.createLegacyGlobber(globCache); Package externalPkg = factory.newExternalPackageBuilder( buildFile.getParentDirectory().getRelative("WORKSPACE"), "TESTING") diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java index 872c4e2bfb..5d8c1dac17 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java @@ -114,7 +114,7 @@ public abstract class PackageFactoryTestBase { PackageFactoryApparatus.createEmptyLocator(), null, TestUtils.getPool()); - assertThat(globCache.glob(include, exclude, false)).containsExactlyElementsIn(expected); + assertThat(globCache.globUnsorted(include, exclude, false)).containsExactlyElementsIn(expected); } @Before 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 e243a82e45..2aec5947e9 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 @@ -434,6 +434,24 @@ public class PackageFunctionTest extends BuildViewTestCase { Label.parseAbsoluteUnchecked("//foo:a.config"), Label.parseAbsoluteUnchecked("//foo:b.txt")) .inOrder(); + getSkyframeExecutor().resetEvaluator(); + getSkyframeExecutor().preparePackageLoading( + new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)), + ConstantRuleVisibility.PUBLIC, true, 7, "", + UUID.randomUUID(), tsgm); + 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 diff --git a/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java b/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java index e8c08f00d5..34cfcc2db2 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java @@ -21,7 +21,6 @@ import static org.junit.Assert.fail; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; -import com.google.common.collect.Ordering; import com.google.common.util.concurrent.Uninterruptibles; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; @@ -277,20 +276,6 @@ public class GlobTest { new UnixGlob.Builder(tmpPath).addPatterns(patterns).globInterruptible()); } - /** - * Tests that a glob returns files in sorted order. - */ - @Test - public void testGlobEntriesAreSorted() throws Exception { - Collection<Path> directoryEntries = tmpPath.getDirectoryEntries(); - List<Path> globResult = new UnixGlob.Builder(tmpPath) - .addPattern("*") - .setExcludeDirectories(false) - .globInterruptible(); - assertThat(Ordering.natural().sortedCopy(directoryEntries)).containsExactlyElementsIn( - globResult).inOrder(); - } - private void assertIllegalPattern(String pattern) throws Exception { try { new UnixGlob.Builder(tmpPath) diff --git a/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java b/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java index 5e0f7875fe..4b3b697150 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java @@ -18,7 +18,6 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; -import com.google.common.collect.Ordering; import com.google.devtools.build.lib.util.BlazeClock; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; @@ -29,7 +28,6 @@ import org.junit.runners.JUnit4; import java.util.Collection; import java.util.HashSet; -import java.util.List; import java.util.Set; /** @@ -140,20 +138,6 @@ public class RecursiveGlobTest { return expectedFiles; } - /** - * Tests that a recursive glob returns files in sorted order. - */ - @Test - public void testGlobEntriesAreSorted() throws Exception { - List<Path> globResult = new UnixGlob.Builder(tmpPath) - .addPattern("**") - .setExcludeDirectories(false) - .globInterruptible(); - - assertThat(Ordering.natural().sortedCopy(globResult)).containsExactlyElementsIn(globResult) - .inOrder(); - } - @Test public void testRecursiveGlobsAreOptimized() throws Exception { long numGlobTasks = new UnixGlob.Builder(tmpPath) |