From 6243ecb9bf45e677a98832d76ce48e3114f2134f Mon Sep 17 00:00:00 2001 From: Nathan Harmata Date: Wed, 15 Feb 2017 23:52:57 +0000 Subject: Fix a hypothetical issue with 'buildfiles' and 'loadfiles' with duplicate targets. These two functions don't use a Uniquifier internally so if the same bzl file foo.bzl is loaded multiple ways in 'e' in 'loadfiles(e)' then 'f' in 'f(loadfiles(e))' will observe foo.bzl multiple times. This isn't observable in practice (i.e. I cannot write a black-box query test that would fail without this CL) because: (1) BlazeQueryEnvironment#eval(QueryExpression, VariableContext, Callback) uses an intermediate aggregating callback and thus doesn't use the streaming query evaluation model. This means that the internal deduping in BlazeQueryEnvironment#getBuildFiles is sufficient. (2) SkyQueryEnvironment uses an outer BatchStreamedCallback which internally uses a Uniquifier. Still, this CL is useful for SkyQuery because without we're doing wasted work (e.g. in my example above, 'f' will wastefully do duplicate work with the .bzl files from the inner loadfiles(e)) because the deduping happens on the final query result, not on the intermediate result from 'buildfiles'/'loadfiles'. For 'buildfiles', there's an additional wart with (1) above because FakeSubincludeTarget doesn't override Object#equals/hashCode. -- PiperOrigin-RevId: 147656183 MOS_MIGRATED_REVID=147656183 --- .../build/lib/query2/FakeSubincludeTarget.java | 21 ++++++++++++++++++++- .../build/lib/query2/engine/BuildFilesFunction.java | 5 +++-- .../build/lib/query2/engine/LoadFilesFunction.java | 5 +++-- .../lib/skyframe/SequencedSkyframeExecutor.java | 5 +++-- 4 files changed, 29 insertions(+), 7 deletions(-) (limited to 'src/main/java/com/google/devtools/build') diff --git a/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java b/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java index 308603bf90..224ada6c01 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java +++ b/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java @@ -23,7 +23,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.util.Preconditions; - +import java.util.Objects; import java.util.Set; /** @@ -88,4 +88,23 @@ public class FakeSubincludeTarget implements Target { public boolean isConfigurable() { return true; } + + @Override + public String toString() { + return label.toString(); + } + + @Override + public int hashCode() { + return Objects.hash(label, pkg); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof FakeSubincludeTarget)) { + return false; + } + FakeSubincludeTarget other = (FakeSubincludeTarget) obj; + return label.equals(other.label) && pkg.equals(other.pkg); + } } diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java index 0e65cbdf05..d2a2eb0fa8 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java @@ -49,6 +49,7 @@ class BuildFilesFunction implements QueryFunction { List args, final Callback callback) throws QueryException, InterruptedException { + final Uniquifier uniquifier = env.createUniquifier(); env.eval( args.get(0).getExpression(), context, @@ -58,9 +59,9 @@ class BuildFilesFunction implements QueryFunction { throws QueryException, InterruptedException { Set result = CompactHashSet.create(); Iterables.addAll(result, partialResult); - callback.process( + callback.process(uniquifier.unique( env.getBuildFiles( - expression, result, /* BUILD */ true, /* subinclude */ true, /* load */ true)); + expression, result, /* BUILD */ true, /* subinclude */ true, /* load */ true))); } }); } diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java index 6b42ba4f37..311a6afff5 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java @@ -45,6 +45,7 @@ class LoadFilesFunction implements QueryEnvironment.QueryFunction { List args, final Callback callback) throws QueryException, InterruptedException { + final Uniquifier uniquifier = env.createUniquifier(); env.eval( args.get(0).getExpression(), context, @@ -54,13 +55,13 @@ class LoadFilesFunction implements QueryEnvironment.QueryFunction { throws QueryException, InterruptedException { Set result = CompactHashSet.create(); Iterables.addAll(result, partialResult); - callback.process( + callback.process(uniquifier.unique( env.getBuildFiles( expression, result, /* BUILD */ false, /* subinclude */ false, - /* load */ true)); + /* load */ true))); } }); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index fbcc10f0c8..34cde7627a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -216,7 +216,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { ImmutableList buildInfoFactories, Iterable diffAwarenessFactories, PathFragment blacklistedPackagePrefixesFile, - String productName) { + String productName, + Preprocessor.Factory.Supplier preprocessorFactorySupplier) { return create( pkgFactory, directories, @@ -225,7 +226,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { buildInfoFactories, diffAwarenessFactories, Predicates.alwaysFalse(), - Preprocessor.Factory.Supplier.NullSupplier.INSTANCE, + preprocessorFactorySupplier, ImmutableMap.of(), ImmutableList.of(), ImmutableList.of(), -- cgit v1.2.3