From eb49c623d6e553082a6dad9d87cadb55b0cf3fa4 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Mon, 26 Aug 2019 14:57:45 +0200 Subject: [PATCH 1/2] bazelrc: support --enable_platform_specific_config When the value of this flag is true, a host platform specific config section will be enabled if it exists. Bazel recognizes your host platform as linux, osx, windows or freebsd. It's equivalent to add --config=linux on Linux platforms and --config=windows on Windows, etc. Fixes https://github.com/bazelbuild/bazel/issues/5055 RELNOTES[NEW]: Support --enable_platform_specific_config, you can use this flag to enable flags in bazelrc according to your host platform. GOOGLE: --- .../build/lib/runtime/BlazeOptionHandler.java | 60 +++++++++++++---- .../lib/runtime/CommonCommandOptions.java | 13 ++++ .../lib/runtime/BlazeOptionHandlerTest.java | 64 +++++++++++++++++++ 3 files changed, 123 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index 9944a75537cb43..a0386e20e3de7f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java @@ -18,13 +18,16 @@ 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.common.collect.Multiset; 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; @@ -315,6 +318,28 @@ ExitCode parseOptions(List 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 commandToRcArgs) { + if (enablePlatformSpecificConfigDescription == null || + !(boolean) enablePlatformSpecificConfigDescription.getValue()) { + return false; + } + + Multiset keys = commandToRcArgs.keys(); + for (String commandName : getCommandNamesToParse(commandAnnotation)) { + String defaultConfigDef = commandName + ":" + OS.getCurrent().getCanonicalName(); + if (keys.contains(defaultConfigDef)) { + return true; + } + } + return false; + } + /** * Expand the values of --config according to the definitions provided in the rc files and the * applicable command. @@ -325,23 +350,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 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 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 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 expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand); + OptionValueDescription enablePlatformSpecificConfigDescription = + optionsParser.getOptionValueDescription("enable_platform_specific_config"); + if (shouldEnablePlatformSpecificConfig(enablePlatformSpecificConfigDescription, commandToRcArgs)) { + List 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 diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index 1ce51636186c64..c55e9c9766b5d3 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -56,6 +56,19 @@ 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 = + "When the value of this flag is true, a host platform specific config section will be " + + "enabled if it exists. Bazel recognizes your host platform as linux, osx, windows " + + "or freebsd. It's equivalent to add --config=linux on Linux platforms and " + + "--config=windows on Windows, etc." + ) + public boolean enablePlatformSpecificConfig; + @Option( name = "config", defaultValue = "", diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java index 21726009d0e842..3ad891a25c32c5 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java @@ -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; @@ -155,6 +156,16 @@ private ListMultimap structuredArgsFromImportedRcsWithOnl return structuredArgs; } + private ListMultimap structuredArgsForDifferentPlatforms() { + ListMultimap 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 structuredRc = @@ -297,6 +308,59 @@ public void testExpandConfigOptions_withConfig() throws Exception { .containsExactly("Found applicable config definition c0:config in file rc1: b"); } + @Test + public void testExpandConfigOptions_withPlatformSpecificConfigEnabled() throws Exception { + 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 { + 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"); From 65f0aff92a18f521a618b6b2111b983ae34ec758 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Mon, 26 Aug 2019 16:44:28 +0200 Subject: [PATCH 2/2] Address reviewer comments --- .../devtools/build/lib/runtime/BlazeOptionHandler.java | 4 +--- .../devtools/build/lib/runtime/CommonCommandOptions.java | 7 ++++--- .../devtools/build/lib/runtime/BlazeOptionHandlerTest.java | 3 +++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index a0386e20e3de7f..7ec11cfde637c3 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; -import com.google.common.collect.Multiset; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -330,10 +329,9 @@ private boolean shouldEnablePlatformSpecificConfig( return false; } - Multiset keys = commandToRcArgs.keys(); for (String commandName : getCommandNamesToParse(commandAnnotation)) { String defaultConfigDef = commandName + ":" + OS.getCurrent().getCanonicalName(); - if (keys.contains(defaultConfigDef)) { + if (commandToRcArgs.containsKey(defaultConfigDef)) { return true; } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index c55e9c9766b5d3..772204c57eb08c 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -62,9 +62,10 @@ public class CommonCommandOptions extends OptionsBase { documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.UNKNOWN}, help = - "When the value of this flag is true, a host platform specific config section will be " - + "enabled if it exists. Bazel recognizes your host platform as linux, osx, windows " - + "or freebsd. It's equivalent to add --config=linux on Linux platforms and " + "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; diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java index 3ad891a25c32c5..f6e434d8e018b5 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java @@ -332,6 +332,9 @@ public void testExpandConfigOptions_withPlatformSpecificConfigEnabled() throws E @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()) {