shell toolchain: genrule now uses it

genrule() now uses the shell toolchain (as well as
the ShellConfiguration config fragment) to look up
the shell interpreter's path.

Also the CommandHelper no longer looks up the
shell interpreter's path itself. Instead its ctor
takes the path as an argument. This allows
supporting rules that already use the shell
toolchain and rules that still only consider the
configuration fragment.

With these changes genrule() is now able to use
the shell interpreter registered as a toolchain,
even if there's no `--shell_executable` flag
specified (or its value is empty).

Subsequent commits will migrate more and more
rules to depend on the shell toolchain.

This commit takes us closer to:

1. be able to detect the local shell interpreter's
   location, especially when it's not the default
   /bin/bash

2. be able to define different shell toolchains
   (different interpreter paths) for remote builds

3. gracefully fail the build if the machine has no
   shell installed but the action graph included a
   shell action

See https://github.com/bazelbuild/bazel/issues/4319

Change-Id: I0674c7e2d5917643d87b48b19a1cb43e606ad2f7

Closes #5282.

Change-Id: I0674c7e2d5917643d87b48b19a1cb43e606ad2f7
PiperOrigin-RevId: 198394021

7 files changed, 223 insertions, 5 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java b/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java
index c785da410e..482ba88380 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java
@@ -14,12 +14,40 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.analysis.ToolchainContext.ResolvedToolchainProviders;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 /** Class to work with the shell toolchain, e.g. get the shell interpreter's path. */
 public final class ShToolchain {
 
+  private static final String TOOLCHAIN_TYPE_ATTR = "$sh_toolchain_type";
+  private static final String TOOLCHAIN_TYPE_LABEL = "//tools/sh:toolchain_type";
+  private static final String TOOLCHAIN_TYPE_PACKAGE = "tools/sh";
+
+  public static RuleClass.Builder addDependency(
+      RuleClass.Builder builder, RuleDefinitionEnvironment env) {
+    return addDependency(builder, env.getToolsLabel(TOOLCHAIN_TYPE_LABEL));
+  }
+
+  private static RuleClass.Builder addDependency(RuleClass.Builder builder, Label toolchainType) {
+    return builder.addRequiredToolchains(toolchainType).add(createAttribute(toolchainType));
+  }
+
+  private static Attribute.Builder createAttribute(Label toolchainType) {
+    return attr(TOOLCHAIN_TYPE_ATTR, LABEL).value(toolchainType);
+  }
+
   /**
    * Returns the shell executable's path, or an empty path if not set.
    *
@@ -43,9 +71,11 @@ public final class ShToolchain {
   /**
    * Returns the shell executable's path, or reports a rule error if the path is empty.
    *
-   * <p>This method checks the rule's configuration's {@link ShellConfiguration} fragment for the
-   * shell executable's path. If null or empty, the method reports an error against the rule.
+   * <p>DO NOT USE THIS METHOD, use {@link #getToolchainPathOrError} instead. This method exists to
+   * allow incremental migration to {@link #getToolchainPathOrError}.
    */
+  // TODO(laszlocsomor): update every rule that calls getPathOrError to depend on the shell
+  // toolchain, and change their use of getPathOrError to getToolchainPathOrError.
   public static PathFragment getPathOrError(RuleContext ctx) {
     PathFragment result = getPath(ctx.getConfiguration());
 
@@ -58,5 +88,61 @@ public final class ShToolchain {
     return result;
   }
 
+  /**
+   * Returns the shell executable's path, or reports a rule error if the path is empty.
+   *
+   * <p>This method checks the rule's configuration's {@link ShellConfiguration} fragment for the
+   * shell executable's path. If null or empty, this method gets the path from the selected shell
+   * toolchain. If the path is still null or empty, the method reports an error against the rule.
+   *
+   * @throws IllegalArgumentException if the rule does not depend on the shell toolchain
+   */
+  public static PathFragment getToolchainPathOrError(RuleContext ctx) {
+    // TODO(laszlocsomor): update every rule that calls getPathOrError to depend on the shell
+    // toolchain, and change their use of getPathOrError to getToolchainPathOrError.
+    Preconditions.checkState(ctx.attributes().has(TOOLCHAIN_TYPE_ATTR, LABEL));
+
+    PathFragment result = getPath(ctx.getConfiguration());
+
+    if (result.isEmpty()) {
+      ResolvedToolchainProviders toolchains =
+          (ResolvedToolchainProviders) ctx.getToolchainContext().getResolvedToolchainProviders();
+
+      ToolchainInfo activeToolchain =
+          toolchains.getForToolchainType(ctx.attributes().get(TOOLCHAIN_TYPE_ATTR, LABEL));
+
+      if (activeToolchain != null) {
+        String path = null;
+        try {
+          path = (String) activeToolchain.getValue("path");
+        } catch (EvalException e) {
+          throw new IllegalStateException(e);
+        }
+
+        if (path != null && !path.isEmpty()) {
+          result = PathFragment.create(path);
+        }
+      }
+    }
+
+    if (result.isEmpty()) {
+      ctx.ruleError(
+          "This rule needs a shell interpreter. Use the --shell_executable=<path> flag to specify"
+              + " the interpreter's path, e.g. --shell_executable=/usr/local/bin/bash");
+    }
+
+    return result;
+  }
+
   private ShToolchain() {}
+
+  @VisibleForTesting
+  public static String getToolchainTypeLabelForTesting() {
+    return TOOLCHAIN_TYPE_LABEL;
+  }
+
+  @VisibleForTesting
+  public static String getToolchainTypePackageForTesting() {
+    return TOOLCHAIN_TYPE_PACKAGE;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java
index 964cfafce1..bda085ec41 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java
@@ -20,6 +20,7 @@ import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
 
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
+import com.google.devtools.build.lib.analysis.ShToolchain;
 import com.google.devtools.build.lib.analysis.config.HostTransition;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.rules.cpp.CcToolchain;
@@ -44,7 +45,7 @@ public final class BazelGenRuleRule implements RuleDefinition {
     rules. If the rule generates source files, you should use the
     <code>srcs</code> attribute.
     <!-- #END_BLAZE_RULE.NAME --> */
-    return builder
+    return ShToolchain.addDependency(builder, env)
         .setOutputToGenfiles()
         .add(
             attr("$genrule_setup", LABEL)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
index 52efdf15de..ab57723303 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
@@ -196,7 +196,7 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
     FilesToRunProvider genruleSetup =
         ruleContext.getPrerequisite("$genrule_setup", Mode.HOST, FilesToRunProvider.class);
     inputs.addTransitive(genruleSetup.getFilesToRun());
-    PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
+    PathFragment shExecutable = ShToolchain.getToolchainPathOrError(ruleContext);
     if (ruleContext.hasErrors()) {
       return null;
     }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index af2220d0f6..d26727a650 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -61,7 +61,8 @@ public final class BazelAnalysisMock extends AnalysisMock {
             "local_repository(name = 'com_google_protobuf', path = '/protobuf')",
             "bind(name = 'android/sdk', actual='@bazel_tools//tools/android:sdk')",
             "bind(name = 'tools/python', actual='//tools/python')",
-            "register_toolchains('@bazel_tools//tools/cpp:all')"));
+            "register_toolchains('@bazel_tools//tools/cpp:all')",
+            "register_toolchains('@bazel_tools//tools/sh:toolchain')"));
   }
 
   @Override
@@ -190,6 +191,31 @@ public final class BazelAnalysisMock extends AnalysisMock {
     config.create("/bazel_tools_workspace/objcproto/empty.cc");
     config.create("/bazel_tools_workspace/objcproto/well_known_type.proto");
 
+    config.create(
+        "/bazel_tools_workspace/tools/sh/BUILD",
+        "package(default_visibility = ['//visibility:public'])",
+        "load(':sh_toolchain.bzl', 'sh_toolchain')",
+        "",
+        "toolchain_type(name = 'toolchain_type')",
+        "sh_toolchain(",
+        "    name = 'dummy_toolchain',",
+        "    path = '/mock/shell/toolchain',",
+        ")",
+        "toolchain(",
+        "    name = 'toolchain',",
+        "    toolchain = ':dummy_toolchain',",
+        "    toolchain_type = ':toolchain_type',",
+        ")");
+    config.create(
+        "/bazel_tools_workspace/tools/sh/sh_toolchain.bzl",
+        "def _sh_toolchain_impl(ctx):",
+        "  return [platform_common.ToolchainInfo(path = ctx.attr.path)]",
+        "",
+        "sh_toolchain = rule(",
+        "    attrs = {'path': attr.string()},",
+        "    implementation = _sh_toolchain_impl,",
+        ")");
+
     ccSupport().setup(config);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
index 7ced520c12..267fedbac2 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.ShToolchain;
 import com.google.devtools.build.lib.analysis.ShellConfiguration;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget;
@@ -475,6 +476,10 @@ public class GenRuleConfiguredTargetTest extends BuildViewTestCase {
     boolean foundTool = false;
     boolean foundSetup = false;
     for (ConfiguredTarget prereq : prereqs) {
+      String label = prereq.getLabel().toString();
+      if (label.endsWith(ShToolchain.getToolchainTypeLabelForTesting())) {
+        continue;
+      }
       String name = prereq.getLabel().getName();
       if (name.contains("cc-") || name.contains("jdk")) {
           // Ignore these, they are present due to the implied genrule dependency on crosstool and
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleUsesShellToolchainTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleUsesShellToolchainTest.java
new file mode 100644
index 0000000000..9783aad9d7
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleUsesShellToolchainTest.java
@@ -0,0 +1,96 @@
+// Copyright 2018 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.genrule;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.TestConstants.GENRULE_SETUP;
+
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Assert that genrule uses the shell toolchain. */
+@RunWith(JUnit4.class)
+public class GenRuleUsesShellToolchainTest extends BuildViewTestCase {
+
+  private static final Pattern SETUP_COMMAND_PATTERN =
+      Pattern.compile(".*/genrule-setup.sh;\\s+(?<command>.*)");
+
+  @Test
+  public void testActionIsShellCommandUsingShellFromShellConfig() throws Exception {
+    useConfiguration("--shell_executable=/custom/shell/executable");
+    assertActionIsShellCommand("/custom/shell/executable");
+  }
+
+  @Test
+  public void testActionIsShellCommandUsingShellFromShellToolchain() throws Exception {
+    useConfiguration("--shell_executable=");
+    assertActionIsShellCommand("/mock/shell/toolchain");
+  }
+
+  @Test
+  public void testActionIsShellCommandUsingShellFromExtraToolchain() throws Exception {
+    useConfiguration("--shell_executable=", "--extra_toolchains=//pkg1:tc");
+    scratch.file(
+        "pkg1/BUILD",
+        "load('@bazel_tools//tools/sh:sh_toolchain.bzl', 'sh_toolchain')",
+        "",
+        "sh_toolchain(",
+        "    name = 'tc_def',",
+        "    path = '/foo/bar/baz',",
+        "    visibility = ['//visibility:public'],",
+        ")",
+        "",
+        "toolchain(",
+        "    name = 'tc',",
+        "    toolchain = 'tc_def',",
+        "    toolchain_type = '@bazel_tools//tools/sh:toolchain_type',",
+        ")");
+
+    assertActionIsShellCommand("/foo/bar/baz");
+  }
+
+  private void assertActionIsShellCommand(String expectedShell) throws Exception {
+    scratch.file(
+        "genrule1/BUILD",
+        "genrule(",
+        "    name = 'genrule1',",
+        "    srcs = ['input.txt'],",
+        "    outs = ['output.txt'],",
+        "    cmd  = 'dummy >$@',",
+        ")");
+
+    Artifact input = getFileConfiguredTarget("//genrule1:input.txt").getArtifact();
+    Artifact genruleSetup = getFileConfiguredTarget(GENRULE_SETUP).getArtifact();
+
+    Artifact out = getFileConfiguredTarget("//genrule1:output.txt").getArtifact();
+    SpawnAction shellAction = (SpawnAction) getGeneratingAction(out);
+    assertThat(shellAction).isNotNull();
+    assertThat(shellAction.getInputs()).containsExactly(input, genruleSetup);
+    assertThat(shellAction.getOutputs()).containsExactly(out);
+
+    String expected = "dummy >" + out.getExecPathString();
+    assertThat(shellAction.getArguments().get(0)).isEqualTo(expectedShell);
+    assertThat(shellAction.getArguments().get(1)).isEqualTo("-c");
+    Matcher m = SETUP_COMMAND_PATTERN.matcher(shellAction.getArguments().get(2));
+    assertThat(m.matches()).isTrue();
+    assertThat(m.group("command")).isEqualTo(expected);
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
index f94c047667..74f614599b 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider;
 import com.google.devtools.build.lib.bazel.rules.CcRules;
 import com.google.devtools.build.lib.bazel.rules.GenericRules;
+import com.google.devtools.build.lib.bazel.rules.ToolchainRules;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -999,6 +1000,9 @@ public class CcCommonTest extends BuildViewTestCase {
           PlatformRules.INSTANCE.init(builder);
           GenericRules.INSTANCE.init(builder);
           CcRules.INSTANCE.init(builder);
+          // Some tests use genrules so they need the shell toolchain (//tools/sh:toolchain_type) so
+          // we need to support the toolchain rules.
+          ToolchainRules.INSTANCE.init(builder);
           return builder.build();
         }