Emit labels in display form in Java rules

Buildozer fixes for Java strict deps violations referring to external repositories now contain labels in display form, which avoid canonical repository names that should never be added to BUILD files and aren't understood by `buildozer`. The Starlark actions can use `Label.to_display_form()`. Java compilation actions, which are still implemented in Java, can't access the main repository mapping via `BazelModuleContext` and instead retrieve it from a new field on `AnalysisEnvironment`. Closes bazelbuild#21180. PiperOrigin-RevId: 607803030 Change-Id: I0cdccabfde0217c9201cef9ca9d260b0c8ca27cd
iancha1992 · Feb 16, 2024 · a50f364 · a50f364
1 parent 3dc745a
commit a50f364
Show file tree

Hide file tree

Showing 22 changed files with 254 additions and 32 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -176,4 +177,10 @@ ImmutableList<Artifact> getBuildInfo(
   ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles();
 
   ActionKeyContext getActionKeyContext();
+
+  /**
+   * The repository mapping applicable to the main repository. This is purely meant to support
+   * {@link com.google.devtools.build.lib.cmdline.Label#getDisplayForm()}.
+   */
+  RepositoryMapping getMainRepoMapping();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Target;
@@ -88,6 +89,8 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment {
    */
   private final List<ActionAnalysisMetadata> actions = new ArrayList<>();
 
+  private final RepositoryMapping mainRepoMapping;
+
   public CachingAnalysisEnvironment(
       ArtifactFactory artifactFactory,
       ActionKeyContext actionKeyContext,
@@ -96,7 +99,8 @@ public CachingAnalysisEnvironment(
       boolean allowAnalysisFailures,
       ExtendedEventHandler errorEventListener,
       SkyFunction.Environment env,
-      StarlarkBuiltinsValue starlarkBuiltinsValue) {
+      StarlarkBuiltinsValue starlarkBuiltinsValue,
+      RepositoryMapping mainRepoMapping) {
     this.artifactFactory = artifactFactory;
     this.actionKeyContext = actionKeyContext;
     this.owner = Preconditions.checkNotNull(owner);
@@ -105,6 +109,7 @@ public CachingAnalysisEnvironment(
     this.errorEventListener = errorEventListener;
     this.skyframeEnv = env;
     this.starlarkBuiltinsValue = starlarkBuiltinsValue;
+    this.mainRepoMapping = mainRepoMapping;
     middlemanFactory = new MiddlemanFactory(artifactFactory, this);
   }
 
@@ -231,6 +236,11 @@ public ActionKeyContext getActionKeyContext() {
     return actionKeyContext;
   }
 
+  @Override
+  public RepositoryMapping getMainRepoMapping() {
+    return mainRepoMapping;
+  }
+
   @Override
   public boolean hasErrors() {
     Preconditions.checkState(enabled);

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -57,6 +57,7 @@
 import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
 import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 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.ImmutableSortedKeyListMultimap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -327,6 +328,11 @@ public String getWorkspaceName() {
     return rule.getPackage().getWorkspaceName();
   }
 
+  /** Returns the repository mapping of the main repository. */
+  public RepositoryMapping getMainRepoMapping() {
+    return getAnalysisEnvironment().getMainRepoMapping();
+  }
+
   /** The configuration conditions that trigger this rule's configurable attributes. */
   public ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions() {
     return configConditions;

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
@@ -16,13 +16,17 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.CommandLineItem;
+import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile;
@@ -38,6 +42,7 @@
 import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -50,6 +55,7 @@
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider.JspecifyInfo;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -63,6 +69,33 @@
  */
 public final class JavaCompilationHelper {
 
+  /**
+   * Cache for the {@link com.google.devtools.build.lib.actions.CommandLineItem.MapFn} that turns a
+   * {@link Label} into its possibly @-prefixed display form, which requires the repository mapping
+   * of the main repo.
+   *
+   * <p>The capacity of this cache after weakly reachable object have been cleaned will always be 1
+   * as there is only a single main repo mapping in a given build.
+   */
+  static final LoadingCache<RepositoryMapping, CommandLineItem.ExceptionlessMapFn<Label>>
+      TARGET_LABEL_MAP_FN_CACHE =
+          Caffeine.newBuilder()
+              .initialCapacity(1)
+              .weakKeys()
+              .build(
+                  mainRepoMapping ->
+                      (ExceptionlessMapFn<Label> & Serializable)
+                          (label, args) -> {
+                            String displayForm = label.getDisplayForm(mainRepoMapping);
+                            if (displayForm.startsWith("@")) {
+                              // @-prefixed strings will be assumed to be filenames and expanded by
+                              // {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
+                              args.accept("@" + displayForm);
+                            } else {
+                              args.accept(displayForm);
+                            }
+                          });
+
   private static final Interner<ImmutableList<String>> javacOptsInterner =
       BlazeInterners.newWeakInterner();
   private static final Interner<ImmutableMap<String, String>> executionInfoInterner =

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java
@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.actions.extra.JavaCompileInfo;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
 import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -313,14 +314,12 @@ private CustomCommandLine buildParamFileContents(ImmutableList<String> javacOpts
       result.add("--");
     }
     if (targetLabel != null) {
-      result.add("--target_label");
-      if (targetLabel.getRepository().isMain()) {
-        result.addLabel(targetLabel);
-      } else {
-        // @-prefixed strings will be assumed to be filenames and expanded by
-        // {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
-        result.addPrefixedLabel("@", targetLabel);
-      }
+      result.addAll(
+          "--target_label",
+          VectorArg.of(ImmutableList.of(targetLabel))
+              .mapped(
+                  JavaCompilationHelper.TARGET_LABEL_MAP_FN_CACHE.get(
+                      ruleContext.getMainRepoMapping())));
     }
     result.add("--injecting_rule_kind", injectingRuleKind);
     // strict_java_deps controls whether the mapping from jars to targets is

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
 import com.google.devtools.build.lib.analysis.actions.PathMappers;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
@@ -443,14 +444,12 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
       }
 
       if (targetLabel != null) {
-        commandLine.add("--target_label");
-        if (targetLabel.getRepository().isMain()) {
-          commandLine.addLabel(targetLabel);
-        } else {
-          // @-prefixed strings will be assumed to be params filenames and expanded,
-          // so add an extra @ to escape it.
-          commandLine.addPrefixedLabel("@", targetLabel);
-        }
+        commandLine.addAll(
+            "--target_label",
+            VectorArg.of(ImmutableList.of(targetLabel))
+                .mapped(
+                    JavaCompilationHelper.TARGET_LABEL_MAP_FN_CACHE.get(
+                        ruleContext.getMainRepoMapping())));
       }
 
       ImmutableMap<String, String> executionInfo =

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -56,6 +56,7 @@
 import com.google.devtools.build.lib.causes.LabelCause;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
@@ -756,12 +757,23 @@ private AspectValue createAspect(
     if (env.valuesMissing()) {
       return null;
     }
+    RepositoryMappingValue mainRepoMappingValue =
+        (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
+    if (mainRepoMappingValue == null) {
+      return null;
+    }
 
     SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();
 
     StoredEventHandler events = new StoredEventHandler();
     CachingAnalysisEnvironment analysisEnvironment =
-        view.createAnalysisEnvironment(key, events, env, configuration, starlarkBuiltinsValue);
+        view.createAnalysisEnvironment(
+            key,
+            events,
+            env,
+            configuration,
+            starlarkBuiltinsValue,
+            mainRepoMappingValue.getRepositoryMapping());
 
     ConfiguredAspect configuredAspect;
     if (aspect.getDefinition().applyToGeneratingRules() && associatedTarget instanceof OutputFile) {

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.causes.AnalysisFailedCause;
 import com.google.devtools.build.lib.causes.Cause;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -372,11 +373,21 @@ private static ConfiguredTargetValue createConfiguredTarget(
     if (starlarkBuiltinsValue == null) {
       return null;
     }
+    RepositoryMappingValue mainRepoMappingValue =
+        (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
+    if (mainRepoMappingValue == null) {
+      return null;
+    }
 
     StoredEventHandler events = new StoredEventHandler();
     CachingAnalysisEnvironment analysisEnvironment =
         view.createAnalysisEnvironment(
-            configuredTargetKey, events, env, configuration, starlarkBuiltinsValue);
+            configuredTargetKey,
+            events,
+            env,
+            configuration,
+            starlarkBuiltinsValue,
+            mainRepoMappingValue.getRepositoryMapping());
 
     Preconditions.checkNotNull(depValueMap);
     ConfiguredTarget configuredTarget;

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -75,6 +75,7 @@
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics;
 import com.google.devtools.build.lib.causes.AnalysisFailedCause;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -1270,7 +1271,8 @@ CachingAnalysisEnvironment createAnalysisEnvironment(
       ExtendedEventHandler eventHandler,
       Environment env,
       BuildConfigurationValue config,
-      StarlarkBuiltinsValue starlarkBuiltinsValue) {
+      StarlarkBuiltinsValue starlarkBuiltinsValue,
+      RepositoryMapping mainRepoMapping) {
     boolean extendedSanityChecks = config != null && config.extendedSanityChecks();
     boolean allowAnalysisFailures = config != null && config.allowAnalysisFailures();
     return new CachingAnalysisEnvironment(
@@ -1281,7 +1283,8 @@ CachingAnalysisEnvironment createAnalysisEnvironment(
         allowAnalysisFailures,
         eventHandler,
         env,
-        starlarkBuiltinsValue);
+        starlarkBuiltinsValue,
+        mainRepoMapping);
   }
 
   /**

diff --git a/src/main/starlark/builtins_bzl/common/java/android_lint.bzl b/src/main/starlark/builtins_bzl/common/java/android_lint.bzl
@@ -92,7 +92,7 @@ def android_lint_action(ctx, source_files, source_jars, compilation_info):
     args.add_all("--bootclasspath", bootclasspath)
     args.add_all("--classpath", classpath)
     args.add_all("--lint_rules", compilation_info.plugins.processor_jars)
-    args.add("--target_label", ctx.label)
+    args.add_all("--target_label", [ctx.label], map_each = helper.map_to_display_form)
 
     javac_opts = compilation_info.javac_options
     if (javac_opts):

diff --git a/src/main/starlark/builtins_bzl/common/java/java_common.bzl b/src/main/starlark/builtins_bzl/common/java/java_common.bzl
@@ -120,7 +120,7 @@ def _stamp_jar(actions, jar, java_toolchain, target_label):
     args.add(jar)
     args.add(output)
     args.add("--nostrip_jar")
-    args.add("--target_label", target_label)
+    args.add_all("--target_label", [target_label], map_each = helper.map_to_display_form)
     actions.run(
         mnemonic = "JavaIjar",
         inputs = [jar],

diff --git a/src/main/starlark/builtins_bzl/common/java/java_common_internal_for_builtins.bzl b/src/main/starlark/builtins_bzl/common/java/java_common_internal_for_builtins.bzl
@@ -396,7 +396,7 @@ def run_ijar(
     args.add(jar)
     args.add(output)
     if target_label != None:
-        args.add("--target_label", target_label)
+        args.add_all("--target_label", [target_label], map_each = helper.map_to_display_form)
     if injecting_rule_kind != None:
         args.add("--injecting_rule_kind", injecting_rule_kind)
 

diff --git a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl
@@ -444,6 +444,17 @@ def _derive_output_file(ctx, base_file, *, name_suffix = "", extension = None, e
     new_basename = paths.replace_extension(base_file.basename, name_suffix + "." + extension + extension_suffix)
     return ctx.actions.declare_file(new_basename, sibling = base_file)
 
+def _map_to_display_form(label):
+    """`map_each` callback that formats a `Label` with `Label.to_display_form()`
+
+    Args:
+        label: (Label) the label of a target.
+
+    Returns:
+        (str) the display form representation of `label`
+    """
+    return label.to_display_form()
+
 helper = struct(
     collect_all_targets_as_deps = _collect_all_targets_as_deps,
     filter_launcher_for_target = _filter_launcher_for_target,
@@ -470,4 +481,5 @@ helper = struct(
     tokenize_javacopts = _tokenize_javacopts,
     detokenize_javacopts = _detokenize_javacopts,
     derive_output_file = _derive_output_file,
+    map_to_display_form = _map_to_display_form,
 )
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
@@ -50,6 +50,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 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.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -233,6 +234,11 @@ public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
     public ActionKeyContext getActionKeyContext() {
       return original.getActionKeyContext();
     }
+
+    @Override
+    public RepositoryMapping getMainRepoMapping() {
+      return original.getMainRepoMapping();
+    }
   }
 
   /** A dummy WorkspaceStatusAction. */
@@ -469,6 +475,11 @@ public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
     public ActionKeyContext getActionKeyContext() {
       return null;
     }
+
+    @Override
+    public RepositoryMapping getMainRepoMapping() {
+      throw new UnsupportedOperationException();
+    }
   }
 
   /** Matches the output path prefix contributed by a C++ configuration fragment. */

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD
@@ -107,6 +107,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:diff_awareness",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_roots_no_symlink_creation",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_executor_repository_helpers_holder",