Remove options class parameters from SkyframeExecutor sync methods …

…- an `OptionsProvider` is already passed in, and it's confusing that some tests have conflicting options in the raw parameter vs the provider.

Tests make use of the very useful `FakeOptions` test utility.

Also remove `syncPackageLoadingInternal` in favor of calling `super`.

PiperOrigin-RevId: 441799752

src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

@@ -30,7 +30,6 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.SingleBuildFileCache;
-import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PackageOptions;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -697,9 +696,7 @@ public void syncPackageLoading(OptionsProvider options)
  getSkyframeExecutor()
  .sync(
  reporter,
- options.getOptions(PackageOptions.class),
  packageLocator,
- options.getOptions(BuildLanguageOptions.class),
  getCommandId(),
  clientEnv,
  repoEnvFromOptions,

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -19,6 +19,8 @@
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
+import com.google.common.collect.ClassToInstanceMap;
+import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -57,7 +59,6 @@
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.WorkspaceFileValue;
 import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey;
-import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.pkgcache.PackageOptions;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -105,6 +106,8 @@
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.common.options.Options;
+import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsProvider;
 import java.io.IOException;
 import java.io.PrintStream;
@@ -260,9 +263,7 @@ public void invalidated(SkyKey skyKey, InvalidationState state) {
  @Override
  public WorkspaceInfoFromDiff sync(
  ExtendedEventHandler eventHandler,
- PackageOptions packageOptions,
  PathPackageLocator packageLocator,
- BuildLanguageOptions buildLanguageOptions,
  UUID commandId,
  Map<String, String> clientEnv,
  Map<String, String> repoEnvOption,
@@ -277,21 +278,14 @@ public WorkspaceInfoFromDiff sync(
  }
  super.sync(
  eventHandler,
- packageOptions,
  packageLocator,
- buildLanguageOptions,
  commandId,
  clientEnv,
  repoEnvOption,
  tsgm,
  options);
  long startTime = System.nanoTime();
- RepositoryOptions repoOptions = options.getOptions(RepositoryOptions.class);
- boolean checkExternalRepositoryFiles =
- repoOptions == null || repoOptions.checkExternalRepositoryFiles;
- WorkspaceInfoFromDiff workspaceInfo =
- handleDiffs(
- eventHandler, packageOptions.checkOutputFiles, checkExternalRepositoryFiles, options);
+ WorkspaceInfoFromDiff workspaceInfo = handleDiffs(eventHandler, options);
  long stopTime = System.nanoTime();
  Profiler.instance().logSimpleTask(startTime, stopTime, ProfilerTask.INFO, "handleDiffs");
  long duration = stopTime - startTime;
@@ -344,24 +338,35 @@ public void setDeletedPackages(Iterable<PackageIdentifier> pkgs) {
  @VisibleForTesting
  public void handleDiffsForTesting(ExtendedEventHandler eventHandler)
  throws InterruptedException, AbruptExitException {
- if (super.lastAnalysisDiscarded) {
+ if (lastAnalysisDiscarded) {
  // Values were cleared last build, but they couldn't be deleted because they were needed for
  // the execution phase. We can delete them now.
  dropConfiguredTargetsNow(eventHandler);
- super.lastAnalysisDiscarded = false;
+ lastAnalysisDiscarded = false;
  }
+ PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
+ packageOptions.checkOutputFiles = false;
+ ClassToInstanceMap<OptionsBase> options =
+ ImmutableClassToInstanceMap.of(PackageOptions.class, packageOptions);
  handleDiffs(
  eventHandler,
- /*checkOutputFiles=*/ false,
- /*checkExternalRepositoryFiles=*/ true,
- OptionsProvider.EMPTY);
+ new OptionsProvider() {
+ @Nullable
+ @Override
+ public <O extends OptionsBase> O getOptions(Class<O> optionsClass) {
+ return options.getInstance(optionsClass);
+ }
+
+ @Override
+ public ImmutableMap<String, Object> getStarlarkOptions() {
+ return ImmutableMap.of();
+ }
+ });
  }
 
  @Nullable
  private WorkspaceInfoFromDiff handleDiffs(
  ExtendedEventHandler eventHandler,
- boolean checkOutputFiles,
- boolean checkExternalRepositoryFiles,
  OptionsProvider options)
  throws InterruptedException, AbruptExitException {
  TimestampGranularityMonitor tsgm = this.tsgm.get();
@@ -398,17 +403,16 @@ private WorkspaceInfoFromDiff handleDiffs(
  }
  }
  BuildRequestOptions buildRequestOptions = options.getOptions(BuildRequestOptions.class);
- // TODO(bazel-team): Should use --experimental_fsvc_threads instead of the hardcoded constant
- // but plumbing the flag through is hard.
  int fsvcThreads = buildRequestOptions == null ? 200 : buildRequestOptions.fsvcThreads;
  handleDiffsWithCompleteDiffInformation(
  tsgm, modifiedFilesByPathEntry, managedDirectoriesChanged, fsvcThreads);
+ RepositoryOptions repoOptions = options.getOptions(RepositoryOptions.class);
  handleDiffsWithMissingDiffInformation(
  eventHandler,
  tsgm,
  pathEntriesWithoutDiffInformation,
- checkOutputFiles,
- checkExternalRepositoryFiles,
+ options.getOptions(PackageOptions.class).checkOutputFiles,
+ repoOptions == null || repoOptions.checkExternalRepositoryFiles,
  managedDirectoriesChanged,
  fsvcThreads);
  handleClientEnvironmentChanges();

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -2697,16 +2697,17 @@ ActionExecutionStatusReporter getActionExecutionStatusReporterForTesting() {
  /**
  * Initializes and syncs the graph with the given options, readying it for the next evaluation.
  *
+ * <p>At a minimum, {@link PackageOptions} and {@link BuildLanguageOptions} are expected to be
+ * present in the given {@link OptionsProvider}.
+ *
  * <p>Returns precomputed information about the workspace if it is available at this stage. This
  * is an optimization allowing implementations which have such information to make it available
  * early in the build.
  */
  @Nullable
  public WorkspaceInfoFromDiff sync(
  ExtendedEventHandler eventHandler,
- PackageOptions packageOptions,
  PathPackageLocator pathPackageLocator,
- BuildLanguageOptions buildLanguageOptions,
  UUID commandId,
  Map<String, String> clientEnv,
  Map<String, String> repoEnvOption,
@@ -2721,14 +2722,7 @@ public WorkspaceInfoFromDiff sync(
  AnalysisOptions analysisOptions = options.getOptions(AnalysisOptions.class);
  keepBuildConfigurationNodesWhenDiscardingAnalysis =
  analysisOptions == null || analysisOptions.keepConfigNodes;
- syncPackageLoading(
- packageOptions,
- pathPackageLocator,
- buildLanguageOptions,
- commandId,
- clientEnv,
- tsgm,
- options);
+ syncPackageLoading(pathPackageLocator, commandId, clientEnv, tsgm, options);
 
  if (lastAnalysisDiscarded) {
  dropConfiguredTargetsNow(eventHandler);
@@ -2754,28 +2748,21 @@ private void updateSkyFunctionsSemaphoreSize(OptionsProvider options) {
  }
 
  protected void syncPackageLoading(
- PackageOptions packageOptions,
  PathPackageLocator pathPackageLocator,
- BuildLanguageOptions buildLanguageOptions,
  UUID commandId,
  Map<String, String> clientEnv,
  TimestampGranularityMonitor tsgm,
  OptionsProvider options)
  throws AbruptExitException {
- syncPackageLoadingInternal(
- packageOptions, pathPackageLocator, buildLanguageOptions, commandId, clientEnv, tsgm);
- }
-
- protected final void syncPackageLoadingInternal(
- PackageOptions packageOptions,
- PathPackageLocator pathPackageLocator,
- BuildLanguageOptions buildLanguageOptions,
- UUID commandId,
- Map<String, String> clientEnv,
- TimestampGranularityMonitor tsgm) {
+ PackageOptions packageOptions = options.getOptions(PackageOptions.class);
  try (SilentCloseable c = Profiler.instance().profile("preparePackageLoading")) {
  preparePackageLoading(
- pathPackageLocator, packageOptions, buildLanguageOptions, commandId, clientEnv, tsgm);
+ pathPackageLocator,
+ packageOptions,
+ options.getOptions(BuildLanguageOptions.class),
+ commandId,
+ clientEnv,
+ tsgm);
  }
  try (SilentCloseable c = Profiler.instance().profile("setDeletedPackages")) {
  setDeletedPackages(packageOptions.getDeletedPackages());

src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD

@@ -41,6 +41,7 @@ java_library(
  "//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
  "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
  "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
+ "//src/main/java/com/google/devtools/build/lib/testing/common:fake-options",
  "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
  "//src/main/java/com/google/devtools/build/lib/util:filetype",
  "//src/main/java/com/google/devtools/build/lib/util/io",

src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java

@@ -57,6 +57,7 @@
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.skyframe.SkyframeTargetPatternEvaluator;
+import com.google.devtools.build.lib.testing.common.FakeOptions;
 import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper;
 import com.google.devtools.build.lib.testutil.TestPackageFactoryBuilderFactory;
 import com.google.devtools.build.lib.util.AbruptExitException;
@@ -72,7 +73,6 @@
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.common.options.Options;
-import com.google.devtools.common.options.OptionsProvider;
 import com.google.errorprone.annotations.ForOverride;
 import java.io.IOException;
 import java.util.AbstractSet;
@@ -254,7 +254,7 @@ public static ResultAndTargets<Target> evaluateQuery(
  public Set<Target> evaluateQueryRaw(String query) throws QueryException, InterruptedException {
  Set<Target> result = new LinkedHashSet<>();
  ThreadSafeOutputFormatterCallback<Target> callback =
- new ThreadSafeOutputFormatterCallback<Target>() {
+ new ThreadSafeOutputFormatterCallback<>() {
  @Override
  public synchronized void processOutput(Iterable<Target> partialResult) {
  Iterables.addAll(result, partialResult);
@@ -299,14 +299,15 @@ protected void initTargetPatternEvaluator(ConfiguredRuleClassProvider ruleClassP
  try {
  skyframeExecutor.sync(
  getReporter(),
- packageOptions,
  packageLocator,
- Options.getDefaults(BuildLanguageOptions.class),
  UUID.randomUUID(),
  ImmutableMap.of(),
  ImmutableMap.of(),
  new TimestampGranularityMonitor(BlazeClock.instance()),
- OptionsProvider.EMPTY);
+ FakeOptions.builder()
+ .put(packageOptions)
+ .putDefaults(BuildLanguageOptions.class)
+ .build());
  } catch (InterruptedException | AbruptExitException e) {
  throw new IllegalStateException(e);
  }

src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java

@@ -33,6 +33,7 @@
 import com.google.devtools.build.lib.pkgcache.PackageOptions;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
+import com.google.devtools.build.lib.testing.common.FakeOptions;
 import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.testutil.TestPackageFactoryBuilderFactory;
@@ -54,7 +55,6 @@
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.common.options.Options;
-import com.google.devtools.common.options.OptionsProvider;
 import java.io.IOException;
 import java.util.List;
 import java.util.Optional;
@@ -310,14 +310,12 @@ private void initEvaluator() throws AbruptExitException, InterruptedException, I
  PrecomputedValue.injected(RepositoryDelegatorFunction.ENABLE_BZLMOD, false)));
  skyframeExecutor.sync(
  reporter,
- packageOptions,
  pathPackageLocator,
- Options.getDefaults(BuildLanguageOptions.class),
  UUID.randomUUID(),
  /*clientEnv=*/ ImmutableMap.of(),
  /*repoEnvOption=*/ ImmutableMap.of(),
  new TimestampGranularityMonitor(BlazeClock.instance()),
- OptionsProvider.EMPTY);
+ FakeOptions.builder().put(packageOptions).putDefaults(BuildLanguageOptions.class).build());
  evaluator = skyframeExecutor.getEvaluator();
  }
 

src/test/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -94,7 +94,6 @@ java_test(
  deps = select({
  "//src/conditions:darwin": [
  "//src/main/java/com/google/devtools/build/lib/skyframe:local_diff_awareness",
- "//src/main/java/com/google/devtools/build/lib/testing/common:fake-options",
  ],
  "//conditions:default": [],
  }) + [
@@ -294,6 +293,7 @@ java_test(
  "//src/test/java/com/google/devtools/build/lib/events:testutil",
  "//src/test/java/com/google/devtools/build/lib/packages:testutil",
  "//src/test/java/com/google/devtools/build/lib/rules/platform:testutil",
+ "//src/main/java/com/google/devtools/build/lib/testing/common:fake-options",
  "//src/test/java/com/google/devtools/build/lib/testutil",
  "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
  "//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper",