From 306f3d915695db6bbf167805ae5f371c0685564c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 21 Feb 2024 01:05:13 -0800 Subject: [PATCH] Automated rollback of commit b11fa7a7c7fdb37012c7a442b16f6fdcf90b9177. *** Reason for rollback *** Introduces a significant memory regression by adding a new skyframe edge from every target/aspect -> RepoMapping. This is particularly wasteful internally since we don't have any external repositories. *** Original change description *** 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 v... *** PiperOrigin-RevId: 608896971 Change-Id: Ibb7860d46a13d1a5cec394473fee192e6b122e2d --- .../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, 32 insertions(+), 254 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 50c17df23f53e8..12bab8df8168a8 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 @@ -26,7 +26,6 @@ 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; @@ -177,10 +176,4 @@ ImmutableList getBuildInfo( 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 633ef39e220fc6..d9c9d5d171ffbb 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 @@ -30,7 +30,6 @@ 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; @@ -89,8 +88,6 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment { */ private final List actions = new ArrayList<>(); - private final RepositoryMapping mainRepoMapping; - public CachingAnalysisEnvironment( ArtifactFactory artifactFactory, ActionKeyContext actionKeyContext, @@ -99,8 +96,7 @@ public CachingAnalysisEnvironment( boolean allowAnalysisFailures, ExtendedEventHandler errorEventListener, SkyFunction.Environment env, - StarlarkBuiltinsValue starlarkBuiltinsValue, - RepositoryMapping mainRepoMapping) { + StarlarkBuiltinsValue starlarkBuiltinsValue) { this.artifactFactory = artifactFactory; this.actionKeyContext = actionKeyContext; this.owner = Preconditions.checkNotNull(owner); @@ -109,7 +105,6 @@ public CachingAnalysisEnvironment( this.errorEventListener = errorEventListener; this.skyframeEnv = env; this.starlarkBuiltinsValue = starlarkBuiltinsValue; - this.mainRepoMapping = mainRepoMapping; middlemanFactory = new MiddlemanFactory(artifactFactory, this); } @@ -236,11 +231,6 @@ 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 400ffc4a8557fe..ecb5c28e176382 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 @@ -57,7 +57,6 @@ 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; @@ -328,11 +327,6 @@ 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 303d170367c138..2ea7ca146d3649 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,8 +16,6 @@ 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; @@ -25,8 +23,6 @@ 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; @@ -42,7 +38,6 @@ 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; @@ -55,7 +50,6 @@ 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; @@ -69,33 +63,6 @@ */ 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