Move ownership of SkyframeBuildView to SkyframeExecutor.

Also move ownership of ArtifactFactory to SkyframeBuildView; simplify the
code.

--
MOS_MIGRATED_REVID=103722228

8 files changed, 50 insertions, 82 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 92290ac959..eb8ec0fde4 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
@@ -220,8 +220,6 @@ public class BuildView {
 
   private final ConfiguredRuleClassProvider ruleClassProvider;
 
-  private final ArtifactFactory artifactFactory;
-
   /**
    * A factory class to create the coverage report action. May be null.
    */
@@ -266,23 +264,9 @@ public class BuildView {
     this.packageManager = skyframeExecutor.getPackageManager();
     this.binTools = binTools;
     this.coverageReportActionFactory = coverageReportActionFactory;
-    this.artifactFactory = new ArtifactFactory(directories.getExecRoot());
     this.ruleClassProvider = ruleClassProvider;
     this.skyframeExecutor = Preconditions.checkNotNull(skyframeExecutor);
-    this.skyframeBuildView =
-        new SkyframeBuildView(
-            new ConfiguredTargetFactory(ruleClassProvider),
-            artifactFactory,
-            skyframeExecutor,
-            new Runnable() {
-              @Override
-              public void run() {
-                clear();
-              }
-            },
-            binTools,
-            ruleClassProvider);
-    skyframeExecutor.setSkyframeBuildView(skyframeBuildView);
+    this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView();
   }
 
   /** Returns the action graph. */
@@ -312,16 +296,8 @@ public class BuildView {
     skyframeBuildView.setTopLevelHostConfiguration(configurations.getHostConfiguration());
   }
 
-  /**
-   * Clear the graphs of ConfiguredTargets and Artifacts.
-   */
-  @VisibleForTesting
-  public void clear() {
-    artifactFactory.clear();
-  }
-
   public ArtifactFactory getArtifactFactory() {
-    return artifactFactory;
+    return skyframeBuildView.getArtifactFactory();
   }
 
   @VisibleForTesting
@@ -627,7 +603,6 @@ public class BuildView {
 
       skyframeExecutor.dropConfiguredTargets();
       skyframeCacheWasInvalidated = true;
-      clear();
     }
     skyframeAnalysisWasDiscarded = false;
     this.configurations = configurations;
@@ -741,7 +716,7 @@ public class BuildView {
       actionsWrapper = coverageReportActionFactory.createCoverageReportActionsWrapper(
           allTargetsToTest,
           baselineCoverageArtifacts,
-          artifactFactory,
+          getArtifactFactory(),
           CoverageReportValue.ARTIFACT_OWNER);
       if (actionsWrapper != null) {
         ImmutableList <Action> actions = actionsWrapper.getActions();
@@ -952,7 +927,7 @@ public class BuildView {
       realPackageRoots.put(entry.getKey(), root);
     }
     // Source Artifact roots:
-    artifactFactory.setPackageRoots(realPackageRoots);
+    getArtifactFactory().setPackageRoots(realPackageRoots);
 
     // Derived Artifact roots:
     ImmutableList.Builder<Root> roots = ImmutableList.builder();
@@ -965,7 +940,7 @@ public class BuildView {
       roots.addAll(cfg.getRoots());
     }
 
-    artifactFactory.setDerivedArtifactRoots(roots.build());
+    getArtifactFactory().setDerivedArtifactRoots(roots.build());
   }
 
   /**
@@ -995,7 +970,7 @@ public class BuildView {
       BuildConfigurationCollection configurations) throws InterruptedException {
     BuildConfiguration config = target.getConfiguration();
     CachingAnalysisEnvironment analysisEnvironment =
-        new CachingAnalysisEnvironment(artifactFactory,
+        new CachingAnalysisEnvironment(getArtifactFactory(),
             new ConfiguredTargetKey(target.getLabel(), config),
             /*isSystemEnv=*/false, config.extendedSanityChecks(), eventHandler,
             /*skyframeEnv=*/null, config.isActionsEnabled(), binTools);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
index bef594b828..4c885a3663 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
@@ -103,7 +103,7 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment {
     this.errorEventListener = errorEventListener;
     this.skyframeEnv = env;
     this.allowRegisteringActions = allowRegisteringActions;
-    this.binTools = binTools;
+    this.binTools = Preconditions.checkNotNull(binTools);
     middlemanFactory = new MiddlemanFactory(artifactFactory, this);
     artifacts = new HashMap<>();
   }
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 368812f671..6584af68ec 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
@@ -621,7 +621,6 @@ public final class BlazeRuntime {
   /** Removes skyframe cache and other caches that must be kept synchronized with skyframe. */
   private void clearSkyframeRelevantCaches() {
     skyframeExecutor.resetEvaluator();
-    view.clear();
   }
 
   /**
@@ -1598,6 +1597,7 @@ public final class BlazeRuntime {
               pkgFactory,
               timestampMonitor,
               directories,
+              binTools,
               workspaceStatusActionFactory,
               ruleClassProvider.getBuildInfoFactories(),
               immutableDirectories,
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 6461e3f085..9f677143aa 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
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
+import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.Package;
@@ -100,6 +101,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       PackageFactory pkgFactory,
       TimestampGranularityMonitor tsgm,
       BlazeDirectories directories,
+      BinTools binTools,
       Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,
@@ -115,6 +117,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         pkgFactory,
         tsgm,
         directories,
+        binTools,
         workspaceStatusActionFactory,
         buildInfoFactories,
         immutableDirectories,
@@ -132,6 +135,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       PackageFactory pkgFactory,
       TimestampGranularityMonitor tsgm,
       BlazeDirectories directories,
+      BinTools binTools,
       Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,
@@ -148,6 +152,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
             pkgFactory,
             tsgm,
             directories,
+            binTools,
             workspaceStatusActionFactory,
             buildInfoFactories,
             immutableDirectories,
@@ -163,7 +168,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
 
   @VisibleForTesting
   public static SequencedSkyframeExecutor create(Reporter reporter, PackageFactory pkgFactory,
-      TimestampGranularityMonitor tsgm, BlazeDirectories directories,
+      TimestampGranularityMonitor tsgm, BlazeDirectories directories, BinTools binTools,
       WorkspaceStatusAction.Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,
@@ -173,6 +178,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         pkgFactory,
         tsgm,
         directories,
+        binTools,
         workspaceStatusActionFactory,
         buildInfoFactories,
         immutableDirectories,
@@ -498,9 +504,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
 
   @Override
   public void dropConfiguredTargets() {
-    if (skyframeBuildView != null) {
-      skyframeBuildView.clearInvalidatedConfiguredTargets();
-    }
+    skyframeBuildView.clearInvalidatedConfiguredTargets();
+    skyframeBuildView.clearLegacyData();
     memoizingEvaluator.delete(
         // We delete any value that can hold an action -- all subclasses of ActionLookupValue -- as
         // well as ActionExecutionValues, since they do not depend on ActionLookupValues.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
index b39707f3f4..df68c74191 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
+import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
@@ -41,6 +42,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory
       PackageFactory pkgFactory,
       TimestampGranularityMonitor tsgm,
       BlazeDirectories directories,
+      BinTools binTools,
       Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,
@@ -55,6 +57,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory
         pkgFactory,
         tsgm,
         directories,
+        binTools,
         workspaceStatusActionFactory,
         buildInfoFactories,
         immutableDirectories,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 718fd5037b..0c6d78e800 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -34,8 +34,10 @@ import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictEx
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
 import com.google.devtools.build.lib.analysis.Aspect;
+import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
+import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
@@ -88,7 +90,6 @@ public final class SkyframeBuildView {
   private final ConfiguredTargetFactory factory;
   private final ArtifactFactory artifactFactory;
   private final SkyframeExecutor skyframeExecutor;
-  private final Runnable legacyDataCleaner;
   private final BinTools binTools;
   private boolean enableAnalysis = false;
 
@@ -115,16 +116,14 @@ public final class SkyframeBuildView {
   private Map<Set<Class<? extends BuildConfiguration.Fragment>>, BuildConfiguration>
       hostConfigurationCache = Maps.newConcurrentMap();
 
-  public SkyframeBuildView(ConfiguredTargetFactory factory, ArtifactFactory artifactFactory,
-      SkyframeExecutor skyframeExecutor, Runnable legacyDataCleaner,  BinTools binTools,
-      RuleClassProvider ruleClassProvider) {
-    this.factory = factory;
-    this.artifactFactory = artifactFactory;
+  public SkyframeBuildView(BlazeDirectories directories,
+      SkyframeExecutor skyframeExecutor, BinTools binTools,
+      ConfiguredRuleClassProvider ruleClassProvider) {
+    this.factory = new ConfiguredTargetFactory(ruleClassProvider);
+    this.artifactFactory = new ArtifactFactory(directories.getExecRoot());
     this.skyframeExecutor = skyframeExecutor;
-    this.legacyDataCleaner = legacyDataCleaner;
     this.binTools = binTools;
     this.ruleClassProvider = ruleClassProvider;
-    skyframeExecutor.setArtifactFactoryAndBinTools(artifactFactory, binTools);
   }
 
   public void resetEvaluatedConfiguredTargetKeysSet() {
@@ -375,7 +374,7 @@ public final class SkyframeBuildView {
     }
   }
 
-  ArtifactFactory getArtifactFactory() {
+  public ArtifactFactory getArtifactFactory() {
     return artifactFactory;
   }
 
@@ -480,12 +479,12 @@ public final class SkyframeBuildView {
   }
 
   /**
-   * Workaround to clear all legacy data, like the action graph and the artifact factory. We need
+   * Workaround to clear all legacy data, like the artifact factory. We need
    * to clear them to avoid conflicts.
    * TODO(bazel-team): Remove this workaround. [skyframe-execution]
    */
   void clearLegacyData() {
-    legacyDataCleaner.run();
+    artifactFactory.clear();
   }
 
   /**
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 443a444452..bc624d1837 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
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.analysis.Aspect;
 import com.google.devtools.build.lib.analysis.AspectWithParameters;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BuildView.Options;
+import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.DependencyResolver.Dependency;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget;
@@ -229,7 +230,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   private final Set<Path> immutableDirectories;
 
-  private BinTools binTools = null;
+  private final BinTools binTools;
   private boolean needToInjectEmbeddedArtifacts = true;
   private boolean needToInjectPrecomputedValuesForAnalysis = true;
   protected int modifiedFiles;
@@ -256,6 +257,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       PackageFactory pkgFactory,
       TimestampGranularityMonitor tsgm,
       BlazeDirectories directories,
+      BinTools binTools,
       Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,
@@ -288,6 +290,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     this.extraSkyFunctions = extraSkyFunctions;
     this.extraPrecomputedValues = extraPrecomputedValues;
     this.errorOnExternalFiles = errorOnExternalFiles;
+    this.binTools = binTools;
+
+    this.skyframeBuildView = new SkyframeBuildView(
+        directories,
+        this,
+        binTools,
+        (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider());
+    this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
   }
 
   private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions(
@@ -479,9 +489,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   public void resetEvaluator() {
     init();
     emittedEventState.clear();
-    if (skyframeBuildView != null) {
-      skyframeBuildView.clearLegacyData();
-    }
+    skyframeBuildView.clearLegacyData();
     reinjectConstantValuesLazily();
   }
 
@@ -890,13 +898,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     }
   }
 
-  /**
-   * Specifies the current {@link SkyframeBuildView} instance. This should only be set once over the
-   * lifetime of the Blaze server, except in tests.
-   */
-  public void setSkyframeBuildView(SkyframeBuildView skyframeBuildView) {
-    this.skyframeBuildView = skyframeBuildView;
-    this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
+  public SkyframeBuildView getSkyframeBuildView() {
+    return skyframeBuildView;
   }
 
   /**
@@ -1062,11 +1065,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   public ImmutableMap<Dependency, ConfiguredTarget> getConfiguredTargetMap(
       BuildConfiguration originalConfig, Iterable<Dependency> keys, boolean useOriginalConfig) {
     checkActive();
-    if (skyframeBuildView == null) {
-      // If build view has not yet been initialized, no configured targets can have been created.
-      // This is most likely to happen after a failed loading phase.
-      return ImmutableMap.of();
-    }
 
     Map<Dependency, BuildConfiguration> configs;
     if (originalConfig != null) {
@@ -1620,15 +1618,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     this.statusReporterRef.set(statusReporter);
   }
 
-  /**
-   * This should be called at most once in the lifetime of the SkyframeExecutor (except for
-   * tests), and it should be called before the execution phase.
-   */
-  void setArtifactFactoryAndBinTools(ArtifactFactory artifactFactory, BinTools binTools) {
-    this.artifactFactory.set(artifactFactory);
-    this.binTools = binTools;
-  }
-
   public void prepareExecution(boolean checkOutputFiles,
       @Nullable Range<Long> lastExecutionTimeRange) throws AbruptExitException,
       InterruptedException {
@@ -1704,9 +1693,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       if (ignoreInvalidations) {
         return;
       }
-      if (skyframeBuildView != null) {
-        skyframeBuildView.getInvalidationReceiver().invalidated(skyKey, state);
-      }
+      skyframeBuildView.getInvalidationReceiver().invalidated(skyKey, state);
     }
 
     @Override
@@ -1714,9 +1701,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       if (ignoreInvalidations) {
         return;
       }
-      if (skyframeBuildView != null) {
-        skyframeBuildView.getInvalidationReceiver().enqueueing(skyKey);
-      }
+      skyframeBuildView.getInvalidationReceiver().enqueueing(skyKey);
       if (executionProgressReceiver != null) {
         executionProgressReceiver.enqueueing(skyKey);
       }
@@ -1730,9 +1715,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       if (ignoreInvalidations) {
         return;
       }
-      if (skyframeBuildView != null) {
-        skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, valueSupplier, state);
-      }
+      skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, valueSupplier, state);
       if (executionProgressReceiver != null) {
         executionProgressReceiver.evaluated(skyKey, valueSupplier, state);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java
index 7710fb0b67..f5e3ae5d64 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
+import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
@@ -43,6 +44,7 @@ public interface SkyframeExecutorFactory {
    * @param pkgFactory the package factory
    * @param tsgm timestamp granularity monitor
    * @param directories Blaze directories
+   * @param binTools the embedded tools
    * @param workspaceStatusActionFactory a factory for creating WorkspaceStatusAction objects
    * @param buildInfoFactories list of BuildInfoFactories
    * @param diffAwarenessFactories
@@ -59,6 +61,7 @@ public interface SkyframeExecutorFactory {
       PackageFactory pkgFactory,
       TimestampGranularityMonitor tsgm,
       BlazeDirectories directories,
+      BinTools binTools,
       Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       Set<Path> immutableDirectories,