Fix SkyframeLoadingPhaseRunner posting of EventBus events.
The TargetParsingCompleteEvent was posting the post-expansion targets, and the
LoadingPhaseCompleteEvent was missing the test-suite targets.
--
MOS_MIGRATED_REVID=114312273
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 9b6b684..7fda021 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -281,7 +281,8 @@
private void postLoadingLogging(EventBus eventBus, ImmutableSet<Target> originalTargetsToLoad,
ImmutableSet<Target> expandedTargetsToLoad, Stopwatch timer) {
Set<Target> testSuiteTargets = Sets.difference(originalTargetsToLoad, expandedTargetsToLoad);
- eventBus.post(new LoadingPhaseCompleteEvent(expandedTargetsToLoad, testSuiteTargets,
+ eventBus.post(new LoadingPhaseCompleteEvent(
+ expandedTargetsToLoad, ImmutableSet.copyOf(testSuiteTargets),
packageManager.getStatistics(), timer.stop().elapsed(TimeUnit.MILLISECONDS)));
LOG.info("Loading phase finished");
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
index 914ce3b..4f42c0f 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseCompleteEvent.java
@@ -14,18 +14,18 @@
package com.google.devtools.build.lib.pkgcache;
import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Target;
-import java.util.Collection;
-
/**
* This event is fired after the loading phase is complete.
*/
-public class LoadingPhaseCompleteEvent {
- private final Collection<Target> targets;
- private final Collection<Target> filteredTargets;
+public final class LoadingPhaseCompleteEvent {
+ private final ImmutableSet<Target> targets;
+ private final ImmutableSet<Target> filteredTargets;
private final PackageManager.PackageManagerStatistics pkgManagerStats;
private final long timeInMs;
@@ -35,11 +35,12 @@
* @param targets the set of active targets that remain
* @param pkgManagerStats statistics about the package cache
*/
- public LoadingPhaseCompleteEvent(Collection<Target> targets, Collection<Target> filteredTargets,
- PackageManager.PackageManagerStatistics pkgManagerStats, long timeInMs) {
- this.targets = targets;
- this.filteredTargets = filteredTargets;
- this.pkgManagerStats = pkgManagerStats;
+ public LoadingPhaseCompleteEvent(ImmutableSet<Target> targets,
+ ImmutableSet<Target> filteredTargets, PackageManager.PackageManagerStatistics pkgManagerStats,
+ long timeInMs) {
+ this.targets = Preconditions.checkNotNull(targets);
+ this.filteredTargets = Preconditions.checkNotNull(filteredTargets);
+ this.pkgManagerStats = Preconditions.checkNotNull(pkgManagerStats);
this.timeInMs = timeInMs;
}
@@ -47,14 +48,14 @@
* @return The set of active targets remaining, which is a subset of the
* targets we attempted to load.
*/
- public Collection<Target> getTargets() {
+ public ImmutableSet<Target> getTargets() {
return targets;
}
/**
* @return The set of filtered targets.
*/
- public Collection<Target> getFilteredTargets() {
+ public ImmutableSet<Target> getFilteredTargets() {
return filteredTargets;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 12c38a8..92c6804 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1728,7 +1728,6 @@
* Skyframe-based implementation of {@link LoadingPhaseRunner} based on {@link
* TargetPatternPhaseFunction}.
*/
- // TODO(ulfjack): This is still incomplete.
final class SkyframeLoadingPhaseRunner extends LoadingPhaseRunner {
private final TargetPatternEvaluator targetPatternEvaluator;
private final Set<String> ruleClassNames;
@@ -1785,17 +1784,14 @@
long time = timer.stop().elapsed(TimeUnit.MILLISECONDS);
TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
- // TODO(ulfjack): The first event should be pre-test_suite expansion, the second post.
- eventBus.post(new TargetParsingCompleteEvent(patternParsingValue.getTargets(),
+ eventBus.post(new TargetParsingCompleteEvent(patternParsingValue.getOriginalTargets(),
patternParsingValue.getFilteredTargets(), patternParsingValue.getTestFilteredTargets(),
time));
if (callback != null) {
callback.notifyTargets(patternParsingValue.getTargets());
}
eventBus.post(new LoadingPhaseCompleteEvent(
- /*was expandedTargetsToLoad*/patternParsingValue.getTargets(),
- // TODO(ulfjack): Should be: Sets.difference(originalTargetsToLoad, expandedTargetsToLoad)
- /*was testSuiteTargets*/ImmutableSet.<Target>of(),
+ patternParsingValue.getTargets(), patternParsingValue.getTestSuiteTargets(),
packageManager.getStatistics(), /*timeInMs=*/0));
return patternParsingValue.toLoadingResult();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index ea30c05..9608878 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -159,9 +159,12 @@
expandedTargetsBuilder.add(target);
}
}
- targets = expandedTargetsBuilder.build();
- return new TargetPatternPhaseValue(targets.getTargets(), testsToRun, preExpansionError,
- targets.hasError(), filteredTargets, testFilteredTargets);
+ ResolvedTargets<Target> expandedTargets = expandedTargetsBuilder.build();
+ Set<Target> testSuiteTargets =
+ Sets.difference(targets.getTargets(), expandedTargets.getTargets());
+ return new TargetPatternPhaseValue(expandedTargets.getTargets(), testsToRun, preExpansionError,
+ expandedTargets.hasError(), filteredTargets, testFilteredTargets,
+ targets.getTargets(), ImmutableSet.copyOf(testSuiteTargets));
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
index becc737..4c864db 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
@@ -52,15 +52,23 @@
private final ImmutableSet<Target> filteredTargets;
private final ImmutableSet<Target> testFilteredTargets;
+ // These two fields are only for the purposes of generating the TargetParsingCompleteEvent.
+ // TODO(ulfjack): Support EventBus event posting in Skyframe, and remove this code again.
+ private final ImmutableSet<Target> originalTargets;
+ private final ImmutableSet<Target> testSuiteTargets;
+
TargetPatternPhaseValue(ImmutableSet<Target> targets, @Nullable ImmutableSet<Target> testsToRun,
boolean hasError, boolean hasPostExpansionError, ImmutableSet<Target> filteredTargets,
- ImmutableSet<Target> testFilteredTargets) {
+ ImmutableSet<Target> testFilteredTargets, ImmutableSet<Target> originalTargets,
+ ImmutableSet<Target> testSuiteTargets) {
this.targets = Preconditions.checkNotNull(targets);
this.testsToRun = testsToRun;
this.hasError = hasError;
this.hasPostExpansionError = hasPostExpansionError;
this.filteredTargets = Preconditions.checkNotNull(filteredTargets);
this.testFilteredTargets = Preconditions.checkNotNull(testFilteredTargets);
+ this.originalTargets = Preconditions.checkNotNull(originalTargets);
+ this.testSuiteTargets = Preconditions.checkNotNull(testSuiteTargets);
}
public ImmutableSet<Target> getTargets() {
@@ -88,6 +96,14 @@
return testFilteredTargets;
}
+ public ImmutableSet<Target> getOriginalTargets() {
+ return originalTargets;
+ }
+
+ public ImmutableSet<Target> getTestSuiteTargets() {
+ return testSuiteTargets;
+ }
+
public LoadingResult toLoadingResult() {
return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun(),
ImmutableMap.<PackageIdentifier, Path>of());
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 1e0b234..65c10cb 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -27,6 +27,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
@@ -230,8 +231,8 @@
tester.addFile("my_test/BUILD",
"sh_test(name = 'my_test', srcs = ['test.cc'])");
assertNoErrors(tester.loadTests("-//my_test"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
}
@Test
@@ -239,8 +240,8 @@
tester.addFile("my_library/BUILD",
"cc_library(name = 'my_library', srcs = ['test.cc'])");
assertNoErrors(tester.loadTests("-//my_library"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
}
private void writeBuildFilesForTestFiltering() throws Exception {
@@ -258,8 +259,8 @@
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
assertThat(loadingResult.getTestsToRun())
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
}
@Test
@@ -271,8 +272,8 @@
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
assertThat(loadingResult.getTestsToRun())
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
}
@Test
@@ -283,8 +284,8 @@
assertThat(loadingResult.getTargets())
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t2"));
assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets("//tests:t1"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets());
}
@Test
@@ -294,8 +295,8 @@
LoadingResult loadingResult = assertNoErrors(tester.loadTests("//tests:all"));
assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//tests:t1"));
assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets("//tests:t1"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//tests:t2"));
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets("//tests:t2"));
}
@Test
@@ -307,8 +308,8 @@
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t3"));
assertThat(loadingResult.getTestsToRun())
.containsExactlyElementsIn(getTargets("//tests:t1", "//tests:t3"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//tests:t2"));
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets()).containsExactlyElementsIn(getTargets("//tests:t2"));
}
@Test
@@ -324,6 +325,10 @@
assertThat(loadingResult.getPackageRoots().entrySet())
.contains(entryFor(PackageIdentifier.createInDefaultRepo("cc"), tester.getWorkspace()));
}
+ assertThat(tester.getOriginalTargets())
+ .containsExactlyElementsIn(getTargets("//cc:tests", "//cc:my_test"));
+ assertThat(tester.getTestSuiteTargets())
+ .containsExactlyElementsIn(getTargets("//cc:tests"));
}
@Test
@@ -384,8 +389,9 @@
.containsExactlyElementsIn(getTargets("//foo:foo", "//foo:baz"));
assertThat(loadingResult.getTestsToRun())
.containsExactlyElementsIn(getTargets("//foo:foo", "//foo:baz"));
- assertThat(tester.filteredTargets).containsExactlyElementsIn(getTargets());
- assertThat(tester.testFilteredTargets).containsExactlyElementsIn(getTargets("//foo:foo_suite"));
+ assertThat(tester.getFilteredTargets()).containsExactlyElementsIn(getTargets());
+ assertThat(tester.getTestFilteredTargets())
+ .containsExactlyElementsIn(getTargets("//foo:foo_suite"));
}
/** Regression test for bug: "subtracting tests from test doesn't work" */
@@ -630,8 +636,8 @@
private final StoredEventHandler storedErrors;
private LoadingCallback loadingCallback;
- private Set<Target> filteredTargets;
- private Set<Target> testFilteredTargets;
+ private TargetParsingCompleteEvent targetParsingCompleteEvent;
+ private LoadingPhaseCompleteEvent loadingPhaseCompleteEvent;
private MockToolsConfig mockToolsConfig;
@@ -709,8 +715,8 @@
result = loadingPhaseRunner.execute(storedErrors, eventBus,
ImmutableList.copyOf(patterns), options, ImmutableListMultimap.<String, Label>of(),
keepGoing, /*enableLoading=*/true, determineTests, loadingCallback);
- this.filteredTargets = listener.filteredTargets;
- this.testFilteredTargets = listener.testFilteredTargets;
+ this.targetParsingCompleteEvent = listener.targetParsingCompleteEvent;
+ this.loadingPhaseCompleteEvent = listener.loadingPhaseCompleteEvent;
} catch (LoadingFailedException e) {
System.err.println(storedErrors.getEvents());
throw e;
@@ -780,6 +786,22 @@
return skyframeExecutor.getPackageManager();
}
+ public ImmutableSet<Target> getFilteredTargets() {
+ return targetParsingCompleteEvent.getFilteredTargets();
+ }
+
+ public ImmutableSet<Target> getTestFilteredTargets() {
+ return targetParsingCompleteEvent.getTestFilteredTargets();
+ }
+
+ public ImmutableSet<Target> getOriginalTargets() {
+ return targetParsingCompleteEvent.getTargets();
+ }
+
+ public ImmutableSet<Target> getTestSuiteTargets() {
+ return loadingPhaseCompleteEvent.getFilteredTargets();
+ }
+
private Iterable<Event> filteredEvents() {
return Iterables.filter(storedErrors.getEvents(), new Predicate<Event>() {
@Override
@@ -808,12 +830,17 @@
}
public static class FilteredTargetListener {
- private Set<Target> filteredTargets;
- private Set<Target> testFilteredTargets;
+ private TargetParsingCompleteEvent targetParsingCompleteEvent;
+ private LoadingPhaseCompleteEvent loadingPhaseCompleteEvent;
+
@Subscribe
- public void notifyFilteredTargets(TargetParsingCompleteEvent event) {
- filteredTargets = event.getFilteredTargets();
- testFilteredTargets = event.getTestFilteredTargets();
+ public void targetParsingComplete(TargetParsingCompleteEvent event) {
+ this.targetParsingCompleteEvent = event;
+ }
+
+ @Subscribe
+ public void loadingPhaseComplete(LoadingPhaseCompleteEvent event) {
+ this.loadingPhaseCompleteEvent = event;
}
}
}
