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

8 files changed, 38 insertions, 23 deletions

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 65c4a05667..c39ab0f9fe 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 @@ public class BuildView {
   private final SkyframeExecutor skyframeExecutor;
   private final SkyframeBuildView skyframeBuildView;
 
+  // Same as skyframeExecutor.getPackageManager().
   private final PackageManager packageManager;
 
   private final BinTools binTools;
@@ -230,13 +231,13 @@ public class BuildView {
    * 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 @@ public class BuildView {
     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 @@ public class BuildView {
         });
   }
 
-  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 @@ public class BuildView {
     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 @@ public class BuildView {
     // 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 @@ public class BuildView {
   }
 
   @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 @@ public class BuildView {
    * </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 ed165e00c1..a55a95a6c2 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 @@ package com.google.devtools.build.lib.buildtool;
 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 @@ public final class BuildResult {
   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 @@ public final class BuildResult {
     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 0f7251202d..8881cb579a 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 @@ public final class BuildTool {
 
       // 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 f27c807c82..f6d9715e17 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 @@ public class ExecutionTool {
    * @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 @@ public class ExecutionTool {
     // 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 aedbecadc0..307544f11e 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 @@ public final class BlazeRuntime {
     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 67b769cf43..65a63ec1d8 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 @@ public class RunCommand implements BlazeCommand  {
     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 6feb7bc933..ec4e2efaa9 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 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
         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 396f415230..4e4714c5b9 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 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     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();
   }