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