Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore usages of ignored flag aliases instead of failing #25125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ignore usages of ignored flag aliases instead of failing #25125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -178,6 +179,7 @@ public static Builder builder() {
private final List<ParsedOptionDescription> skippedOptions = new ArrayList<>();

private final Map<String, String> flagAliasMappings;
private final Set<String> permittedButIgnoredFlagAliases = new HashSet<>();
// We want to keep the invariant that warnings are produced as they are encountered, but only
// show each one once.
private final Set<String> warnings = new LinkedHashSet<>();
Expand Down Expand Up @@ -797,6 +799,12 @@ private ParsedOptionDescriptionOrIgnoredArgs identifyOptionAndPossibleArgument(
throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
}

if (lookupResult == null && permittedButIgnoredFlagAliases.contains(parsedOptionName)) {
// The name references a flag alias defined on an ignored command, ignore it as well.
return new ParsedOptionDescriptionOrIgnoredArgs(
Optional.empty(), Optional.of(commandLineForm.toString()));
}

// Do not recognize internal options, which are treated as if they did not exist.
if (lookupResult == null || shouldIgnoreOption(lookupResult.definition)) {
String suggestion;
Expand Down Expand Up @@ -825,6 +833,23 @@ private ParsedOptionDescriptionOrIgnoredArgs identifyOptionAndPossibleArgument(
}

if (lookupResult.fromFallback) {
if (Objects.equals(aliasFlag, "--" + lookupResult.definition.getOptionName())) {
Map.Entry<String, String> alias;
try {
alias =
(Map.Entry<String, String>)
lookupResult
.definition
.getConverter()
.convert(unconvertedValue, conversionContext);
} catch (OptionsParsingException e) {
// The converter doesn't know the option name, so we supply it here by re-throwing:
throw new OptionsParsingException(
String.format("While parsing option %s: %s", commandLineForm, e.getMessage()), e);
}
permittedButIgnoredFlagAliases.add(alias.getKey());
}

// The option was not found on the current command, but is a valid option for some other
// command. Ignore it.
return new ParsedOptionDescriptionOrIgnoredArgs(
Expand Down
22 changes: 22 additions & 0 deletions src/test/shell/integration/starlark_configurations_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,8 +719,30 @@ function test_rc_flag_alias_supported_under_common_command() {

bazel canonicalize-flags -- --drink=coffee \
>& "$TEST_log" || fail "Expected success"
expect_log "--//$pkg:type=coffee"

bazel build //$pkg:my_drink --drink=coffee \
>& "$TEST_log" || fail "Expected success"
expect_log "type=coffee"
}

function test_rc_flag_alias_with_usage_supported_under_common_command() {
local -r pkg=$FUNCNAME
mkdir -p $pkg

add_to_bazelrc "common --flag_alias=drink=//$pkg:type"
add_to_bazelrc "common --drink=coffee"
write_build_setting_bzl

# canonicalize-flags does not see the common flag in .bazelrc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean? I thought canonicalize-flags does see common settings.

bazel canonicalize-flags -- --drink=coffee >& "$TEST_log" || fail "Expected success"
expect_log "--//$pkg:type=coffee"

bazel build //$pkg:my_drink >& "$TEST_log" || fail "Expected success"
expect_log "type=coffee"

# Does not support Starlark flags.
bazel query //$pkg:my_drink >& "$TEST_log" || fail "Expected success"
}

function test_rc_flag_alias_unsupported_under_test_command() {
Expand Down