Bazel sh_* rules are documented and cleared up.

 - Unused attribute bash_version is removed from all sh_* rules.
 - ShTest.java is deleted. Shell binary and test rules have the same implementation.

--
MOS_MIGRATED_REVID=88434794

6 files changed, 52 insertions, 129 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShBinaryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShBinaryRule.java
index 39d4a0c15c..9a2aafdcc7 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShBinaryRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShBinaryRule.java
@@ -13,9 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.bazel.rules.sh;
 
-import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.Type.STRING;
-
 import com.google.devtools.build.lib.analysis.BlazeRule;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
@@ -33,9 +30,6 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder;
 public final class BazelShBinaryRule implements RuleDefinition {
   @Override
   public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
-    return builder.add(
-        attr("bash_version", STRING)
-        .value(BazelShRuleClasses.DEFAULT_BASH_VERSION)
-        .allowedValues(BazelShRuleClasses.BASH_VERSION_ALLOWED_VALUES)).build();
+    return builder.build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShLibraryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShLibraryRule.java
index 9d9640b7b3..552d99aaef 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShLibraryRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShLibraryRule.java
@@ -33,29 +33,16 @@ public final class BazelShLibraryRule implements RuleDefinition {
   @Override
   public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
     return builder
-        /* <!-- #BLAZE_RULE(sh_library).ATTRIBUTE(deps) -->
-        The list of other targets to be aggregated in to this "library" target.
-        <i>(List of <a href="build-ref.html#labels">labels</a>; optional)</i><br/>
-        See general comments about <code>deps</code>
-        at <a href="#common-attributes">Attributes common to all build rules</a>.
-        You should use this attribute to list other
-        <code>sh_library</code> or <code>proto_library</code> rules that provide
-        interpreted program source code depended on by the code in
-        <code>srcs</code>.  If you depend on a <code>proto_library</code> target,
-        the proto sources in that target will be included in this library, but
-        no generated files will be built.
-        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-
         /* <!-- #BLAZE_RULE(sh_library).ATTRIBUTE(srcs) -->
         The list of input files.
-        <i>(List of <a href="build-ref.html#labels">labels</a>,
-        optional)</i><br/>
-        You should use this attribute to list interpreted program
-        source files that belong to this package, such as additional
-        files containing Bourne shell subroutines, loaded via the shell's
-        <code>source</code> or <code>.</code> command.
+        ${SYNOPSIS}
+        <p>
+          This attribute should be used to list shell script source files that belong to
+          this library. Scripts can load other scripts using the shell's <code>source</code>
+          or <code>.</code> command.
+        </p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
-        .override(attr("srcs", LABEL_LIST).legacyAllowAnyFileType())
+        .override(attr("srcs", LABEL_LIST).allowedFileTypes(BazelShRuleClasses.SH_FILES))
         .build();
   }
 }
@@ -69,31 +56,24 @@ ${ATTRIBUTE_SIGNATURE}
   "library" consisting of related scripts&mdash;programs in an
   interpreted language that does not require compilation or linking,
   such as the Bourne shell&mdash;and any data those programs need at
-  run-time.  Such "libraries" can then be used from
+  run-time. Such "libraries" can then be used from
   the <code>data</code> attribute of one or
   more <code>sh_binary</code> rules.
 </p>
 
 <p>
-  Historically, a second use was to aggregate a collection of data files
-  together, to ensure that they are available at runtime in
-  the <code>.runfiles</code> area of one or more <code>*_binary</code>
-  rules (not necessarily <code>sh_binary</code>).
-  However, the <a href="#filegroup"><code>filegroup()</code></a> rule
-  should be used now; it is intended to replace this use of
-  <code>sh_library</code>.
+  You can use the <a href="#filegroup"><code>filegroup</code></a> rule to aggregate data files.
 </p>
 
 <p>
   In interpreted programming languages, there's not always a clear
   distinction between "code" and "data": after all, the program is
-  just "data" from the interpreter's point of view.  For this reason
-  (and historical accident) this rule has three attributes which are
-  all essentially equivalent: <code>srcs</code>, <code>deps</code>
-  and <code>data</code>.
-  The recommended usage of each attribute is mentioned below.  The
-  current implementation does not distinguish the elements of these lists.
-  All three attributes accept rules, source files and derived files.
+  just "data" from the interpreter's point of view. For this reason
+  this rule has three attributes which are all essentially equivalent:
+  <code>srcs</code>, <code>deps</code> and <code>data</code>.
+  The current implementation does not distinguish between the elements of these lists.
+  All three attributes accept rules, source files and generated files.
+  It is however good practice to use the attributes for their usual purpose (as with other rules).
 </p>
 
 ${ATTRIBUTE_DEFINITION}
@@ -104,7 +84,7 @@ ${ATTRIBUTE_DEFINITION}
 sh_library(
     name = "foo",
     data = [
-        ":foo_service_script",  # a sh_binary with srcs
+        ":foo_service_script",  # an sh_binary with srcs
         ":deploy_foo",  # another sh_binary with srcs
     ],
 )
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShRuleClasses.java
index 6f66465726..c1692afc27 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShRuleClasses.java
@@ -15,21 +15,22 @@
 package com.google.devtools.build.lib.bazel.rules.sh;
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.fromTemplates;
 import static com.google.devtools.build.lib.packages.Type.LABEL_LIST;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.BlazeRule;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
-import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.PredicateWithMessage;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
+import com.google.devtools.build.lib.util.FileType;
+import com.google.devtools.build.lib.util.FileTypeSet;
 
 import java.util.Collection;
 import java.util.Map;
@@ -42,7 +43,9 @@ import javax.annotation.Nullable;
 public final class BazelShRuleClasses {
 
   static final Collection<String> ALLOWED_RULES_IN_DEPS_WITH_WARNING = ImmutableSet.of(
-      "filegroup", "Fileset", "genrule", "sh_binary", "sh_test", "test_suite");
+      "filegroup", "genrule", "sh_binary", "sh_test", "test_suite");
+
+  static final FileTypeSet SH_FILES = FileTypeSet.of(FileType.of(".sh"));
 
   /**
    * Common attributes for shell rules.
@@ -54,9 +57,32 @@ public final class BazelShRuleClasses {
     @Override
     public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
       return builder
-          .add(attr("srcs", LABEL_LIST).mandatory().legacyAllowAnyFileType())
+          /* <!-- #BLAZE_RULE($sh_target).ATTRIBUTE(srcs) -->
+          The file containing the shell script.
+          ${SYNOPSIS}
+          <p>
+            This attribute must be a singleton list, whose element is the shell script.
+            This script must be executable, and may be a source file or a generated file.
+            All other files required at runtime (whether scripts or data) belong in the
+            <code>data</code> attribute.
+          </p>
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(attr("srcs", LABEL_LIST)
+              .mandatory()
+              .allowedFileTypes(SH_FILES))
+          /* <!-- #BLAZE_RULE($sh_target).ATTRIBUTE(deps) -->
+          The list of "library" targets to be aggregated into this target.
+          ${SYNOPSIS}
+          See general comments about <code>deps</code>
+          at <a href="#common-attributes.deps">Attributes common to all build rules</a>.
+          <p>
+            This attribute should be used to list other <code>sh_library</code> rules that provide
+            interpreted program source code depended on by the code in <code>srcs</code>. The files
+            provided by these rules will be present among the <code>runfiles</code> of this target.
+          </p>
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .override(builder.copy("deps")
-              .allowedRuleClasses("sh_library", "proto_library")
+              .allowedRuleClasses("sh_library")
               .allowedRuleClassesWithWarning(ALLOWED_RULES_IN_DEPS_WITH_WARNING)
               .allowedFileTypes())
           .build();
@@ -64,12 +90,6 @@ public final class BazelShRuleClasses {
   }
 
   /**
-   * Defines the file name of an sh_binary's implicit .sar (script package) output.
-   */
-  static final ImplicitOutputsFunction SAR_PACKAGE_FILENAME =
-      fromTemplates("%{name}.sar");
-
-  /**
    * Convenience structure for the bash dependency combinations defined
    * by BASH_BINARY_BINDINGS.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestRule.java
index 4a1e51f019..4472d53d22 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestRule.java
@@ -13,9 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.bazel.rules.sh;
 
-import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.Type.STRING;
-
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.BlazeRule;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
@@ -31,14 +28,10 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 @BlazeRule(name = "sh_test",
              type = RuleClassType.TEST,
              ancestors = { ShRule.class, BaseRuleClasses.TestBaseRule.class },
-             factoryClass = ShTest.class)
+             factoryClass = ShBinary.class)
 public final class BazelShTestRule implements RuleDefinition {
   @Override
   public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
-    return builder
-        .add(attr("bash_version", STRING)
-            .value(BazelShRuleClasses.DEFAULT_BASH_VERSION)
-            .allowedValues(BazelShRuleClasses.BASH_VERSION_ALLOWED_VALUES))
-        .build();
+    return builder.build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
index 4e6ba814a3..acdba76fc0 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
@@ -41,8 +41,8 @@ public class ShBinary implements RuleConfiguredTargetFactory {
     }
 
     Artifact symlink = ruleContext.createOutputArtifact();
+    // Note that src is used as the executable script too
     Artifact src = srcs.get(0);
-    Artifact executableScript = getExecutableScript(ruleContext, src);
     // The interpretation of this deceptively simple yet incredibly generic rule is complicated
     // by the distinction between targets and (not properly encapsulated) artifacts. It depends
     // on the notion of other rule's "files-to-build" sets, which are undocumented, making it
@@ -50,11 +50,10 @@ public class ShBinary implements RuleConfiguredTargetFactory {
     // happens when srcs = ['x', 'y'] but 'x' is an empty filegroup?). This is a pervasive
     // problem in Blaze.
     ruleContext.registerAction(
-        new ExecutableSymlinkAction(ruleContext.getActionOwner(), executableScript, symlink));
+        new ExecutableSymlinkAction(ruleContext.getActionOwner(), src, symlink));
 
     NestedSet<Artifact> filesToBuild = NestedSetBuilder.<Artifact>stableOrder()
         .add(src)
-        .add(executableScript) // May be the same as src, in which case set semantics apply.
         .add(symlink)
         .build();
     Runfiles runfiles = new Runfiles.Builder()
@@ -69,14 +68,4 @@ public class ShBinary implements RuleConfiguredTargetFactory {
         .addProvider(RunfilesProvider.class, RunfilesProvider.simple(runfiles))
         .build();
   }
-
-  /**
-   * Hook for sh_test to provide the executable.
-   *
-   * @param ruleContext
-   * @param src
-   */
-  protected Artifact getExecutableScript(RuleContext ruleContext, Artifact src) {
-    return src;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShTest.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShTest.java
deleted file mode 100644
index cc965aa405..0000000000
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShTest.java
+++ /dev/null
@@ -1,53 +0,0 @@
-// Copyright 2014 Google Inc. 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.sh;
-
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.packages.Type;
-import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
-import com.google.devtools.build.lib.vfs.PathFragment;
-
-/**
- * Implementation for sh_test rules.
- */
-public class ShTest extends ShBinary implements RuleConfiguredTargetFactory {
-
-  @Override
-  protected Artifact getExecutableScript(RuleContext ruleContext, Artifact src) {
-    if (ruleContext.attributes().get("bash_version", Type.STRING)
-        .equals(BazelShRuleClasses.SYSTEM_BASH_VERSION)) {
-      return src;
-    }
-
-    // What *will* this script run with the wrapper?
-    PathFragment newOutput = src.getRootRelativePath().getParentDirectory().getRelative(
-        ruleContext.getLabel().getName() + "_runner.sh");
-    Artifact testRunner = ruleContext.getAnalysisEnvironment().getDerivedArtifact(
-        newOutput, ruleContext.getConfiguration().getBinDirectory());
-
-    String bashPath = BazelShRuleClasses.BASH_BINARY_BINDINGS
-        .get(BazelShRuleClasses.SYSTEM_BASH_VERSION).execPath;
-
-    // Generate the runner contents.
-    String runnerContents =
-        "#!/bin/bash\n"
-        + bashPath + " \"" + src.getRootRelativePath().getPathString() + "\" \"$@\"\n";
-
-    ruleContext.registerAction(
-        new FileWriteAction(ruleContext.getActionOwner(), testRunner, runnerContents, true));
-    return testRunner;
-  }
-}