Simplify BuildView construction and store configurations in the build result.
I was persuing the idea that BuildView could become stateless. While that
should be possible, we're currently still relying on minimal state in
BuildView (from tests at least) in a way that makes it tricky to remove.
Instead, I'm now trying to move the BuildView into CommandEnvironment, and
create a new one as needed (only for build commands); that makes it safe in the
presence of concurrently running commands, as long as they don't use the same
BuildView instace. (Of course, allowing commands to run concurrently will need
more changes outside of BuildView.)
--
MOS_MIGRATED_REVID=103279370
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 65c4a05..c39ab0f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -211,6 +211,7 @@
private final SkyframeExecutor skyframeExecutor;
private final SkyframeBuildView skyframeBuildView;
+ // Same as skyframeExecutor.getPackageManager().
private final PackageManager packageManager;
private final BinTools binTools;
@@ -230,13 +231,13 @@
* Used only for testing that we clear Skyframe caches correctly.
* TODO(bazel-team): Remove this once we get rid of legacy Skyframe synchronization.
*/
- private boolean skyframeCacheWasInvalidated = false;
+ private boolean skyframeCacheWasInvalidated;
/**
* If the last build was executed with {@code Options#discard_analysis_cache} and we are not
* running Skyframe full, we should clear the legacy data since it is out-of-sync.
*/
- private boolean skyframeAnalysisWasDiscarded = false;
+ private boolean skyframeAnalysisWasDiscarded;
@VisibleForTesting
public Set<SkyKey> getSkyframeEvaluatedTargetKeysForTesting() {
@@ -257,12 +258,12 @@
return skyframeCacheWasInvalidated;
}
- public BuildView(BlazeDirectories directories, PackageManager packageManager,
+ public BuildView(BlazeDirectories directories,
ConfiguredRuleClassProvider ruleClassProvider,
SkyframeExecutor skyframeExecutor,
BinTools binTools, CoverageReportActionFactory coverageReportActionFactory) {
this.directories = directories;
- this.packageManager = packageManager;
+ this.packageManager = skyframeExecutor.getPackageManager();
this.binTools = binTools;
this.coverageReportActionFactory = coverageReportActionFactory;
this.artifactFactory = new ArtifactFactory(directories.getExecRoot());
@@ -586,7 +587,8 @@
});
}
- private void prepareToBuild(PackageRootResolver resolver) throws ViewCreationFailedException {
+ private void prepareToBuild(BuildConfigurationCollection configurations,
+ PackageRootResolver resolver) throws ViewCreationFailedException {
for (BuildConfiguration config : configurations.getAllConfigurations()) {
config.prepareToBuild(directories.getExecRoot(), getArtifactFactory(), resolver);
}
@@ -633,7 +635,7 @@
skyframeBuildView.setTopLevelHostConfiguration(this.configurations.getHostConfiguration());
// Determine the configurations.
- List<TargetAndConfiguration> nodes = nodesForTargets(targets);
+ List<TargetAndConfiguration> nodes = nodesForTargets(configurations, targets);
List<ConfiguredTargetKey> targetSpecs =
Lists.transform(nodes, new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
@@ -664,16 +666,16 @@
// artifactRoots, so we need to set them before calling prepareToBuild. In that case loading
// phase has to be enabled.
if (loadingEnabled) {
- setArtifactRoots(loadingResult.getPackageRoots());
+ setArtifactRoots(loadingResult.getPackageRoots(), configurations);
}
- prepareToBuild(new SkyframePackageRootResolver(skyframeExecutor));
+ prepareToBuild(configurations, new SkyframePackageRootResolver(skyframeExecutor));
skyframeExecutor.injectWorkspaceStatusData();
SkyframeAnalysisResult skyframeAnalysisResult;
try {
skyframeAnalysisResult =
skyframeBuildView.configureTargets(
targetSpecs, aspectKeys, eventBus, viewOptions.keepGoing);
- setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
+ setArtifactRoots(skyframeAnalysisResult.getPackageRoots(), configurations);
} finally {
skyframeBuildView.clearInvalidatedConfiguredTargets();
}
@@ -854,7 +856,8 @@
}
@VisibleForTesting
- List<TargetAndConfiguration> nodesForTargets(Collection<Target> targets) {
+ List<TargetAndConfiguration> nodesForTargets(BuildConfigurationCollection configurations,
+ Collection<Target> targets) {
// We use a hash set here to remove duplicate nodes; this can happen for input files and package
// groups.
LinkedHashSet<TargetAndConfiguration> nodes = new LinkedHashSet<>(targets.size());
@@ -937,7 +940,8 @@
* </em>
*/
@VisibleForTesting // for BuildViewTestCase
- public void setArtifactRoots(ImmutableMap<PackageIdentifier, Path> packageRoots) {
+ public void setArtifactRoots(ImmutableMap<PackageIdentifier, Path> packageRoots,
+ BuildConfigurationCollection configurations) {
Map<Path, Root> rootMap = new HashMap<>();
Map<PackageIdentifier, Root> realPackageRoots = new HashMap<>();
for (Map.Entry<PackageIdentifier, Path> entry : packageRoots.entrySet()) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
index ed165e0..a55a95a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java
@@ -17,6 +17,7 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
import com.google.devtools.build.lib.util.ExitCode;
import java.util.Collection;
@@ -35,6 +36,8 @@
private Throwable crash = null;
private boolean catastrophe = false;
private ExitCode exitCondition = ExitCode.BLAZE_INTERNAL_ERROR;
+
+ private BuildConfigurationCollection configurations;
private Collection<ConfiguredTarget> actualTargets;
private Collection<ConfiguredTarget> testTargets;
private Collection<ConfiguredTarget> successfulTargets;
@@ -118,6 +121,17 @@
return crash;
}
+ public void setBuildConfigurationCollection(BuildConfigurationCollection configurations) {
+ this.configurations = configurations;
+ }
+
+ /**
+ * Returns the build configuration collection used for the build.
+ */
+ public BuildConfigurationCollection getBuildConfigurationCollection() {
+ return configurations;
+ }
+
/**
* @see #getActualTargets
*/
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 0f72512..8881cb5 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -193,6 +193,7 @@
// Analysis phase.
AnalysisResult analysisResult = runAnalysisPhase(request, loadingResult, configurations);
+ result.setBuildConfigurationCollection(configurations);
result.setActualTargets(analysisResult.getTargetsToBuild());
result.setTestTargets(analysisResult.getTargetsToTest());
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
index f27c807..f6d9715 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -336,7 +336,6 @@
* @param buildId UUID of the build id
* @param analysisResult the analysis phase output
* @param buildResult the mutable build result
- * @param skyframeExecutor the skyframe executor (if any)
* @param packageRoots package roots collected from loading phase and BuildConfigutaionCollection
* creation
*/
@@ -356,8 +355,7 @@
// Create symlinks only after we've verified that we're actually
// supposed to build something.
if (getWorkspace().getFileSystem().supportsSymbolicLinks()) {
- List<BuildConfiguration> targetConfigurations =
- getView().getConfigurationCollection().getTargetConfigurations();
+ List<BuildConfiguration> targetConfigurations = configurations.getTargetConfigurations();
// TODO(bazel-team): This is not optimal - we retain backwards compatibility in the case where
// there's only a single configuration, but we don't create any symlinks in the multi-config
// case. Can we do better? [multi-config]
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index aedbeca..307544f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -227,8 +227,8 @@
this.blazeModules = blazeModules;
this.ruleClassProvider = ruleClassProvider;
this.configurationFactory = configurationFactory;
- this.view = new BuildView(directories, getPackageManager(), ruleClassProvider,
- skyframeExecutor, binTools, getCoverageReportActionFactory(blazeModules));
+ this.view = new BuildView(directories, ruleClassProvider, skyframeExecutor, binTools,
+ getCoverageReportActionFactory(blazeModules));
this.clock = clock;
this.timestampGranularityMonitor = Preconditions.checkNotNull(timestampGranularityMonitor);
this.startupOptionsProvider = startupOptionsProvider;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 67b769c..65a63ec 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -211,8 +211,7 @@
if (configuration == null) {
// The target may be an input file, which doesn't have a configuration. In that case, we
// choose any target configuration.
- configuration = runtime.getView().getConfigurationCollection()
- .getTargetConfigurations().get(0);
+ configuration = result.getBuildConfigurationCollection().getTargetConfigurations().get(0);
}
Path workingDir;
try {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 6feb7bc..ec4e2ef 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -182,8 +182,8 @@
3, ruleClassProvider.getDefaultsPackageContent(), UUID.randomUUID());
packageManager = skyframeExecutor.getPackageManager();
loadingPhaseRunner = new LoadingPhaseRunner(packageManager, pkgFactory.getRuleClassNames());
- buildView = new BuildView(directories, skyframeExecutor.getPackageManager(), ruleClassProvider,
- skyframeExecutor, BinTools.forUnitTesting(directories, TestConstants.EMBEDDED_TOOLS), null);
+ buildView = new BuildView(directories, ruleClassProvider, skyframeExecutor,
+ BinTools.forUnitTesting(directories, TestConstants.EMBEDDED_TOOLS), null);
useConfiguration();
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 396f415..4e4714c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -368,12 +368,11 @@
skyframeExecutor.setupDefaultPackage(defaultsPackageContent);
skyframeExecutor.dropConfiguredTargets();
- view = new BuildView(directories, getPackageManager(), ruleClassProvider, skyframeExecutor,
- binTools, null);
+ view = new BuildView(directories, ruleClassProvider, skyframeExecutor, binTools, null);
view.setConfigurationsForTesting(masterConfig);
view.setArtifactRoots(
- ImmutableMap.of(PackageIdentifier.createInDefaultRepo(""), rootDirectory));
+ ImmutableMap.of(PackageIdentifier.createInDefaultRepo(""), rootDirectory), masterConfig);
simulateLoadingPhase();
}
