Skip to content

Commit

Permalink
Support flag overrides of config settings in external repositories.
Browse files Browse the repository at this point in the history
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 #10039

Closes #10052.

PiperOrigin-RevId: 276153907
  • Loading branch information
jmillikin-stripe authored and copybara-github committed Oct 22, 2019
1 parent 42b3be2 commit fff7309
Show file tree
Hide file tree
Showing 7 changed files with 199 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,7 @@ private OptionsParser createOptionsParser(BlazeCommand command)
OptionsParser.builder()
.optionsData(optionsData)
.skippedPrefix("--//")
.skippedPrefix("--@")
.allowResidue(annotation.allowResidue())
.build();
return parser;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -183,7 +182,7 @@ public Builder argsPreProcessor(ArgsPreProcessor preProcessor) {
}

/** 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class OptionsParserImpl {
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. */
Expand All @@ -63,8 +63,8 @@ public Builder argsPreProcessor(ArgsPreProcessor preProcessor) {
}

/** 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;
}

Expand All @@ -77,7 +77,10 @@ public Builder ignoreInternalOptions(boolean ignoreInternalOptions) {
/** 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);
}
}

Expand Down Expand Up @@ -124,17 +127,17 @@ public static Builder builder() {

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;
}

Expand All @@ -145,10 +148,11 @@ OptionsData getOptionsData() {

/** 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()}. */
Expand Down Expand Up @@ -350,7 +354,7 @@ private ResidueAndPriority parse(
continue; // not an option arg
}

if (skippedPrefix != null && arg.startsWith(skippedPrefix)) {
if (skippedPrefixes.stream().anyMatch(prefix -> arg.startsWith(prefix))) {
unparsedArgs.add(arg);
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,21 @@ public void testFlagSpaceValueForm() throws Exception {
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 {
Expand Down
20 changes: 20 additions & 0 deletions src/test/shell/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,26 @@ sh_test(
],
)

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",
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
4 changes: 2 additions & 2 deletions src/test/shell/integration/starlark_configurations_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,8 @@ EOF
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
Expand Down

0 comments on commit fff7309

Please sign in to comment.