Support flag overrides of config settings in external repositories.
The user-defined flags documented at https://docs.bazel.build/versions/1.0.0/skylark/config.html can't currently be set across repositories with `--@some_repo//:some_flag`. It looks like this is just a simple oversight in the flag parser, and after changing it to accept `--@` as a Starlark flag prefix the rest of the functionality is working.
Fixes https://github.com/bazelbuild/bazel/issues/10039
Closes #10052.
PiperOrigin-RevId: 276153907
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 5a6557b..a245343 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
@@ -655,6 +655,7 @@
OptionsParser.builder()
.optionsData(optionsData)
.skippedPrefix("--//")
+ .skippedPrefix("--@")
.allowResidue(annotation.allowResidue())
.build();
return parser;
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index c495b7f..8b6e673 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -36,7 +36,6 @@
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
-import javax.annotation.Nullable;
/**
* A parser for options. Typical use case in a main method:
@@ -183,7 +182,7 @@
}
/** Any flags with this prefix will be skipped during processing. */
- public Builder skippedPrefix(@Nullable String skippedPrefix) {
+ public Builder skippedPrefix(String skippedPrefix) {
this.implBuilder.skippedPrefix(skippedPrefix);
return this;
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index d8b47e4..f4ad4bf 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -47,7 +47,7 @@
static final class Builder {
private OptionsData optionsData;
private ArgsPreProcessor argsPreProcessor = args -> args;
- @Nullable private String skippedPrefix;
+ private final ArrayList<String> skippedPrefixes = new ArrayList<>();
private boolean ignoreInternalOptions = true;
/** Set the {@link OptionsData} to be used in this instance. */
@@ -63,8 +63,8 @@
}
/** Any flags with this prefix will be skipped during processing. */
- public Builder skippedPrefix(@Nullable String skippedPrefix) {
- this.skippedPrefix = skippedPrefix;
+ public Builder skippedPrefix(String skippedPrefix) {
+ this.skippedPrefixes.add(skippedPrefix);
return this;
}
@@ -77,7 +77,10 @@
/** Returns a newly-initialized {@link OptionsParserImpl}. */
public OptionsParserImpl build() {
return new OptionsParserImpl(
- this.optionsData, this.argsPreProcessor, this.skippedPrefix, this.ignoreInternalOptions);
+ this.optionsData,
+ this.argsPreProcessor,
+ this.skippedPrefixes,
+ this.ignoreInternalOptions);
}
}
@@ -124,17 +127,17 @@
private final List<String> warnings = new ArrayList<>();
private final ArgsPreProcessor argsPreProcessor;
- @Nullable private final String skippedPrefix;
+ private final List<String> skippedPrefixes;
private final boolean ignoreInternalOptions;
OptionsParserImpl(
OptionsData optionsData,
ArgsPreProcessor argsPreProcessor,
- @Nullable String skippedPrefix,
+ List<String> skippedPrefixes,
boolean ignoreInternalOptions) {
this.optionsData = optionsData;
this.argsPreProcessor = argsPreProcessor;
- this.skippedPrefix = skippedPrefix;
+ this.skippedPrefixes = skippedPrefixes;
this.ignoreInternalOptions = ignoreInternalOptions;
}
@@ -145,10 +148,11 @@
/** Returns a {@link Builder} that is configured the same as this parser. */
Builder toBuilder() {
- return builder()
- .optionsData(optionsData)
- .argsPreProcessor(argsPreProcessor)
- .skippedPrefix(skippedPrefix);
+ Builder builder = builder().optionsData(optionsData).argsPreProcessor(argsPreProcessor);
+ for (String skippedPrefix : skippedPrefixes) {
+ builder.skippedPrefix(skippedPrefix);
+ }
+ return builder;
}
/** Implements {@link OptionsParser#asCompleteListOfParsedOptions()}. */
@@ -350,7 +354,7 @@
continue; // not an option arg
}
- if (skippedPrefix != null && arg.startsWith(skippedPrefix)) {
+ if (skippedPrefixes.stream().anyMatch(prefix -> arg.startsWith(prefix))) {
unparsedArgs.add(arg);
continue;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
index cce15bc..9d0b565 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
@@ -161,6 +161,21 @@
assertThat(result.getResidue()).isEmpty();
}
+ // test --@workspace//flag=value
+ @Test
+ public void testFlagNameWithWorkspace() throws Exception {
+ writeBasicIntFlag();
+ rewriteWorkspace("workspace(name = 'starlark_options_test')");
+
+ OptionsParsingResult result =
+ parseStarlarkOptions("--@starlark_options_test//test:my_int_setting=666");
+
+ assertThat(result.getStarlarkOptions()).hasSize(1);
+ assertThat(result.getStarlarkOptions().get("@starlark_options_test//test:my_int_setting"))
+ .isEqualTo(666);
+ assertThat(result.getResidue()).isEmpty();
+ }
+
// test --fake_flag=value
@Test
public void testBadFlag_equalsForm() throws Exception {
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index 80e2baf..7e24350 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -258,6 +258,26 @@
)
sh_test(
+ name = "starlark_configurations_test",
+ size = "medium",
+ srcs = ["starlark_configurations_test.sh"],
+ data = [
+ ":test-deps",
+ "@bazel_tools//tools/bash/runfiles",
+ ],
+)
+
+sh_test(
+ name = "starlark_configurations_external_workspaces_test",
+ size = "medium",
+ srcs = ["starlark_configurations_external_workspaces_test.sh"],
+ data = [
+ ":test-deps",
+ "@bazel_tools//tools/bash/runfiles",
+ ],
+)
+
+sh_test(
name = "analysis_test_test",
size = "medium",
srcs = ["analysis_test_test.sh"],
diff --git a/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh b/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh
new file mode 100755
index 0000000..dc4625f
--- /dev/null
+++ b/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh
@@ -0,0 +1,144 @@
+#!/bin/bash
+#
+# Copyright 2019 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# starlark_configuration_test.sh: integration tests for starlark build
+# configurations with external workspaces.
+
+# --- begin runfiles.bash initialization ---
+# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash).
+
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+set -euo pipefail
+if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ if [[ -f "$0.runfiles_manifest" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
+ elif [[ -f "$0.runfiles/MANIFEST" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST"
+ elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ export RUNFILES_DIR="$0.runfiles"
+ fi
+fi
+if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash"
+elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \
+ "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)"
+else
+ echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash"
+ exit 1
+fi
+# --- end runfiles.bash initialization ---
+
+source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
+ || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+ declare -r is_windows=true
+ ;;
+*)
+ declare -r is_windows=false
+ ;;
+esac
+
+if "$is_windows"; then
+ export MSYS_NO_PATHCONV=1
+ export MSYS2_ARG_CONV_EXCL="*"
+fi
+
+add_to_bazelrc "build --package_path=%workspace%"
+
+#### HELPER FXNS #######################################################
+
+function write_build_setting_bzl() {
+ declare -r at_workspace="${1:-}"
+
+ cat > $pkg/provider.bzl <<EOF
+BuildSettingInfo = provider(fields = ['name', 'value'])
+EOF
+
+ cat > $pkg/build_setting.bzl <<EOF
+load("@${WORKSPACE_NAME}//$pkg:provider.bzl", "BuildSettingInfo")
+
+def _build_setting_impl(ctx):
+ return [BuildSettingInfo(name = ctx.attr.name, value = ctx.build_setting_value)]
+
+drink_attribute = rule(
+ implementation = _build_setting_impl,
+ build_setting = config.string(flag = True),
+)
+EOF
+
+ cat > $pkg/rules.bzl <<EOF
+load("@${WORKSPACE_NAME}//$pkg:provider.bzl", "BuildSettingInfo")
+
+def _impl(ctx):
+ _type_name = ctx.attr._type[BuildSettingInfo].name
+ _type_setting = ctx.attr._type[BuildSettingInfo].value
+ print(_type_name + "=" + str(_type_setting))
+ _temp_name = ctx.attr._temp[BuildSettingInfo].name
+ _temp_setting = ctx.attr._temp[BuildSettingInfo].value
+ print(_temp_name + "=" + str(_temp_setting))
+ print("strict_java_deps=" + ctx.fragments.java.strict_java_deps)
+
+drink = rule(
+ implementation = _impl,
+ attrs = {
+ "_type":attr.label(default = Label("$at_workspace//$pkg:type")),
+ "_temp":attr.label(default = Label("$at_workspace//$pkg:temp")),
+ },
+ fragments = ["java"],
+)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:build_setting.bzl", "drink_attribute")
+load("//$pkg:rules.bzl", "drink")
+
+drink(name = 'my_drink')
+
+drink_attribute(
+ name = 'type',
+ build_setting_default = 'unknown',
+ visibility = ['//visibility:public'],
+)
+drink_attribute(
+ name = 'temp',
+ build_setting_default = 'unknown',
+ visibility = ['//visibility:public'],
+)
+EOF
+}
+
+#### TESTS #############################################################
+
+
+function test_set_flag_with_workspace_name() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+
+ write_build_setting_bzl "@${WORKSPACE_NAME}"
+
+ bazel build //$pkg:my_drink --@"${WORKSPACE_NAME}"//$pkg:type="coffee" \
+ --experimental_build_setting_api > output 2>"$TEST_log" \
+ || fail "Expected success"
+
+ expect_log "type=coffee"
+}
+
+
+run_suite "${PRODUCT_NAME} starlark configurations tests"
diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh
index a8697d4..71c6c29 100755
--- a/src/test/shell/integration/starlark_configurations_test.sh
+++ b/src/test/shell/integration/starlark_configurations_test.sh
@@ -515,8 +515,8 @@
TARGET=$(grep "//$pkg:with_rule_class_transition " output)
# Trim to just configuration
- OUTPUT_CONFIG=${OUTPUT/"//$pkg:with_rule_class_transition.output "}
- TARGET_CONFIG=${TARGET/"//$pkg:with_rule_class_transition "}
+ OUTPUT_CONFIG=${OUTPUT#* }
+ TARGET_CONFIG=${TARGET#* }
# Confirm same configuration
assert_equals $OUTPUT_CONFIG $TARGET_CONFIG
