From 26a1a6596e0707b01e5031cf930f6fe1cb4f12fc Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Mon, 10 Jul 2023 09:54:48 -0700 Subject: [PATCH] Revert "Don't eagerly flatten a `NestedSet` in `RepoMappingManifestAction` (#18419)" This reverts commit e023bd398eb5eb3d16cf1f81ae990460acf44209. --- .../google/devtools/build/lib/analysis/BUILD | 2 +- .../analysis/RepoMappingManifestAction.java | 130 +++++++----------- .../build/lib/analysis/RunfilesSupport.java | 47 ++++++- .../google/devtools/build/lib/analysis/BUILD | 8 +- .../RunfilesRepoMappingManifestTest.java | 97 +------------ 5 files changed, 101 insertions(+), 183 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 546afe042eb40c..c8b032e3eb9730 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -995,9 +995,9 @@ java_library( "//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/packages", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/net/starlark/java/eval", + "//third_party:auto_value", "//third_party:guava", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java index 59534d2c72f1be..2714680906f90a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java @@ -13,69 +13,61 @@ // limitations under the License. package com.google.devtools.build.lib.analysis; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.util.Comparator.comparing; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.CommandLineExpansionException; -import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction; import com.google.devtools.build.lib.analysis.actions.DeterministicWriter; -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.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; -import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.util.Fingerprint; import java.io.PrintWriter; -import java.util.Map.Entry; +import java.util.List; import java.util.UUID; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; /** Creates a manifest file describing the repos and mappings relevant for a runfile tree. */ -public final class RepoMappingManifestAction extends AbstractFileWriteAction { - +public class RepoMappingManifestAction extends AbstractFileWriteAction { private static final UUID MY_UUID = UUID.fromString("458e351c-4d30-433d-b927-da6cddd4737f"); - // Uses MapFn's args parameter just like Fingerprint#addString to compute a cacheable fingerprint - // of just the repo name and mapping of a given Package. - private static final MapFn REPO_AND_MAPPING_DIGEST_FN = - (pkg, args) -> { - args.accept(pkg.getPackageIdentifier().getRepository().getName()); + private final ImmutableList entries; + private final String workspaceName; - var mapping = pkg.getRepositoryMapping().entries(); - args.accept(String.valueOf(mapping.size())); - mapping.forEach( - (apparentName, canonicalName) -> { - args.accept(apparentName); - args.accept(canonicalName.getName()); - }); - }; + /** An entry in the repo mapping manifest file. */ + @AutoValue + public abstract static class Entry { + public static Entry of( + RepositoryName sourceRepo, String targetRepoApparentName, RepositoryName targetRepo) { + return new AutoValue_RepoMappingManifestAction_Entry( + sourceRepo, targetRepoApparentName, targetRepo); + } - private final NestedSet transitivePackages; - private final NestedSet runfilesArtifacts; - private final String workspaceName; + public abstract RepositoryName sourceRepo(); + + public abstract String targetRepoApparentName(); + + public abstract RepositoryName targetRepo(); + } public RepoMappingManifestAction( - ActionOwner owner, - Artifact output, - NestedSet transitivePackages, - NestedSet runfilesArtifacts, - String workspaceName) { + ActionOwner owner, Artifact output, List entries, String workspaceName) { super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /*makeExecutable=*/ false); - this.transitivePackages = transitivePackages; - this.runfilesArtifacts = runfilesArtifacts; + this.entries = + ImmutableList.sortedCopyOf( + comparing((Entry e) -> e.sourceRepo().getName()) + .thenComparing(Entry::targetRepoApparentName), + entries); this.workspaceName = workspaceName; } @@ -86,7 +78,7 @@ public String getMnemonic() { @Override protected String getRawProgressMessage() { - return "Writing repo mapping manifest for " + getOwner().getLabel(); + return "writing repo mapping manifest for " + getOwner().getLabel(); } @Override @@ -96,61 +88,35 @@ protected void computeKey( Fingerprint fp) throws CommandLineExpansionException, EvalException, InterruptedException { fp.addUUID(MY_UUID); - actionKeyContext.addNestedSetToFingerprint(REPO_AND_MAPPING_DIGEST_FN, fp, transitivePackages); - actionKeyContext.addNestedSetToFingerprint(fp, runfilesArtifacts); fp.addString(workspaceName); + for (Entry entry : entries) { + fp.addString(entry.sourceRepo().getName()); + fp.addString(entry.targetRepoApparentName()); + fp.addString(entry.targetRepo().getName()); + } } @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) throws InterruptedException, ExecException { return out -> { - PrintWriter writer = new PrintWriter(out, /* autoFlush= */ false, ISO_8859_1); - - ImmutableSet reposContributingRunfiles = - runfilesArtifacts.toList().stream() - .filter(a -> a.getOwner() != null) - .map(a -> a.getOwner().getRepository()) - .collect(toImmutableSet()); - transitivePackages.toList().stream() - .collect( - toImmutableSortedMap( - comparing(RepositoryName::getName), - pkg -> pkg.getPackageIdentifier().getRepository(), - Package::getRepositoryMapping, - // All packages in a given repository have the same repository mapping, so the - // particular way of resolving duplicates does not matter. - (first, second) -> first)) - .forEach( - (repoName, mapping) -> - writeRepoMapping(writer, reposContributingRunfiles, repoName, mapping)); + PrintWriter writer = new PrintWriter(out, /*autoFlush=*/ false, ISO_8859_1); + for (Entry entry : entries) { + if (entry.targetRepoApparentName().isEmpty()) { + // The apparent repo name can only be empty for the main repo. We skip this line as + // Rlocation paths can't reference an empty apparent name anyway. + continue; + } + // The canonical name of the main repo is the empty string, which is not a valid name for a + // directory, so the "workspace name" is used the name of the directory under the runfiles + // tree for it. + String targetRepoDirectoryName = + entry.targetRepo().isMain() ? workspaceName : entry.targetRepo().getName(); + writer.format( + "%s,%s,%s\n", + entry.sourceRepo().getName(), entry.targetRepoApparentName(), targetRepoDirectoryName); + } writer.flush(); }; } - - private void writeRepoMapping( - PrintWriter writer, - ImmutableSet reposContributingRunfiles, - RepositoryName repoName, - RepositoryMapping repoMapping) { - for (Entry mappingEntry : - ImmutableSortedMap.copyOf(repoMapping.entries()).entrySet()) { - if (mappingEntry.getKey().isEmpty()) { - // The apparent repo name can only be empty for the main repo. We skip this line as - // Rlocation paths can't reference an empty apparent name anyway. - continue; - } - if (!reposContributingRunfiles.contains(mappingEntry.getValue())) { - // We only write entries for repos that actually contribute runfiles. - continue; - } - // The canonical name of the main repo is the empty string, which is not a valid name for a - // directory, so the "workspace name" is used the name of the directory under the runfiles - // tree for it. - String targetRepoDirectoryName = - mappingEntry.getValue().isMain() ? workspaceName : mappingEntry.getValue().getName(); - writer.format( - "%s,%s,%s\n", repoName.getName(), mappingEntry.getKey(), targetRepoDirectoryName); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index b6154a2ed4d4a5..3772cf7cb700da 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -14,20 +14,26 @@ package com.google.devtools.build.lib.analysis; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CommandLine; +import com.google.devtools.build.lib.analysis.RepoMappingManifestAction.Entry; import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.RunUnder; +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.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -35,6 +41,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -551,9 +558,45 @@ private static Artifact createRepoMappingManifestAction( new RepoMappingManifestAction( ruleContext.getActionOwner(), repoMappingManifest, - ruleContext.getTransitivePackagesForRunfileRepoMappingManifest(), - runfiles.getAllArtifacts(), + collectRepoMappings( + Preconditions.checkNotNull( + ruleContext.getTransitivePackagesForRunfileRepoMappingManifest()), + runfiles), ruleContext.getWorkspaceName())); return repoMappingManifest; } + + /** Returns the list of entries (unsorted) that should appear in the repo mapping manifest. */ + private static ImmutableList collectRepoMappings( + NestedSet transitivePackages, Runfiles runfiles) { + // NOTE: It might appear that the flattening of `transitivePackages` is better suited to the + // execution phase rather than here in the analysis phase, but we can't do that since it would + // necessitate storing `transitivePackages` in an action, which breaks skyframe serialization + // since packages cannot be serialized here. + + ImmutableSet reposContributingRunfiles = + runfiles.getAllArtifacts().toList().stream() + .filter(a -> a.getOwner() != null) + .map(a -> a.getOwner().getRepository()) + .collect(toImmutableSet()); + Set seenRepos = new HashSet<>(); + ImmutableList.Builder entries = ImmutableList.builder(); + for (Package pkg : transitivePackages.toList()) { + if (!seenRepos.add(pkg.getPackageIdentifier().getRepository())) { + // Any package from the same repo would have the same repo mapping. + continue; + } + for (Map.Entry repoMappingEntry : + pkg.getRepositoryMapping().entries().entrySet()) { + if (reposContributingRunfiles.contains(repoMappingEntry.getValue())) { + entries.add( + Entry.of( + pkg.getPackageIdentifier().getRepository(), + repoMappingEntry.getKey(), + repoMappingEntry.getValue())); + } + } + } + return entries.build(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index a7d493fe1de797..d89744dfd73bd8 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -361,14 +361,16 @@ java_test( srcs = ["RunfilesRepoMappingManifestTest.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", + "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:repo_mapping_manifest_action", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl", "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", - "//src/main/java/com/google/devtools/build/lib/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/vfs", - "//src/main/java/net/starlark/java/eval", + "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", "//third_party:guava", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java index 377164876ce25a..edb5955f579b64 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.bazel.bzlmod.BazelLockFileFunction; import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleResolutionFunction; @@ -31,10 +30,8 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Injected; -import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; import java.util.Map.Entry; -import net.starlark.java.eval.EvalException; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -98,22 +95,10 @@ public void setupBareBinaryRule() throws Exception { "bare_binary(name='bare_binary')"); } - private RepoMappingManifestAction getRepoMappingManifestActionForTarget(String label) - throws Exception { + private ImmutableList getRepoMappingManifestForTarget(String label) throws Exception { Action action = getGeneratingAction(getRunfilesSupport(label).getRepoMappingManifest()); assertThat(action).isInstanceOf(RepoMappingManifestAction.class); - return (RepoMappingManifestAction) action; - } - - private String computeKey(RepoMappingManifestAction action) - throws CommandLineExpansionException, EvalException, InterruptedException { - Fingerprint fp = new Fingerprint(); - action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp); - return fp.hexDigestAndReset(); - } - - private ImmutableList getRepoMappingManifestForTarget(String label) throws Exception { - return getRepoMappingManifestActionForTarget(label) + return ((RepoMappingManifestAction) action) .newDeterministicWriter(null) .getBytes() .toStringUtf8() @@ -243,82 +228,4 @@ public void runfilesFromToolchain() throws Exception { "tooled_rule~1.0,bare_rule,bare_rule~1.0") .inOrder(); } - - @Test - public void actionRerunsOnRepoMappingChange_workspaceName() throws Exception { - rewriteWorkspace("workspace(name='aaa_ws')"); - scratch.overwriteFile( - "MODULE.bazel", - "module(name='aaa',version='1.0')", - "bazel_dep(name='bare_rule',version='1.0')"); - scratch.overwriteFile( - "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')"); - - RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa"); - - rewriteWorkspace("workspace(name='not_aaa_ws')"); - - RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa"); - assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange)); - } - - @Test - public void actionRerunsOnRepoMappingChange_repoName() throws Exception { - rewriteWorkspace("workspace(name='aaa_ws')"); - scratch.overwriteFile( - "MODULE.bazel", - "module(name='aaa',version='1.0')", - "bazel_dep(name='bare_rule',version='1.0')"); - scratch.overwriteFile( - "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')"); - - RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa"); - - scratch.overwriteFile( - "MODULE.bazel", - "module(name='aaa',version='1.0',repo_name='not_aaa')", - "bazel_dep(name='bare_rule',version='1.0')"); - invalidatePackages(); - - RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa"); - assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange)); - } - - @Test - public void actionRerunsOnRepoMappingChange_newEntry() throws Exception { - rewriteWorkspace("workspace(name='aaa_ws')"); - scratch.overwriteFile( - "MODULE.bazel", - "module(name='aaa',version='1.0')", - "bazel_dep(name='bare_rule',version='1.0')"); - scratch.overwriteFile( - "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')"); - - registry.addModule( - createModuleKey("bbb", "1.0"), - "module(name='bbb',version='1.0')", - "bazel_dep(name='bare_rule',version='1.0')"); - scratch.overwriteFile( - moduleRoot.getRelative("bbb~1.0").getRelative("WORKSPACE").getPathString()); - scratch.overwriteFile(moduleRoot.getRelative("bbb~1.0").getRelative("BUILD").getPathString()); - scratch.overwriteFile( - moduleRoot.getRelative("bbb~1.0").getRelative("def.bzl").getPathString(), "BBB = '1'"); - - RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa"); - - scratch.overwriteFile( - "MODULE.bazel", - "module(name='aaa',version='1.0')", - "bazel_dep(name='bbb',version='1.0')", - "bazel_dep(name='bare_rule',version='1.0')"); - scratch.overwriteFile( - "BUILD", - "load('@bare_rule//:defs.bzl', 'bare_binary')", - "load('@bbb//:def.bzl', 'BBB')", - "bare_binary(name='aaa')"); - invalidatePackages(); - - RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa"); - assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange)); - } }