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
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index 08cd449..2b0fdaa 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/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 @@
getSkyframeExecutor()
.sync(
reporter,
- options.getOptions(PackageOptions.class),
packageLocator,
- options.getOptions(BuildLanguageOptions.class),
getCommandId(),
clientEnv,
repoEnvFromOptions,
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 511a686..6bada69 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
@@ -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 @@
@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 @@
}
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 @@
@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 @@
}
}
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();
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 954ac1c..9aec4a6 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
@@ -2697,6 +2697,9 @@
/**
* 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.
@@ -2704,9 +2707,7 @@
@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 @@
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 @@
}
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());
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
index b2fed83..447e862 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
@@ -41,6 +41,7 @@
"//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",
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
index 663ea39..cfd4b76 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
+++ b/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 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 @@
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);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java
index 9f592a5..ae9a0df 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java
+++ b/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 @@
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();
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
index bf142bc..5b1fdd1 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -94,7 +94,6 @@
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 @@
"//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",
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
index 4324ffc..e836283 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -110,9 +110,9 @@
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.testutil.ManualClock;
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.CrashFailureDetails;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.Fingerprint;
@@ -136,7 +136,6 @@
import com.google.devtools.build.skyframe.TaggedEvents;
import com.google.devtools.build.skyframe.TrackingAwaiter;
import com.google.devtools.build.skyframe.ValueWithMetadata;
-import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsProvider;
import com.google.protobuf.CodedInputStream;
@@ -189,11 +188,12 @@
options =
OptionsParser.builder()
.optionsClasses(
- ImmutableList.of(
- KeepGoingOption.class,
- BuildRequestOptions.class,
- AnalysisOptions.class,
- CoreOptions.class))
+ AnalysisOptions.class,
+ BuildLanguageOptions.class,
+ BuildRequestOptions.class,
+ CoreOptions.class,
+ KeepGoingOption.class,
+ PackageOptions.class)
.build();
options.parse("--jobs=20");
}
@@ -306,16 +306,7 @@
skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
scratch.overwriteFile("/external/file", "new content");
- skyframeExecutor.sync(
- NullEventHandler.INSTANCE,
- Options.getDefaults(PackageOptions.class),
- skyframeExecutor.getPackageLocator().get(),
- Options.getDefaults(BuildLanguageOptions.class),
- UUID.randomUUID(),
- /*clientEnv=*/ ImmutableMap.of(),
- /*repoEnvOption=*/ ImmutableMap.of(),
- new TimestampGranularityMonitor(new ManualClock()),
- options);
+ syncSkyframeExecutor();
Diff diff = getRecordedDiff();
assertThat(diff.changedKeysWithNewValues()).containsKey(file);
@@ -334,16 +325,7 @@
// Initial sync to establish the baseline DiffAwareness.View.
skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
- skyframeExecutor.sync(
- NullEventHandler.INSTANCE,
- Options.getDefaults(PackageOptions.class),
- skyframeExecutor.getPackageLocator().get(),
- Options.getDefaults(BuildLanguageOptions.class),
- UUID.randomUUID(),
- /*clientEnv=*/ ImmutableMap.of(),
- /*repoEnvOption=*/ ImmutableMap.of(),
- new TimestampGranularityMonitor(new ManualClock()),
- options);
+ syncSkyframeExecutor();
Diff diff = getRecordedDiff();
assertThat(diff.changedKeysWithoutNewValues()).doesNotContain(file);
@@ -372,16 +354,7 @@
skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
scratch.file("/external/foo/new_file");
- skyframeExecutor.sync(
- NullEventHandler.INSTANCE,
- Options.getDefaults(PackageOptions.class),
- skyframeExecutor.getPackageLocator().get(),
- Options.getDefaults(BuildLanguageOptions.class),
- UUID.randomUUID(),
- /*clientEnv=*/ ImmutableMap.of(),
- /*repoEnvOption=*/ ImmutableMap.of(),
- new TimestampGranularityMonitor(new ManualClock()),
- options);
+ syncSkyframeExecutor();
Diff diff = getRecordedDiff();
assertThat(diff.changedKeysWithoutNewValues()).containsNoneOf(dir, dirListingKey);
@@ -411,16 +384,7 @@
// Initial sync to establish the baseline DiffAwareness.View.
skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
- skyframeExecutor.sync(
- NullEventHandler.INSTANCE,
- Options.getDefaults(PackageOptions.class),
- skyframeExecutor.getPackageLocator().get(),
- Options.getDefaults(BuildLanguageOptions.class),
- UUID.randomUUID(),
- /*clientEnv=*/ ImmutableMap.of(),
- /*repoEnvOption=*/ ImmutableMap.of(),
- new TimestampGranularityMonitor(new ManualClock()),
- options);
+ syncSkyframeExecutor();
Diff diff = getRecordedDiff();
assertThat(diff.changedKeysWithoutNewValues()).containsNoneOf(dir, dirListingKey);
@@ -1388,7 +1352,7 @@
* even if "top" is delayed to wait for the shared action2 to run, the assertion that the artifact
* exists will fail, since action2's "prepare" step deleted it.
*/
- @Ignore
+ @Ignore("b/150153544")
@Test
public void incrementalSharedActions() throws Exception {
Path root = getExecRoot();
@@ -2490,4 +2454,15 @@
MoreAsserts.assertNotContainsEvent(
eventCollector, Pattern.compile(".*after scanning.*\n.*Scanning.*\n.*Test dir/top.*"));
}
+
+ private void syncSkyframeExecutor() throws AbruptExitException, InterruptedException {
+ skyframeExecutor.sync(
+ reporter,
+ skyframeExecutor.getPackageLocator().get(),
+ UUID.randomUUID(),
+ /*clientEnv=*/ ImmutableMap.of(),
+ /*repoEnvOption=*/ ImmutableMap.of(),
+ new TimestampGranularityMonitor(BlazeClock.instance()),
+ options);
+ }
}
