Repository: bazelbuild/bazel
Issue: bazelrc: multiplatform support
Labels: type: feature request
P2
team-OSS

### Description of the problem / feature request:

I'd like to have some mechanism to use platform-specific flags in bazelrc files.

### Feature requests: what underlying problem are you trying to solve with this feature?

Motivation is to let checked-in, project-specific bazelrc files work on all platforms. Flags dealing with paths (e.g. `--output_user_root`), envvars (e.g. `--test_env`), and external programs or shells (e.g. `--workspace_status_command`) are all platform-specific.

Issue comments:
We've talked about different solutions to this problem in the past, but the easiest solution is to accept multiple rc files (#4502), which will let users keep platform specific flags in the global rc while keeping project specific flags in a workspace rc file. The workspace one can be shared between different platforms. 

Other solutions we've considered include checking for a "\<platform\>.bazelrc" filename, or making the rc file syntax more complicated so it can include some flags on some platforms and not others. Neither of these options seem to solve a problem that #4502 doesn't already address, though, and 4502 has other reasons for needing to happen. 

Thoughts?
The general platform issue for bazel flags is very much not solved, the solution I mentioned leaves us in basically the following state:
- host platform: goes in global bazelrc
- execution platform: if host, in global bazelrc, if remote, likely in the workspace file. 
- target platform: the workspace rc file can contain --config definitions that correspond to the different configurations needed.

I realize this is not ideal, but the "right" solution is not clear to me. The fixes in the other issue will only get us out of the unusable into the "works, I guess" territory.
+1 to Chloe’s description
On Thu, 19 Apr 2018 at 21:03 Chloe Calvarin <notifications@github.com>
wrote:

> The general platform issue for bazel flags is very much not solved, the
> solution I mentioned leaves us in basically the following state:
>
>    - host platform: goes in global bazelrc
>    - execution platform: if host, in global bazelrc, if remote, likely in
>    the workspace file.
>    - target platform: the workspace rc file can contain --config
>    definitions that correspond to the different configurations needed.
>
> I realize this is not ideal, but the "right" solution is not clear to me.
> The fixes in the other issue will only get us out of the unusable into the
> "works, I guess" territory.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/bazelbuild/bazel/issues/5055#issuecomment-382828169>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABUIF6stjy7TekvRVqt6abeEuuYw3fqEks5tqNF1gaJpZM4TbuWV>
> .
>

> Thoughts?

I don't really have any, you're the domain expert. :)

> The fixes in the other issue will only get us out of the unusable into the "works, I guess" territory.

SGTM.
> ... and external programs or shells (e.g. --workspace_status_command) are all platform-specific

This is indeed a serious problem. Gerrit Code Review has this content of `tools/bazel.rc` (committed in the repository):

```
build --workspace_status_command=./tools/workspace-status.sh
```

Stamping machinery shells out to `cmd.exe` on Windows, so that we have to provide a different script: `bazel build --workspace_status_command=./tools/workspace-status.cmd ...`. There is currently no way to specify different option, except to overwrite it on bazel invocation.

One way to fix it is what Buck is doing in `genrule` by offering [`cmd_exe`: platform specific attributes/options](https://buckbuild.com/rule/genrule.html#cmd_exe). So that at least for status command it could be fixed by adding platform specific option, Bazel detects the current platform and uses the rights script:

```
build --workspace_status_command=./tools/workspace-status.sh --workspace_status_command_exe=./tools/workspace-status.cmd
```
> Other solutions we've considered include checking for a "<platform>.bazelrc" filename, or making the rc file syntax more complicated so it can include some flags on some platforms and not others. Neither of these options seem to solve a problem that #4502 doesn't already address, though,

Although https://github.com/bazelbuild/bazel/issues/4502 is closed, we still don't have a solution to add platform specific flags without passing different build options on different platforms (such as `--config=windows`, `--config=macos`). I think we still need to come up with a solution for this.
Although this issue is closed, we cannot use **cutom** platform specific_config setting like 

```sh
test:@local_config_platform//:host --test_env=LD_LIBRARY_PATH=/gw_demo/target/xlab/sysroot/lib:$LD_LIBRARY_PATH
```

Correct me if I am wrong. @meteorcloudy 
 I think we still need to come up with a solution for this.
If `@local_config_platform//:host` always means whatever host platform you are current on, why not just `test <flags>` so that it always applys?
> If `@local_config_platform//:host` always means whatever host platform you are current on, why not just `test <flags>` so that it always applys?

Oh, I mean the actual platform specific config, is that I can do the following 

```sh
build:@tools/platforms//:platform1  ...blablabla configs
build:@tools/platforms//:platform2  ...blablabla configs
test:@tools/platforms//:platform1  ...blablabla configs
test:@tools/platforms//:platform2  ...blablabla configs
```

Pull request review comments:
Is this cast safe? Shouldn't it be `(Boolean)` and check for null?
If you inlined this, you wouldn't need to import Multiset.
Also, call `containsKey` instead of `keys().contains` -- it's possible that the latter constructs a set and the former doesn't.
Why make a copy?

(I realize this is old code, just asking.)
The new test methods are only subtly different, I find it difficult to understand what each of them tests.
Could you add short descriptions to explain what a test method does and how it's different from the related test methods? (E.g. one uses `--enable_platform_specific_config` therefore X, the other one uses `--config=platform_config` therefore Y.)
> When the value of this flag is true

Could be rewritten as "If true".

> ... a host platform specific config section will be enabled if it exists

I find that confusing. Using active voice and straight word order feels clearer to me. How about: "... Bazel picks up host-OS-specific config lines from the bazelrc files."

I feel like an example would greatly help too: "For example, if the host OS is Linux and you run bazel build, Bazel would pick up lines starting with build:linux"

> linux, osx, windows

macOS is the official name so let's use "macos" instead of "osx".

> It's equivalent to add --config=linux on Linux platforms and --config=windows on Windows, etc.

We could be a little clearer here: "Enabling this flag is equivalent to using --config=linux on Linux, --config=windows on Windows, etc."

Putting it together:

> 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, macos, windows, and freebsd. Enabling this flag is equivalent to using --config=linux on Linux, --config=windows on Windows, etc.
Wow, thanks for helping with the documentation. This is great improvement!

As for the name for mac, it seems we still use `osx` in a lot of places in our code base. For example, https://github.com/bazelbuild/bazel/blob/71a213c42a4572f64b323b3db731e1334b6a0c38/tools/platforms/BUILD#L63

It prefer to keep the consistence util we decided to migrate everything to `macos`. WDYT?
Done.
Sorry, I actually don't know the reason.. 
Nice, thanks for the suggestion!
Yes, as long as there is a default value, it won't be null and will be converted to the correct type. 
See https://github.com/bazelbuild/bazel/blob/1f684e1b87cd8881a0a4b33e86ba66743e32d674/src/main/java/com/google/devtools/common/options/OptionDefinition.java#L255
Thanks!
Thanks!
Could you please ask the Apple team what's their preference?

Patch that resolved the issue:
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..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
@@ -18,6 +18,7 @@
 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;
@@ -25,6 +26,7 @@
 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 +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()) {
+      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.
@@ -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());
+
+      // 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
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..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
@@ -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 = "",


Test patch:
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..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
@@ -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<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 =
@@ -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 {
+    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");