Add --experimental_strict_action_env and --shell_executable flags

- --experimental_strict_action_env makes Bazel not forward PATH,
  LD_LIBRARY_PATH, and TMPDIR to all actions. This is intended to be a
  transitional flag; as part of rollout, we'll need to update all users that
  rely on the current behavior to specify their needs explicitly (with
  --action_env). But note that action_env is not yet applied to all actions,
  which also needs to be fixed.

- --shell_executable can be used to explicitly set the shell executable to
  use in actions. On Windows, if --experimental_strict_action_env is unset,
  then the PATH is computed to include the path to the shell executable.

Progress on #2574.

PiperOrigin-RevId: 161652996

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.bazel.rules;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -23,7 +25,11 @@
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.proto.OptionFilters.OptionEffectTag;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -32,14 +38,57 @@
  */
 @Immutable
 public class BazelConfiguration extends Fragment {
+  /** Command-line options. */
+  public static class Options extends FragmentOptions {
+    @Option(
+      name = "experimental_strict_action_env",
+      defaultValue = "false",
+      category = "semantics",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      help =
+          "If true, Bazel uses an environment with a static value for PATH, and LANG and does not "
+              + "inherit LD_LIBRARY_PATH or TMPDIR. Use --action_env=ENV_VARIABLE if you want to "
+              + "inherit specific environment variables from the client, but note that doing so "
+              + "can prevent cross-user caching if a shared cache is used."
+    )
+    public boolean useStrictActionEnv;
+
+    @Option(
+      name = "shell_executable",
+      converter = PathFragmentConverter.class,
+      defaultValue = "null",
+      category = "semantics",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      help =
+          "Absolute path to the shell executable for Bazel to use. If this is unset, but the "
+              + "BAZEL_SH environment variable is set on the first Bazel invocation (that starts "
+              + "up a Bazel server), Bazel uses that. If neither is set, Bazel uses a hard-coded "
+              + "default path depending on the operating system it runs on (Windows: "
+              + "c:/tools/msys64/usr/bin/bash.exe, FreeBSD: /usr/local/bin/bash, all others: "
+              + "/bin/bash). Note that using a shell that is not compatible with bash may lead to "
+              + "build failures or runtime failures of the generated binaries."
+    )
+    public PathFragment shellExecutable;
+
+    @Override
+    public Options getHost(boolean fallback) {
+      Options host = (Options) getDefault();
+      host.useStrictActionEnv = useStrictActionEnv;
+      host.shellExecutable = shellExecutable;
+      return host;
+    }
+  }
+
   /**
    * Loader for Bazel-specific settings.
    */
   public static class Loader implements ConfigurationFragmentFactory {
     @Override
     public Fragment create(ConfigurationEnvironment env, BuildOptions buildOptions)
         throws InvalidConfigurationException {
-      return new BazelConfiguration();
+      return new BazelConfiguration(OS.getCurrent(), buildOptions.get(Options.class));
     }
 
     @Override
@@ -49,59 +98,81 @@ public Class<? extends Fragment> creates() {
 
     @Override
     public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
-      return ImmutableSet.of();
+      return ImmutableSet.of(Options.class);
     }
   }
 
-  public BazelConfiguration() {
+  private static final ImmutableMap<OS, PathFragment> OS_SPECIFIC_SHELL =
+      ImmutableMap.<OS, PathFragment>builder()
+          .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe"))
+          .put(OS.FREEBSD, PathFragment.create("/usr/local/bin/bash"))
+          .build();
+  private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash");
+
+  private final OS os;
+  private final boolean useStrictActionEnv;
+  private final PathFragment shellExecutable;
+
+  public BazelConfiguration(OS os, Options options) {
+    this.os = os;
+    this.useStrictActionEnv = options.useStrictActionEnv;
+    this.shellExecutable = determineShellExecutable(os, options.shellExecutable);
   }
 
   @Override
   public PathFragment getShellExecutable() {
-    if (OS.getCurrent() == OS.WINDOWS) {
-      String path = System.getenv("BAZEL_SH");
-      if (path != null) {
-        return PathFragment.create(path);
-      } else {
-        return PathFragment.create("c:/tools/msys64/usr/bin/bash.exe");
-      }
-    }
-    if (OS.getCurrent() == OS.FREEBSD) {
-      String path = System.getenv("BAZEL_SH");
-      if (path != null) {
-        return  PathFragment.create(path);
-      } else {
-        return PathFragment.create("/usr/local/bin/bash");
-      }
-    }
-    return PathFragment.create("/bin/bash");
+    return shellExecutable;
   }
 
   @Override
   public void setupActionEnvironment(Map<String, String> builder) {
-    // TODO(ulfjack): Avoid using System.getenv; it's the wrong environment!
-    builder.put("PATH", pathOrDefault(System.getenv("PATH"), getShellExecutable()));
+    if (useStrictActionEnv) {
+      String path = pathOrDefault(os, null, getShellExecutable());
+      builder.put("PATH", path);
+    } else {
+      // TODO(ulfjack): Avoid using System.getenv; it's the wrong environment!
+      builder.put("PATH", pathOrDefault(os, System.getenv("PATH"), getShellExecutable()));
 
-    String ldLibraryPath = System.getenv("LD_LIBRARY_PATH");
-    if (ldLibraryPath != null) {
-      builder.put("LD_LIBRARY_PATH", ldLibraryPath);
+      String ldLibraryPath = System.getenv("LD_LIBRARY_PATH");
+      if (ldLibraryPath != null) {
+        builder.put("LD_LIBRARY_PATH", ldLibraryPath);
+      }
+
+      String tmpdir = System.getenv("TMPDIR");
+      if (tmpdir != null) {
+        builder.put("TMPDIR", tmpdir);
+      }
     }
+  }
 
-    String tmpdir = System.getenv("TMPDIR");
-    if (tmpdir != null) {
-      builder.put("TMPDIR", tmpdir);
+  private static PathFragment determineShellExecutable(OS os, PathFragment fromOption) {
+    if (fromOption != null) {
+      return fromOption;
+    }
+    // Honor BAZEL_SH env variable for backwards compatibility.
+    String path = System.getenv("BAZEL_SH");
+    if (path != null) {
+      return PathFragment.create(path);
     }
+    // TODO(ulfjack): instead of using the OS Bazel runs on, we need to use the exec platform, which
+    // may be different for remote execution. For now, this can be overridden with
+    // --shell_executable, so at least there's a workaround.
+    PathFragment result = OS_SPECIFIC_SHELL.get(os);
+    return result != null ? result : FALLBACK_SHELL;
   }
 
-  private static String pathOrDefault(@Nullable String path, @Nullable PathFragment sh) {
-    if (OS.getCurrent() != OS.WINDOWS) {
+  @VisibleForTesting
+  static String pathOrDefault(OS os, @Nullable String path, @Nullable PathFragment sh) {
+    // TODO(ulfjack): The default PATH should be set from the exec platform, which may be different
+    // from the local machine. For now, this can be overridden with --action_env=PATH=<value>, so
+    // at least there's a workaround.
+    if (os != OS.WINDOWS) {
       return path == null ? "/bin:/usr/bin" : path;
     }
 
     // Attempt to compute the MSYS root (the real Windows path of "/") from `sh`.
-    String newPath = "";
     if (sh != null && sh.getParentDirectory() != null) {
-      newPath = sh.getParentDirectory().getPathString();
+      String newPath = sh.getParentDirectory().getPathString();
       if (sh.getParentDirectory().endsWith(PathFragment.create("usr/bin"))) {
         newPath +=
             ";" + sh.getParentDirectory().getParentDirectory().replaceName("bin").getPathString();
@@ -114,7 +185,11 @@ private static String pathOrDefault(@Nullable String path, @Nullable PathFragmen
       if (path != null) {
         newPath += ";" + path;
       }
+      return newPath;
+    } else if (path != null) {
+      return path;
+    } else {
+      return "";
     }
-    return newPath;
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

@@ -218,6 +218,7 @@ public void init(Builder builder) {
 
           builder.setUniversalConfigurationFragment(BazelConfiguration.class);
           builder.addConfigurationFragment(new BazelConfiguration.Loader());
+          builder.addConfigurationOptions(BazelConfiguration.Options.class);
           builder.addConfigurationOptions(BuildConfiguration.Options.class);
         }
 

src/test/java/com/google/devtools/build/lib/BUILD

@@ -1017,6 +1017,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",
+        "//src/main/java/com/google/devtools/common/options",
         "//src/main/protobuf:crosstool_config_java_proto",
         "//src/test/java/com/google/devtools/build/lib:actions_testutil",
         "//src/test/java/com/google/devtools/build/lib:packages_testutil",

src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java

@@ -0,0 +1,91 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.bazel.rules;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.bazel.rules.BazelConfiguration.pathOrDefault;
+
+import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.common.options.Options;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for {@link BazelConfiguration}.
+ */
+@RunWith(JUnit4.class)
+public class BazelConfigurationTest {
+  @Test
+  public void getShellExecutableUnset() {
+    BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class);
+    assertThat(new BazelConfiguration(OS.LINUX, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("/bin/bash"));
+    assertThat(new BazelConfiguration(OS.FREEBSD, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("/usr/local/bin/bash"));
+    assertThat(new BazelConfiguration(OS.WINDOWS, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("c:/tools/msys64/usr/bin/bash.exe"));
+  }
+
+  @Test
+  public void getShellExecutableIfSet() {
+    BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class);
+    o.shellExecutable = PathFragment.create("/bin/bash");
+    assertThat(new BazelConfiguration(OS.LINUX, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("/bin/bash"));
+    assertThat(new BazelConfiguration(OS.FREEBSD, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("/bin/bash"));
+    assertThat(new BazelConfiguration(OS.WINDOWS, o).getShellExecutable())
+        .isEqualTo(PathFragment.create("/bin/bash"));
+  }
+
+  @Test
+  public void strictActionEnv() {
+    BazelConfiguration.Options options = Options.getDefaults(BazelConfiguration.Options.class);
+    options.useStrictActionEnv = true;
+    BazelConfiguration config = new BazelConfiguration(OS.LINUX, options);
+    Map<String, String> env = new HashMap<>();
+    config.setupActionEnvironment(env);
+    assertThat(env).containsEntry("PATH", "/bin:/usr/bin");
+  }
+
+  @Test
+  public void pathOrDefaultOnLinux() {
+    assertThat(pathOrDefault(OS.LINUX, null, null)).isEqualTo("/bin:/usr/bin");
+    assertThat(pathOrDefault(OS.LINUX, "/not/bin", null)).isEqualTo("/not/bin");
+  }
+
+  @Test
+  public void pathOrDefaultOnWindows() {
+    assertThat(pathOrDefault(OS.WINDOWS, null, null)).isEqualTo("");
+    assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", null))
+        .isEqualTo("C:/mypath");
+    assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", PathFragment.create("D:/foo/shell")))
+        .isEqualTo("D:\\foo;C:/mypath");
+  }
+
+  @Test
+  public void optionsAlsoApplyToHost() {
+    BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class);
+    o.shellExecutable = PathFragment.create("/my/shell/binary");
+    o.useStrictActionEnv = true;
+    BazelConfiguration.Options h = o.getHost(false);
+    assertThat(h.shellExecutable).isEqualTo(PathFragment.create("/my/shell/binary"));
+    assertThat(h.useStrictActionEnv).isTrue();
+  }
+}