Stringify Labels in display form in Args

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -411,6 +411,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:workspace_status_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe/config",
@@ -2442,6 +2443,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",

src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java

@@ -23,6 +23,8 @@
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -70,7 +72,8 @@ public void repr(Printer printer) {
   @Override
   public void debugPrint(Printer printer, StarlarkSemantics semantics) {
     try {
-      printer.append(Joiner.on(" ").join(build().arguments()));
+      printer.append(
+          Joiner.on(" ").join(build(/* mainRepoMappingSupplier */ () -> null).arguments()));
     } catch (CommandLineExpansionException e) {
       printer.append("Cannot expand command line: " + e.getMessage());
     } catch (InterruptedException e) {
@@ -101,8 +104,13 @@ public void debugPrint(Printer printer, StarlarkSemantics semantics) {
    */
   public abstract ImmutableSet<Artifact> getDirectoryArtifacts();
 
+  public interface InterruptibleRepoMappingSupplier {
+    RepositoryMapping get() throws InterruptedException;
+  }
+
   /** Returns the command line built by this {@link Args} object. */
-  public abstract CommandLine build();
+  public abstract CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier)
+      throws InterruptedException;
 
   /**
    * Returns a frozen {@link Args} representation corresponding to an already-registered action.
@@ -157,7 +165,7 @@ public ImmutableSet<Artifact> getDirectoryArtifacts() {
     }
 
     @Override
-    public CommandLine build() {
+    public CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier) {
       return commandLine;
     }
 
@@ -259,6 +267,12 @@ private static class MutableArgs extends Args implements StarlarkValue, Mutabili
      */
     private boolean flagPerLine = false;
 
+    /**
+     * True if the command line needs to stringify any {@link Label}s without an explicit 'map_each'
+     * function.
+     */
+    private boolean mayStringifyExternalLabel = false;
+
     // May be set explicitly once -- if unset defaults to ParameterFileType.SHELL_QUOTED.
     private ParameterFileType parameterFileType = null;
     private String flagFormatString;
@@ -307,6 +321,9 @@ public CommandLineArgsApi addArgument(
             "Args.add() doesn't accept vectorized arguments. Please use Args.add_all() or"
                 + " Args.add_joined() instead.");
       }
+      if (value instanceof Label label && !label.getRepository().isMain()) {
+        mayStringifyExternalLabel = true;
+      }
       addSingleArg(value, format != Starlark.NONE ? (String) format : null);
       return this;
     }
@@ -436,8 +453,10 @@ private void addVectorArg(
       validateFormatString("format_each", formatEach);
       validateFormatString("format_joined", formatJoined);
       StarlarkCustomCommandLine.VectorArg.Builder vectorArg;
-      if (value instanceof Depset) {
-        Depset starlarkNestedSet = (Depset) value;
+      if (value instanceof Depset starlarkNestedSet) {
+        if (mapEach == null && Label.class.equals(starlarkNestedSet.getElementClass())) {
+          mayStringifyExternalLabel = true;
+        }
         NestedSet<?> nestedSet = starlarkNestedSet.getSet();
         if (nestedSet.isEmpty() && omitIfEmpty) {
           return;
@@ -454,6 +473,10 @@ private void addVectorArg(
         if (expandDirectories) {
           scanForDirectories(starlarkList);
         }
+        mayStringifyExternalLabel |=
+            (mapEach != null)
+                && starlarkList.stream()
+                    .anyMatch(o -> o instanceof Label label && !label.getRepository().isMain());
         vectorArg = new StarlarkCustomCommandLine.VectorArg.Builder(starlarkList);
       }
       commandLine.recordArgStart();
@@ -573,8 +596,10 @@ private MutableArgs(@Nullable Mutability mutability, StarlarkSemantics starlarkS
     }
 
     @Override
-    public CommandLine build() {
-      return commandLine.build(flagPerLine);
+    public CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier)
+        throws InterruptedException {
+      return commandLine.build(
+          flagPerLine, mayStringifyExternalLabel ? mainRepoMappingSupplier.get() : null);
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java

@@ -332,7 +332,8 @@ public void symlink(
   }
 
   @Override
-  public void write(FileApi output, Object content, Boolean isExecutable) throws EvalException {
+  public void write(FileApi output, Object content, Boolean isExecutable)
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.write");
     RuleContext ruleContext = getRuleContext();
 
@@ -347,7 +348,7 @@ public void write(FileApi output, Object content, Boolean isExecutable) throws E
               ruleContext.getActionOwner(),
               NestedSetBuilder.wrap(Order.STABLE_ORDER, args.getDirectoryArtifacts()),
               (Artifact) output,
-              args.build(),
+              args.build(context.getRuleContext().getAnalysisEnvironment()::getMainRepoMapping),
               args.getParameterFileType());
     } else {
       throw new AssertionError("Unexpected type: " + content.getClass().getSimpleName());
@@ -373,7 +374,7 @@ public void run(
       Object shadowedActionUnchecked,
       Object resourceSetUnchecked,
       Object toolchainUnchecked)
-      throws EvalException {
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.run");
     execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
     toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);
@@ -382,7 +383,7 @@ public void run(
     boolean useAutoExecGroups = ruleContext.useAutoExecGroups();
 
     StarlarkAction.Builder builder = new StarlarkAction.Builder();
-    buildCommandLine(builder, arguments);
+    buildCommandLine(builder, arguments, ruleContext.getAnalysisEnvironment()::getMainRepoMapping);
     if (executableUnchecked instanceof Artifact) {
       Artifact executable = (Artifact) executableUnchecked;
       FilesToRunProvider provider = context.getExecutableRunfiles(executable, "executable");
@@ -560,15 +561,15 @@ public void runShell(
       Object shadowedActionUnchecked,
       Object resourceSetUnchecked,
       Object toolchainUnchecked)
-      throws EvalException {
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.run_shell");
     execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
     toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);
 
     RuleContext ruleContext = getRuleContext();
 
     StarlarkAction.Builder builder = new StarlarkAction.Builder();
-    buildCommandLine(builder, arguments);
+    buildCommandLine(builder, arguments, ruleContext.getAnalysisEnvironment()::getMainRepoMapping);
 
     // When we use a shell command, add an empty argument before other arguments.
     //   e.g.  bash -c "cmd" '' 'arg1' 'arg2'
@@ -631,8 +632,11 @@ public void runShell(
         builder);
   }
 
-  private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> argumentsList)
-      throws EvalException {
+  private static void buildCommandLine(
+      SpawnAction.Builder builder,
+      Sequence<?> argumentsList,
+      Args.InterruptibleRepoMappingSupplier repoMappingSupplier)
+      throws EvalException, InterruptedException {
     ImmutableList.Builder<String> stringArgs = null;
     for (Object value : argumentsList) {
       if (value instanceof String) {
@@ -647,7 +651,7 @@ private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> ar
         }
         Args args = (Args) value;
         ParamFileInfo paramFileInfo = args.getParamFileInfo();
-        builder.addCommandLine(args.build(), paramFileInfo);
+        builder.addCommandLine(args.build(repoMappingSupplier), paramFileInfo);
       } else {
         throw Starlark.errorf(
             "expected list of strings or ctx.actions.args() for arguments instead of %s",

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractCommandLine;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -36,6 +37,8 @@
 import com.google.devtools.build.lib.actions.PathMapper;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
@@ -182,7 +185,8 @@ private int eval(
         int argi,
         List<String> builder,
         @Nullable ArtifactExpander artifactExpander,
-        PathMapper pathMapper)
+        PathMapper pathMapper,
+        @Nullable RepositoryMapping mainRepoMapping)
         throws CommandLineExpansionException, InterruptedException {
       StarlarkCallable mapEach = null;
       StarlarkSemantics starlarkSemantics = null;
@@ -219,7 +223,7 @@ private int eval(
         int count = expandedValues.size();
         stringValues = new ArrayList<>(expandedValues.size());
         for (int i = 0; i < count; ++i) {
-          stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper));
+          stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper, mainRepoMapping));
         }
       }
       // It's safe to uniquify at this stage, any transformations after this
@@ -607,9 +611,15 @@ static void push(List<Object> arguments, Object object, String format) {
       arguments.add(format);
     }
 
-    static int eval(List<Object> arguments, int argi, List<String> builder, PathMapper pathMapper) {
+    static int eval(
+        List<Object> arguments,
+        int argi,
+        List<String> builder,
+        PathMapper pathMapper,
+        @Nullable RepositoryMapping mainRepoMapping) {
       Object object = arguments.get(argi++);
-      String stringValue = StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper);
+      String stringValue =
+          StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper, mainRepoMapping);
       String formatStr = (String) arguments.get(argi++);
       builder.add(SingleStringArgFormatter.format(formatStr, stringValue));
       return argi;
@@ -662,10 +672,13 @@ Builder addFormatted(Object object, String format) {
       return this;
     }
 
-    CommandLine build(boolean flagPerLine) {
+    CommandLine build(boolean flagPerLine, @Nullable RepositoryMapping mainRepoMapping) {
       if (arguments.isEmpty()) {
         return CommandLine.empty();
       }
+      if (mainRepoMapping != null) {
+        arguments.add(mainRepoMapping);
+      }
       Object[] args = arguments.toArray();
       return flagPerLine
           ? new StarlarkCustomCommandLineWithIndexes(args, argStartIndexes.build())
@@ -700,20 +713,37 @@ public Iterable<String> arguments(
     List<String> result = new ArrayList<>();
     List<Object> arguments = rawArgsAsList();
 
-    for (int argi = 0; argi < arguments.size(); ) {
+    RepositoryMapping mainRepoMapping;
+    int size;
+    if (arguments.getLast() instanceof RepositoryMapping) {
+      mainRepoMapping = (RepositoryMapping) arguments.getLast();
+      size = arguments.size() - 1;
+    } else {
+      mainRepoMapping = null;
+      size = arguments.size();
+    }
+
+    for (int argi = 0; argi < size; ) {
       Object arg = arguments.get(argi++);
       if (arg instanceof VectorArg) {
-        argi = ((VectorArg) arg).eval(arguments, argi, result, artifactExpander, pathMapper);
+        argi =
+            ((VectorArg) arg)
+                .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping);
       } else if (arg == SingleFormattedArg.MARKER) {
-        argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper);
+        argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper, mainRepoMapping);
       } else {
-        result.add(expandToCommandLine(arg, pathMapper));
+        result.add(expandToCommandLine(arg, pathMapper, mainRepoMapping));
       }
     }
     return ImmutableList.copyOf(pathMapper.mapCustomStarlarkArgs(result));
   }
 
-  private static String expandToCommandLine(Object object, PathMapper pathMapper) {
+  private static String expandToCommandLine(
+      Object object, PathMapper pathMapper, @Nullable RepositoryMapping mainRepoMapping) {
+    if (mainRepoMapping != null && object instanceof Label label) {
+      return label.getDisplayForm(mainRepoMapping);
+    }
+
     // It'd be nice to build this into DerivedArtifact's CommandLine interface so we don't have
     // to explicitly check if an object is a DerivedArtifact. Unfortunately that would require
     // a lot more dependencies on the Java library DerivedArtifact is built into.
@@ -751,7 +781,17 @@ public Iterable<String> arguments(
       Iterator<Integer> startIndexIterator = argStartIndexes.iterator();
       int nextStartIndex = startIndexIterator.hasNext() ? startIndexIterator.next() : -1;
 
-      for (int argi = 0; argi < arguments.size(); ) {
+      RepositoryMapping mainRepoMapping;
+      int size;
+      if (arguments.getLast() instanceof RepositoryMapping) {
+        mainRepoMapping = (RepositoryMapping) arguments.getLast();
+        size = arguments.size() - 1;
+      } else {
+        mainRepoMapping = null;
+        size = arguments.size();
+      }
+
+      for (int argi = 0; argi < size; ) {
 
         // Record the actual beginning of each group.
         if (argi == nextStartIndex) {
@@ -761,11 +801,14 @@ public Iterable<String> arguments(
 
         Object arg = arguments.get(argi++);
         if (arg instanceof VectorArg) {
-          argi = ((VectorArg) arg).eval(arguments, argi, result, artifactExpander, pathMapper);
+          argi =
+              ((VectorArg) arg)
+                  .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping);
         } else if (arg == SingleFormattedArg.MARKER) {
-          argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper);
+          argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper, mainRepoMapping);
         } else {
-          result.add(StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper));
+          result.add(
+              StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper, mainRepoMapping));
         }
       }
 
@@ -796,7 +839,15 @@ public void addToFingerprint(
       Fingerprint fingerprint)
       throws CommandLineExpansionException, InterruptedException {
     List<Object> arguments = rawArgsAsList();
-    for (int argi = 0; argi < arguments.size(); ) {
+    int size;
+    if (arguments.getLast() instanceof RepositoryMapping mainRepoMapping) {
+      fingerprint.addStringMap(
+          Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName));
+      size = arguments.size() - 1;
+    } else {
+      size = arguments.size();
+    }
+    for (int argi = 0; argi < size; ) {
       Object arg = arguments.get(argi++);
       if (arg instanceof VectorArg) {
         argi =

src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java

@@ -31,6 +31,10 @@
  * <p>For repositories from the WORKSPACE file, if the requested repo doesn't exist in the mapping,
  * we fallback to the requested name. For repositories from Bzlmod, we return null to let the caller
  * decide what to do.
+ *
+ * <p>This class must not implement {@link net.starlark.java.eval.StarlarkValue} since instances of
+ * this class are used as markers by {@link
+ * com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine}.
  */
 public class RepositoryMapping {
   /* A repo mapping that always falls back to using the apparent name as the canonical name. */

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java

@@ -66,6 +66,14 @@
             + "<li>Values that are already strings are left as-is." //
             + "<li><a href='../builtins/File.html'><code>File</code></a> objects are turned into"
             + " their     <code>File.path</code> values." //
+            + "<li><a href='../builtins/Label.html'><code>Label</code></a> objects are turned into"
+            + " a string representation that resolves back to the same object when resolved in the"
+            + " context of the main repository. If possible, the string representation uses the"
+            + " apparent name of a repository in favor of the repository's canonical name, which"
+            + " makes this representation suited for use in BUILD files. While the exact form of"
+            + " the representation is not guaranteed, typical examples are"
+            + " <code>//foo:bar</code>, <code>@repo//foo:bar</code> and"
+            + " <code>@@canonical_name~//foo:bar.bzl</code>."
             + "<li>All other types are turned into strings in an <i>unspecified</i> manner. For "
             + "    this reason, you should avoid passing values that are not of string or "
             + "    <code>File</code> type to <code>add()</code>, and if you pass them to "