From b11fa7a7c7fdb37012c7a442b16f6fdcf90b9177 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 16 Feb 2024 14:32:18 -0800 Subject: [PATCH] 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 #21180. PiperOrigin-RevId: 607803030 Change-Id: I0cdccabfde0217c9201cef9ca9d260b0c8ca27cd --- .../lib/analysis/AnalysisEnvironment.java | 7 ++ .../analysis/CachingAnalysisEnvironment.java | 12 ++- .../build/lib/analysis/RuleContext.java | 6 ++ .../lib/rules/java/JavaCompilationHelper.java | 33 ++++++++ .../rules/java/JavaCompileActionBuilder.java | 15 ++-- .../rules/java/JavaHeaderCompileAction.java | 15 ++-- .../build/lib/skyframe/AspectFunction.java | 14 +++- .../skyframe/ConfiguredTargetFunction.java | 13 ++- .../build/lib/skyframe/SkyframeBuildView.java | 7 +- .../builtins_bzl/common/java/android_lint.bzl | 2 +- .../builtins_bzl/common/java/java_common.bzl | 2 +- .../java_common_internal_for_builtins.bzl | 2 +- .../builtins_bzl/common/java/java_helper.bzl | 12 +++ .../lib/analysis/util/AnalysisTestUtil.java | 11 +++ .../devtools/build/lib/analysis/util/BUILD | 1 + .../analysis/util/BuildViewForTesting.java | 9 ++- .../lib/analysis/util/BuildViewTestCase.java | 18 ++++- .../devtools/build/lib/rules/android/BUILD | 1 + .../lib/rules/android/ResourceTestBase.java | 9 ++- src/test/shell/bazel/BUILD | 16 +++- src/test/shell/bazel/bazel_java_test.sh | 79 +++++++++++++++++++ src/test/shell/bazel/local_repository_test.sh | 2 +- 22 files changed, 254 insertions(+), 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 index d4b6ec7ff51dc9..a863b146c6956c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MiddlemanFactory; 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; @@ -166,4 +167,10 @@ Artifact.DerivedArtifact getDerivedArtifact( ImmutableSet 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 index 0ee01e8cf7b47d..48b8a258b82614 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MiddlemanFactory; +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; @@ -84,6 +85,8 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment { */ private final List actions = new ArrayList<>(); + private final RepositoryMapping mainRepoMapping; + public CachingAnalysisEnvironment( ArtifactFactory artifactFactory, ActionKeyContext actionKeyContext, @@ -92,7 +95,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); @@ -101,6 +105,7 @@ public CachingAnalysisEnvironment( this.errorEventListener = errorEventListener; this.skyframeEnv = env; this.starlarkBuiltinsValue = starlarkBuiltinsValue; + this.mainRepoMapping = mainRepoMapping; middlemanFactory = new MiddlemanFactory(artifactFactory, this); } @@ -227,6 +232,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 index a2c4e5ac4f6cdd..95dccd67c07012 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -54,6 +54,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; @@ -375,6 +376,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 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 index 984065b31e65a1..43a6903a91bf5b 100644 --- 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,6 +16,8 @@ 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; @@ -23,6 +25,8 @@ 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. + * + *

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> + TARGET_LABEL_MAP_FN_CACHE = + Caffeine.newBuilder() + .initialCapacity(1) + .weakKeys() + .build( + mainRepoMapping -> + (ExceptionlessMapFn