Skylark: ctx.actions.run_shell uses helper script

If the shell command in ctx.actions.run_shell.command is longer than the platform's shell's limit, Bazel will dump the command to a helper shell script and execute that script in the run_shell action. Genrules also write a helper script when genrule.cmd is longer than the shell's limit, and ctx.actions.run_shell now uses the same machinery. Fixes bazelbuild/bazel#3589 Change-Id: Ib24dce90182ef69552deb2d400e00ae061537309 PiperOrigin-RevId: 167126560
luca-digrazia · Sep 4, 2022 · fddddc9 · fddddc9
1 parent 49e2a44
commit fddddc9
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 73 deletions.
diff --git a/.../bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/.../bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
@@ -26,7 +26,9 @@
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -71,7 +73,7 @@ public final class CommandHelper {
    * This is similar to heuristic location expansion in LocationExpander
    * and should be kept in sync.
    */
-  private final ImmutableMap<Label, ImmutableCollection<Artifact>> labelMap;
+  private final SkylarkDict<Label, ImmutableCollection<Artifact>> labelMap;
 
   /**
    * The ruleContext this helper works on
@@ -133,7 +135,7 @@ public CommandHelper(
     for (Entry<Label, Collection<Artifact>> entry : tempLabelMap.entrySet()) {
       labelMapBuilder.put(entry.getKey(), ImmutableList.copyOf(entry.getValue()));
     }
-    this.labelMap = labelMapBuilder.build();
+    this.labelMap = SkylarkDict.copyOf(null, labelMapBuilder.build());
   }
 
   public SkylarkList<Artifact> getResolvedTools() {
@@ -158,41 +160,41 @@ private static Collection<Artifact> mapGet(Map<Label, Collection<Artifact>> map,
     return values;
   }
 
-  /**
-   * Resolves a command, and expands known locations for $(location) variables. This method supports
-   * legacy heuristic label expansion, which replaces strings that look like labels with their
-   * corresponding file names. Use {@link #resolveCommandAndExpandLabels} instead.
-   */
-  public String resolveCommandAndHeuristicallyExpandLabels(
-      String command, @Nullable String attribute, boolean enableLegacyHeuristicLabelExpansion) {
-    command = resolveCommandAndExpandLabels(command, attribute, false);
-    if (enableLegacyHeuristicLabelExpansion) {
-      command = expandLabels(command, labelMap);
-    }
-    return command;
-  }
-
   /**
    * Resolves a command, and expands known locations for $(location)
    * variables.
    */
   public String resolveCommandAndExpandLabels(
-      String command, @Nullable String attribute, boolean allowDataInLabel) {
-    LocationExpander expander;
-    if (allowDataInLabel) {
-      expander = new LocationExpander(ruleContext, labelMap,
-          LocationExpander.Options.EXEC_PATHS, LocationExpander.Options.ALLOW_DATA);
-    } else {
-      expander = new LocationExpander(ruleContext, labelMap, LocationExpander.Options.EXEC_PATHS);
-    }
+      String command,
+      @Nullable String attribute,
+      Boolean supportLegacyExpansion,
+      Boolean allowDataInLabel) {
+    LocationExpander expander = new LocationExpander(
+        ruleContext, ImmutableMap.copyOf(labelMap), allowDataInLabel);
     if (attribute != null) {
       command = expander.expandAttribute(attribute, command);
     } else {
       command = expander.expand(command);
     }
+    if (supportLegacyExpansion) {
+      command = expandLabels(command, labelMap);
+    }
     return command;
   }
 
+  /**
+   * Resolves the 'cmd' attribute, and expands known locations for $(location)
+   * variables.
+   */
+  public String resolveCommandAndExpandLabels(
+      Boolean supportLegacyExpansion, Boolean allowDataInLabel) {
+    return resolveCommandAndExpandLabels(
+        ruleContext.attributes().get("cmd", Type.STRING),
+        "cmd",
+        supportLegacyExpansion,
+        allowDataInLabel);
+  }
+
   /**
    * Expands labels occurring in the string "expr" in the rule 'cmd'.
    * Each label must be valid, be a declared prerequisite, and expand to a

diff --git a/...el/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/...el/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -292,11 +292,15 @@ public void write(Artifact output, String content, Boolean isExecutable) throws
         type = Object.class,
         allowedTypes = {
           @ParamType(type = SkylarkList.class),
+          @ParamType(type = Args.class),
         },
         defaultValue = "[]",
         named = true,
         positional = false,
-        doc = "command line arguments of the action."
+        doc =
+            "command line arguments of the action. "
+                + "May be either a list or an actions.args() object. "
+                + "See <a href=\"actions.html#args\">ctx.actions.args()</a>."
       ),
       @Param(
         name = "mnemonic",
@@ -438,12 +442,14 @@ public void run(
         name = "arguments",
         allowedTypes = {
           @ParamType(type = SkylarkList.class),
+          @ParamType(type = Args.class),
         },
         defaultValue = "[]",
         named = true,
         positional = false,
         doc =
             "command line arguments of the action. "
+                + "May be either a list or an actions.args() object.<br><br>"
                 + "Blaze passes the elements in this attribute as arguments to the command."
                 + "The command can access these arguments as <code>$1</code>, <code>$2</code>, "
                 + "etc. See <a href=\"actions.html#args\">ctx.actions.args()</a>."
@@ -966,6 +972,15 @@ public SkylarkCustomCommandLine build() {
     }
   }
 
+  @SkylarkCallable(
+    name = "args",
+    doc = "returns an Args object that can be used to build memory-efficient command lines."
+  )
+  public Args args() {
+    return new Args(
+        skylarkSemanticsOptions, ruleContext.getAnalysisEnvironment().getEventHandler());
+  }
+
   @Override
   public boolean isImmutable() {
     return context.isImmutable();

diff --git a/...ild.bazel/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/...ild.bazel/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -16,29 +16,20 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
-import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ActionsProvider;
-import com.google.devtools.build.lib.analysis.BaseRuleClasses;
-import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
-import com.google.devtools.build.lib.analysis.RuleDefinition;
-import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Info;
-import com.google.devtools.build.lib.packages.RuleClass;
-import com.google.devtools.build.lib.packages.RuleClass.Builder;
-import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.rules.python.PyCommon;
@@ -47,9 +38,6 @@
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
-import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
-import com.google.devtools.build.lib.testutil.UnknownRuleConfiguredTarget;
-import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -65,42 +53,6 @@
 @RunWith(JUnit4.class)
 public class SkylarkRuleContextTest extends SkylarkTestCase {
 
-  /**
-   * A test rule that exercises the semantics of mandatory providers.
-   */
-  public static final class TestingRuleForMandatoryProviders implements RuleDefinition {
-    @Override
-    public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
-      return builder
-          .setUndocumented()
-          .add(attr("srcs", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE))
-          .override(builder.copy("deps").mandatoryProvidersList(
-              ImmutableList.of(
-                  ImmutableList.of(SkylarkProviderIdentifier.forLegacy("a")),
-                  ImmutableList.of(
-                      SkylarkProviderIdentifier.forLegacy("b"),
-                      SkylarkProviderIdentifier.forLegacy("c")))))
-          .build();
-    }
-
-    @Override
-    public Metadata getMetadata() {
-      return RuleDefinition.Metadata.builder()
-          .name("testing_rule_for_mandatory_providers")
-          .ancestors(BaseRuleClasses.RuleBase.class)
-          .factoryClass(UnknownRuleConfiguredTarget.class)
-          .build();
-    }
-  }
-
-  @Override
-  protected ConfiguredRuleClassProvider getRuleClassProvider() {
-    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder()
-        .addRuleDefinition(new TestingRuleForMandatoryProviders());
-    TestRuleClassProvider.addStandardRules(builder);
-    return builder.build();
-  }
-
   @Before
   public final void generateBuildFile() throws Exception {
     scratch.file(
@@ -2138,4 +2090,5 @@ public void testIncompatibleNewActionsApi() throws Exception {
       }
     }
   }
+
 }