Skip to content

Let common ignore unsupported options #18130

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

Closed
wants to merge 8 commits into from
Closed

Let common ignore unsupported options #18130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
57b1e31
Support `all-supported` pseudo command in rc files
fmeum Apr 16, 2023
7d1d421
Add comments
fmeum May 5, 2023
b5d22e0
Clarify test and add comments
fmeum May 5, 2023
e291f0e
Add single-line test
fmeum May 6, 2023
d2488c4
Mention ignored arguments with `--announce_rc`
fmeum May 11, 2023
d169c0e
Extract names for pseudo commands into constants
fmeum May 11, 2023
22ebb20
Introduce `always` and change `common` semantics
fmeum May 11, 2023
e282272
Address review comments
fmeum May 19, 2023
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
9 changes: 8 additions & 1 deletion site/en/run/bazelrc.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,14 @@ line specifies when these defaults are applied:

- `startup`: startup options, which go before the command, and are described
in `bazel help startup_options`.
- `common`: options that apply to all Bazel commands.
- `common`: options that should be applied to all Bazel commands that support
them. If a command does not support an option specified in this way, the
option will be ignored if it is valid for *some* other Bazel command. Note
that this only applies to option names: If the current command accepts an
option with the specified name, but doesn't support the specified value, it
will fail.
- `always`: options that apply to all Bazel commands. If a command does not
support an option specified in this way, it will fail.
- _`command`_: Bazel command, such as `build` or `query` to which the options
apply. These options also apply to all commands that inherit from the
specified command. (For example, `test` inherits from `build`.)
Expand Down
65 changes: 55 additions & 10 deletions src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;

import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.collect.ArrayListMultimap;
Expand Down Expand Up @@ -42,6 +44,7 @@
import com.google.devtools.common.options.InvocationPolicyEnforcer;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionPriority.PriorityCategory;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.OptionsParsingResult;
Expand Down Expand Up @@ -74,6 +77,14 @@ public final class BlazeOptionHandler {
"client_env",
"client_cwd");

// All options set on this pseudo command are inherited by all commands, with unrecognized options
// resulting in an error.
private static final String ALWAYS_PSEUDO_COMMAND = "always";

// All options set on this pseudo command are inherited by all commands, with unrecognized options
// being ignored as long as they are recognized by at least one (other) command.
private static final String COMMON_PSEUDO_COMMAND = "common";

// Marks an event to indicate a parsing error.
static final String BAD_OPTION_TAG = "invalidOption";
// Separates the invalid tag from the full error message for easier parsing.
Expand All @@ -86,6 +97,7 @@ public final class BlazeOptionHandler {
private final Command commandAnnotation;
private final InvocationPolicy invocationPolicy;
private final List<String> rcfileNotes = new ArrayList<>();
private final List<Class<? extends OptionsBase>> allOptionsClasses;

BlazeOptionHandler(
BlazeRuntime runtime,
Expand All @@ -100,6 +112,12 @@ public final class BlazeOptionHandler {
this.commandAnnotation = commandAnnotation;
this.optionsParser = optionsParser;
this.invocationPolicy = invocationPolicy;
this.allOptionsClasses = runtime.getCommandMap().values().stream()
.map(BlazeCommand::getClass)
.flatMap(cmd -> BlazeCommandUtils.getOptions(cmd, runtime.getBlazeModules(),
runtime.getRuleClassProvider()).stream())
.distinct()
.collect(toImmutableList());
Copy link
Contributor

@haxorz haxorz Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we used a Set<Class> rather than List<Class> could we then get rid of the allowDuplicatesParsingEquivalently and #equivalentForParsing stuff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, we should add a .distinct() here to make the cache key smaller.

I don't think that passing to a Set would reduce the overall complexity of this PR though: The problem isn't that option classes collide, it's that individual options can collide across classes. For example, both cquery and query define an --output flag, but those flag definition are distinct and differ in certain aspects (e.g. the allowed values). However, crucially, both options accept an argument (that is, are neither boolean nor Void options) and hence parse identically, as checked by equivalentForParsing. That's why this PR introduces an additional type of equality that is more lenient than Object#equals for OptionDefinitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks that wasn't clear to me. Hmm that is an annoying bit of necessary complexity of this FR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would definitely add more comments as part of polishing this PR if you approve of the approach.

I agree that this additional complexity is annoying, but if it isn't taken care of by Bazel itself, it is pushed onto end users. I actually think that compared to the level of effort required to manually apply flags to all commands they are valid for, the complexity of OptionsParserImpl#equivalentForParsing is comparatively contained.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, both cquery and query define an --output flag ... However, crucially, both options

What would happen if they weren't equivalent for parsing?

However, crucially, both options accept an argument (that is, are neither boolean nor Void options) and hence parse identically

What would be wrong with two different options with the same name that were both booleans?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would happen if they weren't equivalent for parsing?

That would result in an error when all-supported is used. This error could be caught by tests. It does restrict what options Bazel commands can provide to some extent, but I see that as necessary to support this feature.

What would be wrong with two different options with the same name that were both booleans?

That would also work. It would only error if one is a boolean and the other accepts a value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I approve of the approach.

That would result in an error when all-supported is used. This error could be caught by tests. It does restrict what options Bazel commands can provide to some extent, but I see that as necessary to support this feature.

Thanks that's what I figured. Agree this restriction to the codebase is a sensible requirement for this FR. Please make this clear in a code comment.

That would also work. It would only error if one is a boolean and the other accepts a value.

Sounds good. I see that #equivalentForParsing handles this. I was moreso questioning the phrasing of your PR comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on why we're doing the equivalentForParsing check, especially because it seems like a very loose check. For example, if some_option accepts different values in different commands (such as two different enums), the check passes correct? What are we trying to guard against exactly?

}

/**
Expand Down Expand Up @@ -186,6 +204,24 @@ void parseRcOptions(
for (String commandToParse : getCommandNamesToParse(commandAnnotation)) {
// Get all args defined for this command (or "common"), grouped by rc chunk.
for (RcChunkOfArgs rcArgs : commandToRcArgs.get(commandToParse)) {
ImmutableList<String> ignoredArgs = ImmutableList.of();
if (commandToParse.equals(COMMON_PSEUDO_COMMAND)) {
// Pass in options data for all commands supported by the runtime so that options that
// apply to some but not the current command can be ignored.
//
// Important note: The consistency checks performed by
// OptionsParser#getFallbackOptionsData ensure that there aren't any two options across
// all commands that have the same name but parse differently (e.g. because one accepts
// a value and the other doesn't). This means that the options available on a command
// limit the options available on other commands even without command inheritance. This
// restriction is necessary to ensure that the options specified on the "common"
// pseudo command can be parsed unambiguously.
ignoredArgs = optionsParser.parseWithSourceFunction(PriorityCategory.RC_FILE,
o -> rcArgs.getRcFile(),
rcArgs.getArgs(), OptionsParser.getFallbackOptionsData(allOptionsClasses));
} else {
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
}
if (!rcArgs.getArgs().isEmpty()) {
String inherited = commandToParse.equals(commandAnnotation.name()) ? "" : "Inherited ";
String source =
Expand All @@ -194,12 +230,15 @@ void parseRcOptions(
: String.format(
"Reading rc options for '%s' from %s",
commandAnnotation.name(), rcArgs.getRcFile());
rcfileNotes.add(
String.format(
"%s:\n %s'%s' options: %s",
source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs())));
String rcFileNote = String.format(
"%s:\n %s'%s' options: %s",
source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs()));
if (!ignoredArgs.isEmpty()) {
rcFileNote += String.format("\n Ignored as unsupported by '%s': %s",
commandAnnotation.name(), Joiner.on(' ').join(ignoredArgs));
}
rcfileNotes.add(rcFileNote.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor issue here detected by an internal test when I tried to import. Adding the note after the call to optionsParser.parse() doesn't work because that call may throw OptionsParsingException, and we won't get a note for the illegal option. Seems like that test case should be open sourced. Anyway, I'm changing this to:

for (RcChunkOfArgs rcArgs : commandToRcArgs.get(commandToParse)) {
  if (!rcArgs.getArgs().isEmpty()) {
    String inherited = commandToParse.equals(commandAnnotation.name()) ? "" : "Inherited ";
    String source =
        rcArgs.getRcFile().equals("client")
            ? "Options provided by the client"
            : String.format(
                "Reading rc options for '%s' from %s",
                commandAnnotation.name(), rcArgs.getRcFile());
    rcfileNotes.add(
        String.format(
            "%s:\n  %s'%s' options: %s",
            source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs())));
  }
  if (commandToParse.equals(COMMON_PSEUDO_COMMAND)) {
    // Pass in options data for all commands supported by the runtime so that options that
    // apply to some but not the current command can be ignored.
    //
    // Important note: The consistency checks performed by
    // OptionsParser#getFallbackOptionsData ensure that there aren't any two options across
    // all commands that have the same name but parse differently (e.g. because one accepts
    // a value and the other doesn't). This means that the options available on a command
    // limit the options available on other commands even without command inheritance. This
    // restriction is necessary to ensure that the options specified on the "common"
    // pseudo command can be parsed unambiguously.
    ImmutableList<String> ignoredArgs =
        optionsParser.parseWithSourceFunction(
            PriorityCategory.RC_FILE,
            o -> rcArgs.getRcFile(),
            rcArgs.getArgs(),
            OptionsParser.getFallbackOptionsData(allOptionsClasses));
    if (!ignoredArgs.isEmpty()) {
      // Append richer information to the note.
      int index = rcfileNotes.size() - 1;
      String note = rcfileNotes.get(index);
      note +=
          String.format(
              "\n  Ignored as unsupported by '%s': %s",
              commandAnnotation.name(), Joiner.on(' ').join(ignoredArgs));
      rcfileNotes.set(index, note);
    }
  } else {
    optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, the change looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

With that out of the way, tests are now passing. But apparently we have more internal code outside of blaze that depends on the options parsing libraries, and I need to fix them up too. Please bear with me.

}
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
}
}
}
Expand Down Expand Up @@ -235,7 +274,8 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa
optionsParser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE,
commandOptionSourceFunction,
defaultOverridesAndRcSources.build());
defaultOverridesAndRcSources.build(),
/* fallbackData= */null);

// Command-specific options from .blazerc passed in via --default_override and --rc_source.
ClientOptions rcFileOptions = optionsParser.getOptions(ClientOptions.class);
Expand All @@ -249,7 +289,8 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa

// Parses the remaining command-line options.
optionsParser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build());
PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build(),
/* fallbackData= */null);

if (commandAnnotation.builds()) {
// splits project files from targets in the traditional sense
Expand Down Expand Up @@ -459,14 +500,17 @@ void expandConfigOptions(
ConfigExpander.expandConfigOptions(
eventHandler,
commandToRcArgs,
commandAnnotation.name(),
getCommandNamesToParse(commandAnnotation),
rcfileNotes::add,
optionsParser);
optionsParser,
OptionsParser.getFallbackOptionsData(allOptionsClasses));
}

private static List<String> getCommandNamesToParse(Command commandAnnotation) {
List<String> result = new ArrayList<>();
result.add("common");
result.add(ALWAYS_PSEUDO_COMMAND);
result.add(COMMON_PSEUDO_COMMAND);
getCommandNamesToParseHelper(commandAnnotation, result);
return result;
}
Expand Down Expand Up @@ -557,7 +601,8 @@ static ListMultimap<String, RcChunkOfArgs> structureRcOptionsAndConfigs(
if (index > 0) {
command = command.substring(0, index);
}
if (!validCommands.contains(command) && !command.equals("common")) {
if (!validCommands.contains(command) && !command.equals(ALWAYS_PSEUDO_COMMAND)
&& !command.equals(COMMON_PSEUDO_COMMAND)) {
eventHandler.handle(
Event.warn(
"while reading option defaults file '"
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,7 +1113,8 @@ private static OptionsParsingResult parseStartupOptions(

// Then parse the command line again, this time with the correct option sources
parser = OptionsParser.builder().optionsClasses(optionClasses).allowResidue(false).build();
parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args);
parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction,
args, /* fallbackData= */null);
return parser;
}

Expand Down
19 changes: 15 additions & 4 deletions src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.common.options.OpaqueOptionsData;
import com.google.devtools.common.options.OptionValueDescription;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
Expand Down Expand Up @@ -86,9 +88,11 @@ private static boolean shouldEnablePlatformSpecificConfig(
static void expandConfigOptions(
EventHandler eventHandler,
ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
String currentCommand,
List<String> commandsToParse,
Consumer<String> rcFileNotesConsumer,
OptionsParser optionsParser)
OptionsParser optionsParser,
OpaqueOptionsData fallbackData)
throws OptionsParsingException {

OptionValueDescription configValueDescription =
Expand All @@ -110,10 +114,16 @@ static void expandConfigOptions(
commandsToParse,
configValueToExpand,
rcFileNotesConsumer);
optionsParser.parseArgsAsExpansionOfOption(
var ignoredArgs = optionsParser.parseArgsAsExpansionOfOption(
configInstance,
String.format("expanded from --config=%s", configValueToExpand),
expansion);
expansion,
fallbackData);
if (!ignoredArgs.isEmpty()) {
rcFileNotesConsumer.accept(
String.format("Ignored as unsupported by '%s': %s", currentCommand,
Joiner.on(' ').join(ignoredArgs)));
}
}
}

Expand All @@ -131,7 +141,8 @@ static void expandConfigOptions(
optionsParser.parseArgsAsExpansionOfOption(
Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()),
String.format("enabled by --enable_platform_specific_config"),
expansion);
expansion,
fallbackData);
}

// At this point, we've expanded everything, identify duplicates, if any, to warn about
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/common/options/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ java_library(
],
),
deps = [
"//src/main/java/com/google/devtools/build/lib/util:pair",
"//third_party:auto_value",
"//third_party:caffeine",
"//third_party:guava",
Expand Down
Loading