Properly setup package cache before doing starlark options parsing.
Consolidate calls to CommandEnvironment#setupPackageCache to one place pre-starlark options parsing and pre-command execution. Both starlark options parsing and CommandEnviroment#setupPackageCache happen only for commands that build (according to CommandAnnotation#build) (with some exceptions).
PiperOrigin-RevId: 252751079
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 c00274d..da70c87 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
@@ -110,10 +110,6 @@
try (SilentCloseable c = Profiler.instance().profile("createBuildOptions")) {
buildOptions = runtime.createBuildOptions(request);
}
- // Sync the package manager before sending the BuildStartingEvent in runLoadingPhase()
- try (SilentCloseable c = Profiler.instance().profile("setupPackageCache")) {
- env.setupPackageCache(request);
- }
ExecutionTool executionTool = null;
boolean catastrophe = false;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index b0dd9a6..ae20413 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -464,6 +464,36 @@
}
}
+ // {@link CleanCommand} is annotated with {@code builds = true}
+ // to have access to relevant build options but don't actually do building.
+ // {@link InfoCommand} is annotated with {@code builds = true} but only conditionally
+ // does this step based on some complicated logic.
+ if (commandAnnotation.builds()
+ && !commandAnnotation.name().equals("clean")
+ && !commandAnnotation.name().equals("info")) {
+ try {
+ env.setupPackageCache(options);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ env.getReporter()
+ .handle(Event.error("command interrupted while setting up package cache"));
+ earlyExitCode = ExitCode.INTERRUPTED;
+ } catch (AbruptExitException e) {
+ env.getReporter().handle(Event.error(e.getMessage()));
+ earlyExitCode = e.getExitCode();
+ }
+ if (!earlyExitCode.equals(ExitCode.SUCCESS)) {
+ return replayEarlyExitEvents(
+ outErr,
+ optionHandler,
+ storedEventHandler,
+ env,
+ earlyExitCode,
+ new NoBuildEvent(
+ commandName, firstContactTime, false, true, env.getCommandId().toString()));
+ }
+ }
+
// Parse starlark options.
earlyExitCode = optionHandler.parseStarlarkOptions(env, storedEventHandler);
if (!earlyExitCode.equals(ExitCode.SUCCESS)) {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 7d28683..9944a75 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -244,11 +244,11 @@
return ExitCode.SUCCESS;
}
try {
- StarlarkOptionsParser.newStarlarkOptionsParser(env, optionsParser, runtime)
+ StarlarkOptionsParser.newStarlarkOptionsParser(env, optionsParser)
.parse(commandAnnotation, eventHandler);
} catch (OptionsParsingException e) {
- eventHandler.handle(Event.error(e.getMessage()));
- return ExitCode.COMMAND_LINE_ERROR;
+ env.getReporter().handle(Event.error(e.getMessage()));
+ return ExitCode.PARSING_FAILURE;
}
return ExitCode.SUCCESS;
}
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 eaf0153..668b333 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
@@ -89,6 +89,7 @@
private OutputService outputService;
private Path workingDirectory;
private String workspaceName;
+ private boolean haveSetupPackageCache = false;
private AtomicReference<AbruptExitException> pendingException = new AtomicReference<>();
@@ -571,6 +572,15 @@
*/
public void setupPackageCache(OptionsProvider options)
throws InterruptedException, AbruptExitException {
+ // We want to ensure that we're never calling #setupPackageCache twice in the same build because
+ // it does the very expensive work of diffing the cache between incremental builds.
+ // {@link SequencedSkyframeExecutor#handleDiffs} is the particular method we don't want to be
+ // calling twice. We could feasibly factor it out of this call.
+ if (this.haveSetupPackageCache) {
+ throw new IllegalStateException(
+ "We should never call this method more than once over the course of a single command");
+ }
+ this.haveSetupPackageCache = true;
getSkyframeExecutor()
.sync(
reporter,
@@ -583,19 +593,6 @@
options);
}
- public void syncPackageLoading(
- PackageCacheOptions packageCacheOptions, StarlarkSemanticsOptions starlarkSemanticsOptions)
- throws AbruptExitException {
- getSkyframeExecutor()
- .syncPackageLoading(
- packageCacheOptions,
- packageLocator,
- starlarkSemanticsOptions,
- getCommandId(),
- clientEnv,
- timestampGranularityMonitor);
- }
-
public void recordLastExecutionTime() {
workspace.recordLastExecutionTime(getCommandStartTime());
}
@@ -687,6 +684,9 @@
eventBus.post(new CommandStartEvent(
command.name(), getCommandId(), getClientEnv(), workingDirectory, getDirectories(),
waitTimeInMs + commonOptions.waitTime));
+
+ // Modules that are subscribed to CommandStartEvents may create pending exceptions.
+ throwPendingException();
}
/** Returns the name of the file system we are writing output to. */
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index 471419d..bba0827 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -28,14 +28,11 @@
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.packages.BuildSetting;
import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.LoadingOptions;
-import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue;
import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.Converter;
@@ -70,15 +67,7 @@
}
static StarlarkOptionsParser newStarlarkOptionsParser(
- CommandEnvironment env, OptionsParser optionsParser, BlazeRuntime runtime)
- throws OptionsParsingException {
- try {
- env.syncPackageLoading(
- optionsParser.getOptions(PackageCacheOptions.class),
- optionsParser.getOptions(StarlarkSemanticsOptions.class));
- } catch (AbruptExitException e) {
- throw new OptionsParsingException(e.getMessage());
- }
+ CommandEnvironment env, OptionsParser optionsParser) {
return new StarlarkOptionsParser(
env.getSkyframeExecutor(),
env.getRelativeWorkingDirectory(),
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java
index 96f0369..78d3eb2 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java
@@ -70,6 +70,7 @@
BlazeRuntime runtime = env.getRuntime();
List<String> targets;
try (SilentCloseable closeable = Profiler.instance().profile("ProjectFileSupport.getTargets")) {
+ // only takes {@code options} to get options.getResidue()
targets = ProjectFileSupport.getTargets(runtime.getProjectFileProvider(), options);
}
if (targets.isEmpty()) {
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 7238381..acac27a 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
@@ -2633,8 +2633,8 @@
TimestampGranularityMonitor tsgm,
OptionsProvider options)
throws InterruptedException, AbruptExitException {
- getActionEnvFromOptions(options);
- setRepoEnv(options);
+ getActionEnvFromOptions(options.getOptions(CoreOptions.class));
+ setRepoEnv(options.getOptions(CoreOptions.class));
RemoteOptions remoteOptions = options.getOptions(RemoteOptions.class);
setRemoteOutputsMode(
remoteOptions != null
@@ -2679,10 +2679,9 @@
invalidateTransientErrors();
}
- private void getActionEnvFromOptions(OptionsProvider options) {
+ private void getActionEnvFromOptions(CoreOptions opt) {
// ImmutableMap does not support null values, so use a LinkedHashMap instead.
LinkedHashMap<String, String> actionEnvironment = new LinkedHashMap<>();
- CoreOptions opt = options.getOptions(CoreOptions.class);
if (opt != null) {
for (Map.Entry<String, String> v : opt.actionEnvironment) {
actionEnvironment.put(v.getKey(), v.getValue());
@@ -2696,8 +2695,7 @@
PrecomputedValue.ACTION_ENV.set(injectable(), actionEnv);
}
- private void setRepoEnv(OptionsProvider options) {
- CoreOptions opt = options.getOptions(CoreOptions.class);
+ private void setRepoEnv(CoreOptions opt) {
LinkedHashMap<String, String> repoEnv = new LinkedHashMap<>();
if (opt != null) {
for (Map.Entry<String, String> v : opt.repositoryEnvironment) {
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
index b008e6e..684581f 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
@@ -34,6 +34,7 @@
"//src/main/java/com/google/devtools/build/lib/exec/local:options",
"//src/main/java/com/google/devtools/build/lib/includescanning",
"//src/main/java/com/google/devtools/build/lib/network:noop_connectivity",
+ "//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/sandbox",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/standalone",
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
index 4ea6a29..230d6ae 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
@@ -36,6 +36,8 @@
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.pkgcache.LoadingOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.runtime.BlazeCommand;
import com.google.devtools.build.lib.runtime.BlazeCommandEventHandler;
import com.google.devtools.build.lib.runtime.BlazeCommandResult;
@@ -335,6 +337,9 @@
}
try {
+ try (SilentCloseable c = Profiler.instance().profile("setupPackageCache")) {
+ env.setupPackageCache(lastRequest);
+ }
buildTool.buildTargets(lastRequest, lastResult, null);
success = true;
} finally {
diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh
index 32bd972..aab82d9 100755
--- a/src/test/shell/integration/discard_graph_edges_test.sh
+++ b/src/test/shell/integration/discard_graph_edges_test.sh
@@ -403,9 +403,10 @@
genrule(name = "foo", cmd = "touch $@", outs = ["foo.out"])
EOF
+ readonly local server_pid="$(bazel info server_pid 2> /dev/null)"
bazel build $BUILD_FLAGS //foo:foo \
>& "$TEST_log" || fail "Expected success"
- "$jmaptool" -histo:live "$(bazel info server_pid)" > histo.txt
+ "$jmaptool" -histo:live $server_pid > histo.txt
genrule_action_count="$(extract_histogram_count histo.txt \
'GenRuleAction$')"
if [[ "$genrule_action_count" -lt 1 ]]; then
diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh
index e0b2e59..8004a9e 100755
--- a/src/test/shell/integration/starlark_configurations_test.sh
+++ b/src/test/shell/integration/starlark_configurations_test.sh
@@ -223,7 +223,196 @@
expect_log "type=cowabunga"
}
-# Test that label-typed build setting has access to providers of the target it points to.
+# Regression tests for b/134580627
+# Make sure package incrementality works during options parsing
+function test_incremental_delete_build_setting() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+
+ cat > $pkg/rules.bzl <<EOF
+def _impl(ctx):
+ return []
+
+my_flag = rule(
+ implementation = _impl,
+ build_setting = config.string(flag = True)
+)
+simple_rule = rule(implementation = _impl)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "my_flag", "simple_rule")
+
+my_flag(name = "my_flag", build_setting_default = "default")
+simple_rule(name = "simple_rule")
+EOF
+
+ bazel build //$pkg:simple_rule --//$pkg:my_flag=cowabunga \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "simple_rule")
+
+simple_rule(name = "simple_rule")
+EOF
+
+ bazel build //$pkg:simple_rule --//$pkg:my_flag=cowabunga \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ && fail "Expected failure" || true
+
+ expect_log "no such target '//$pkg:my_flag'"
+}
+
+#############################
+
+function test_incremental_delete_build_setting_in_different_package() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+
+ cat > $pkg/rules.bzl <<EOF
+def _impl(ctx):
+ return []
+
+my_flag = rule(
+ implementation = _impl,
+ build_setting = config.string(flag = True)
+)
+simple_rule = rule(implementation = _impl)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "my_flag")
+my_flag(name = "my_flag", build_setting_default = "default")
+EOF
+
+ mkdir -p pkg2
+
+ cat > pkg2/BUILD <<EOF
+load("//$pkg:rules.bzl", "simple_rule")
+simple_rule(name = "simple_rule")
+EOF
+
+ bazel build //pkg2:simple_rule --//$pkg:my_flag=cowabunga \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+
+ cat > $pkg/BUILD <<EOF
+EOF
+
+ bazel build //pkg2:simple_rule --//$pkg:my_flag=cowabunga \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ && fail "Expected failure" || true
+
+ expect_log "no such target '//$pkg:my_flag'"
+}
+
+#############################
+
+
+function test_incremental_add_build_setting() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+
+ cat > $pkg/rules.bzl <<EOF
+def _impl(ctx):
+ return []
+
+my_flag = rule(
+ implementation = _impl,
+ build_setting = config.string(flag = True)
+)
+simple_rule = rule(implementation = _impl)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "simple_rule")
+
+simple_rule(name = "simple_rule")
+EOF
+
+ bazel build //$pkg:simple_rule --experimental_build_setting_api \
+ > output 2>"$TEST_log" || fail "Expected success"
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "my_flag", "simple_rule")
+
+my_flag(name = "my_flag", build_setting_default = "default")
+simple_rule(name = "simple_rule")
+EOF
+
+ bazel build //$pkg:simple_rule --//$pkg:my_flag=cowabunga \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+}
+
+function test_incremental_change_build_setting() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+
+ cat > $pkg/rules.bzl <<EOF
+MyProvider = provider(fields = ["value"])
+
+def _flag_impl(ctx):
+ return MyProvider(value = ctx.build_setting_value)
+
+my_flag = rule(
+ implementation = _flag_impl,
+ build_setting = config.string(flag = True)
+)
+
+def _rule_impl(ctx):
+ print("flag = " + ctx.attr.flag[MyProvider].value)
+
+simple_rule = rule(
+ implementation = _rule_impl,
+ attrs = {"flag" : attr.label()}
+)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:rules.bzl", "my_flag", "simple_rule")
+
+my_flag(name = "my_flag", build_setting_default = "default")
+simple_rule(name = "simple_rule", flag = ":my_flag")
+EOF
+
+ bazel build //$pkg:simple_rule --//$pkg:my_flag=yabadabadoo \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+
+ expect_log "flag = yabadabadoo"
+
+# update the flag to return a different value
+ cat > $pkg/rules.bzl <<EOF
+MyProvider = provider(fields = ["value"])
+
+def _flag_impl(ctx):
+ return MyProvider(value = "scoobydoobydoo")
+
+my_flag = rule(
+ implementation = _flag_impl,
+ build_setting = config.string(flag = True)
+)
+
+def _rule_impl(ctx):
+ print("flag = " + ctx.attr.flag[MyProvider].value)
+
+simple_rule = rule(
+ implementation = _rule_impl,
+ attrs = {"flag" : attr.label()}
+)
+EOF
+
+ bazel build //$pkg:simple_rule --//$pkg:my_flag=yabadabadoo \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+
+ expect_log "flag = scoobydoobydoo"
+}
+
+# Test that label-typed build setting has access to providers of the
+# target it points to.
function test_label_flag() {
local -r pkg=$FUNCNAME
mkdir -p $pkg
