Automated rollback of commit 2f10da0db062e023b1f0f8222f8545467b29ae4e.

*** Reason for rollback ***

Breaks skylark rules using $ in action cmd line.

*** Original change description ***

Change CommandHelper to use TemplateExpander directly

This is a partial rollback of https://github.com/bazelbuild/bazel/commit/e8d32b7c922f65539b74357711d5ad6b70934115, only the CommandHelper change.

Progress on #2475.

PiperOrigin-RevId: 179607027

4 files changed, 38 insertions, 57 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
index cd7a8f38b7..85dfc7a043 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
@@ -24,9 +24,6 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
-import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
-import com.google.devtools.build.lib.analysis.stringtemplate.TemplateExpander;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -166,26 +163,24 @@ public final class CommandHelper {
   }
 
   /**
-   * Resolves a command, and expands known locations for $(location) variables.
+   * Resolves a command, and expands known locations for $(location)
+   * variables.
    */
   @Deprecated // Only exists to support a legacy Skylark API.
-  public String expandForSkylark(
-      String command, @Nullable String attribute,
-      TemplateContext templateContext, boolean expandLocations) {
-    try {
-      if (expandLocations) {
-        templateContext = new LocationTemplateContext(
-            templateContext, ruleContext, labelMap, true, false);
-      }
-      return TemplateExpander.expand(command, templateContext);
-    } catch (ExpansionException e) {
-      if (attribute != null) {
-        ruleContext.attributeError(attribute, e.getMessage());
-      } else {
-        ruleContext.ruleError(e.getMessage());
-      }
-      return command;
+  public String resolveCommandAndExpandLabels(
+      String command, @Nullable String attribute, boolean allowDataInLabel) {
+    LocationExpander expander;
+    if (allowDataInLabel) {
+      expander = LocationExpander.withExecPathsAndData(ruleContext, labelMap);
+    } else {
+      expander = LocationExpander.withExecPaths(ruleContext, labelMap);
+    }
+    if (attribute != null) {
+      command = expander.expandAttribute(attribute, command);
+    } else {
+      command = expander.expand(command);
     }
+    return command;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index ff911f02cf..8388391361 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -39,7 +39,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.FragmentCollection;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
-import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -1129,30 +1128,27 @@ public final class SkylarkRuleContext implements SkylarkValue {
       + "Additional variables may come from other places, such as configurations. Note that "
       + "this function is experimental.")
   public String expandMakeVariables(String attributeName, String command,
-      Map<String, String> additionalSubstitutions) throws EvalException {
+      final Map<String, String> additionalSubstitutions) throws EvalException {
     checkMutable("expand_make_variables");
-    TemplateContext templateContext = getConfigurationMakeVariableContext(additionalSubstitutions);
-    return ruleContext.getExpander(templateContext).expand(attributeName, command);
+    ConfigurationMakeVariableContext makeVariableContext =
+        new ConfigurationMakeVariableContext(
+            // TODO(lberki): This should be removed. But only after either verifying that no one
+            // uses it or providing an alternative.
+            ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
+            ruleContext.getRule().getPackage(),
+            ruleContext.getConfiguration()) {
+          @Override
+          public String lookupVariable(String variableName) throws ExpansionException {
+            if (additionalSubstitutions.containsKey(variableName)) {
+              return additionalSubstitutions.get(variableName);
+            } else {
+              return super.lookupVariable(variableName);
+            }
+          }
+        };
+    return ruleContext.getExpander(makeVariableContext).expand(attributeName, command);
   }
 
-  TemplateContext getConfigurationMakeVariableContext(
-      final Map<String, String> additionalSubstitutions) {
-    return new ConfigurationMakeVariableContext(
-        // TODO(lberki): This should be removed. But only after either verifying that no one
-        // uses it or providing an alternative.
-        ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
-        ruleContext.getRule().getPackage(),
-        ruleContext.getConfiguration()) {
-      @Override
-      public String lookupVariable(String variableName) throws ExpansionException {
-        if (additionalSubstitutions.containsKey(variableName)) {
-          return additionalSubstitutions.get(variableName);
-        } else {
-          return super.lookupVariable(variableName);
-        }
-      }
-    };
-  }
 
   FilesToRunProvider getExecutableRunfiles(Artifact executable) {
     return attributesCollection.getExecutableRunfilesMap().get(executable);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
index df2e9a00d8..e6efe86495 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
@@ -25,7 +25,6 @@ import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.configuredtargets.AbstractConfiguredTarget;
-import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.Param;
@@ -710,13 +709,15 @@ public class SkylarkRuleImplementationFunctions {
                   ImmutableMap.copyOf(labelDict));
           String attribute =
               Type.STRING.convertOptional(attributeUnchecked, "attribute", ruleLabel);
-          TemplateContext templateContext = TemplateContext.EMPTY;
+          if (expandLocations) {
+            command = helper.resolveCommandAndExpandLabels(
+                command, attribute, /*allowDataInLabel=*/false);
+          }
           if (!EvalUtils.isNullOrNone(makeVariablesUnchecked)) {
             Map<String, String> makeVariables =
                 Type.STRING_DICT.convert(makeVariablesUnchecked, "make_variables", ruleLabel);
-            templateContext = ctx.getConfigurationMakeVariableContext(makeVariables);
+            command = ctx.expandMakeVariables(attribute, command, makeVariables);
           }
-          command = helper.expandForSkylark(command, attribute, templateContext, expandLocations);
           List<Artifact> inputs = new ArrayList<>();
           inputs.addAll(helper.getResolvedTools());
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
index 179af8746a..dc6d27598d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
@@ -18,17 +18,6 @@ package com.google.devtools.build.lib.analysis.stringtemplate;
  * each template variable and function.
  */
 public interface TemplateContext {
-  public static final TemplateContext EMPTY = new TemplateContext() {
-    @Override
-    public String lookupVariable(String name) throws ExpansionException {
-      throw new ExpansionException(String.format("$(%s) not defined", name));
-    }
-
-    @Override
-    public String lookupFunction(String name, String param) throws ExpansionException {
-      throw new ExpansionException(String.format("$(%s) not defined", name));
-    }
-  };
 
   /**
    * Returns the expansion of the specified template variable.