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.

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

bazelrc: support --enable_platform_specific_config #9246

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
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.events.ExtendedEventHandler;
import com.google.devtools.build.lib.runtime.commands.ProjectFileSupport;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.InvocationPolicyEnforcer;
Expand Down Expand Up @@ -315,6 +317,27 @@ ExitCode parseOptions(List<String> args, ExtendedEventHandler eventHandler) {
return ExitCode.SUCCESS;
}

/**
* If --enable_platform_specific_config is true and the corresponding config definition exists,
* we should enable the platform specific config.
*/
private boolean shouldEnablePlatformSpecificConfig(
OptionValueDescription enablePlatformSpecificConfigDescription,
ListMultimap<String, RcChunkOfArgs> commandToRcArgs) {
if (enablePlatformSpecificConfigDescription == null ||
!(boolean) enablePlatformSpecificConfigDescription.getValue()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this cast safe? Shouldn't it be (Boolean) and check for null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as long as there is a default value, it won't be null and will be converted to the correct type.
See

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

return false;
}

for (String commandName : getCommandNamesToParse(commandAnnotation)) {
String defaultConfigDef = commandName + ":" + OS.getCurrent().getCanonicalName();
if (commandToRcArgs.containsKey(defaultConfigDef)) {
return true;
}
}
return false;
}

/**
* Expand the values of --config according to the definitions provided in the rc files and the
* applicable command.
Expand All @@ -325,23 +348,30 @@ void expandConfigOptions(

OptionValueDescription configValueDescription =
optionsParser.getOptionValueDescription("config");
if (configValueDescription == null || configValueDescription.getCanonicalInstances() == null) {
// No --config values were set, we can avoid this whole thing.
return;
if (configValueDescription != null && configValueDescription.getCanonicalInstances() != null) {
// Find the base set of configs. This does not include the config options that might be
// recursively included.
ImmutableList<ParsedOptionDescription> configInstances =
ImmutableList.copyOf(configValueDescription.getCanonicalInstances());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make a copy?

(I realize this is old code, just asking.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I actually don't know the reason..


// Expand the configs that are mentioned in the input. Flatten these expansions before parsing
// them, to preserve order.
for (ParsedOptionDescription configInstance : configInstances) {
String configValueToExpand = (String) configInstance.getConvertedValue();
List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand);
optionsParser.parseArgsAsExpansionOfOption(
configInstance, String.format("expanded from --%s", configValueToExpand), expansion);
}
}

// Find the base set of configs. This does not include the config options that might be
// recursively incuded.
ImmutableList<ParsedOptionDescription> configInstances =
ImmutableList.copyOf(configValueDescription.getCanonicalInstances());

// Expand the configs that are mentioned in the input. Flatten these expansions before parsing
// them, to preserve order.
for (ParsedOptionDescription configInstance : configInstances) {
String configValueToExpand = (String) configInstance.getConvertedValue();
List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand);
OptionValueDescription enablePlatformSpecificConfigDescription =
optionsParser.getOptionValueDescription("enable_platform_specific_config");
if (shouldEnablePlatformSpecificConfig(enablePlatformSpecificConfigDescription, commandToRcArgs)) {
List<String> expansion = getExpansion(eventHandler, commandToRcArgs, OS.getCurrent().getCanonicalName());
optionsParser.parseArgsAsExpansionOfOption(
configInstance, String.format("expanded from --%s", configValueToExpand), expansion);
Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()),
String.format("enabled by --enable_platform_specific_config"),
expansion);
}

// At this point, we've expanded everything, identify duplicates, if any, to warn about
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,20 @@ public class CommonCommandOptions extends OptionsBase {
+ "your build may break in the future due to deprecations or other changes.")
public Void allIncompatibleChanges;

@Option(
name = "enable_platform_specific_config",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
"If true, Bazel picks up host-OS-specific config lines from bazelrc files. For example, "
+ "if the host OS is Linux and you run bazel build, Bazel picks up lines starting "
+ "with build:linux. Supported OS identifiers are linux, osx, windows, and freebsd. "
+ "Enabling this flag is equivalent to using --config=linux on Linux, "
+ "--config=windows on Windows, etc."
)
public boolean enablePlatformSpecificConfig;

@Option(
name = "config",
defaultValue = "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.util.OS;
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 @@ -155,6 +156,16 @@ private ListMultimap<String, RcChunkOfArgs> structuredArgsFromImportedRcsWithOnl
return structuredArgs;
}

private ListMultimap<String, RcChunkOfArgs> structuredArgsForDifferentPlatforms() {
ListMultimap<String, RcChunkOfArgs> structuredArgs = ArrayListMultimap.create();
structuredArgs.put("c0:linux", new RcChunkOfArgs("rc1", ImmutableList.of("command_linux")));
structuredArgs.put("c0:windows", new RcChunkOfArgs("rc1", ImmutableList.of("command_windows")));
structuredArgs.put("c0:osx", new RcChunkOfArgs("rc1", ImmutableList.of("command_osx")));
structuredArgs.put("c0:freebsd", new RcChunkOfArgs("rc1", ImmutableList.of("command_freebsd")));
structuredArgs.put("c0:platform_config", new RcChunkOfArgs("rc1", ImmutableList.of("--enable_platform_specific_config")));
return structuredArgs;
}

@Test
public void testStructureRcOptionsAndConfigs_argumentless() throws Exception {
ListMultimap<String, RcChunkOfArgs> structuredRc =
Expand Down Expand Up @@ -297,6 +308,62 @@ public void testExpandConfigOptions_withConfig() throws Exception {
.containsExactly("Found applicable config definition c0:config in file rc1: b");
}

@Test
public void testExpandConfigOptions_withPlatformSpecificConfigEnabled() throws Exception {
meteorcloudy marked this conversation as resolved.
Show resolved Hide resolved
parser.parse("--enable_platform_specific_config");
optionHandler.expandConfigOptions(eventHandler, structuredArgsForDifferentPlatforms());
switch (OS.getCurrent()) {
case LINUX:
assertThat(parser.getResidue()).containsExactly("command_linux");
break;
case DARWIN:
assertThat(parser.getResidue()).containsExactly("command_osx");
break;
case WINDOWS:
assertThat(parser.getResidue()).containsExactly("command_windows");
break;
case FREEBSD:
assertThat(parser.getResidue()).containsExactly("command_freebsd");
break;
default:
assertThat(parser.getResidue()).isEmpty();
}
}

@Test
public void testExpandConfigOptions_withPlatformSpecificConfigEnabledInConfig() throws Exception {
// --enable_platform_specific_config itself will affect the selecting of config sections.
// Because Bazel expands config sections recursively, we want to make sure it's fine to enable
// --enable_platform_specific_config via another config section.
parser.parse("--config=platform_config");
optionHandler.expandConfigOptions(eventHandler, structuredArgsForDifferentPlatforms());
switch (OS.getCurrent()) {
case LINUX:
assertThat(parser.getResidue()).containsExactly("command_linux");
break;
case DARWIN:
assertThat(parser.getResidue()).containsExactly("command_osx");
break;
case WINDOWS:
assertThat(parser.getResidue()).containsExactly("command_windows");
break;
case FREEBSD:
assertThat(parser.getResidue()).containsExactly("command_freebsd");
break;
default:
assertThat(parser.getResidue()).isEmpty();
}
}

@Test
public void testExpandConfigOptions_withPlatformSpecificConfigEnabledWhenNothingSpecified()
throws Exception {
parser.parse("--enable_platform_specific_config");
optionHandler.parseRcOptions(eventHandler, ArrayListMultimap.create());
assertThat(eventHandler.getEvents()).isEmpty();
assertThat(parser.getResidue()).isEmpty();
}

@Test
public void testExpandConfigOptions_withConfigForUnapplicableCommand() throws Exception {
parser.parse("--config=other");
Expand Down