From 9b3e9fc42caf447eedecb584de3af4da6ad785b3 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 29 Dec 2024 22:22:05 +0100 Subject: [PATCH] Add `--incompatible_compact_repo_mapping` --- .../analysis/RepoMappingManifestAction.java | 117 +++++++++++++----- .../build/lib/analysis/RunfilesSupport.java | 8 +- .../lib/analysis/config/CoreOptions.java | 12 ++ .../devtools/build/lib/rules/python/BUILD | 1 + .../build/lib/rules/python/PyBuiltins.java | 8 +- .../common/builtin_exec_platforms.bzl | 1 + .../google/devtools/build/lib/analysis/BUILD | 4 + .../RunfilesRepoMappingManifestTest.java | 103 ++++++++++++++- 8 files changed, 217 insertions(+), 37 deletions(-) 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 6d903571edc1ef..be949c7562bd3e 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 @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; @@ -45,9 +46,11 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.PrintWriter; +import java.util.AbstractMap; import java.util.IdentityHashMap; import java.util.Map.Entry; import java.util.UUID; +import java.util.stream.Stream; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -95,6 +98,7 @@ public final class RepoMappingManifestAction extends AbstractFileWriteAction private final boolean hasRunfilesSymlinks; private final NestedSet runfilesRootSymlinks; private final String workspaceName; + private final boolean emitCompactRepoMapping; public RepoMappingManifestAction( ActionOwner owner, @@ -103,13 +107,15 @@ public RepoMappingManifestAction( NestedSet runfilesArtifacts, NestedSet runfilesSymlinks, NestedSet runfilesRootSymlinks, - String workspaceName) { + String workspaceName, + boolean emitCompactRepoMapping) { super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.transitivePackages = transitivePackages; this.runfilesArtifacts = runfilesArtifacts; this.hasRunfilesSymlinks = !runfilesSymlinks.isEmpty(); this.runfilesRootSymlinks = runfilesRootSymlinks; this.workspaceName = workspaceName; + this.emitCompactRepoMapping = emitCompactRepoMapping; } @Override @@ -134,6 +140,7 @@ protected void computeKey( fp.addBoolean(hasRunfilesSymlinks); actionKeyContext.addNestedSetToFingerprint(FIRST_SEGMENT_FN, fp, runfilesRootSymlinks); fp.addString(workspaceName); + fp.addBoolean(emitCompactRepoMapping); } /** @@ -189,41 +196,73 @@ public DeterministicWriter newDeterministicWriter() { // All packages in a given repository have the same repository mapping, so the // particular way of resolving duplicates does not matter. (first, second) -> first)); - // All repositories generated by a module extension have the same Map instance as the entries - // of their RepositoryMapping, with every repo appearing as an entry. If a module extension - // generates N repos and all of them are in transitivePackages, iterating over the packages - // and then over each mapping's entries would thus require time quadratic in N. We prevent - // this by caching the relevant (target apparent name, target canonical name) pairs per entry - // map instance. - IdentityHashMap< - ImmutableMap, ImmutableList>> - cachedRelevantEntries = new IdentityHashMap<>(); - for (var repoAndMapping : sortedRepoMappings.entrySet()) { - cachedRelevantEntries - .computeIfAbsent( - repoAndMapping.getValue().entries(), - entries -> computeRelevantEntries(reposInRunfilesPaths, entries)) - .forEach( - mappingEntry -> { - // 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", - repoAndMapping.getKey().getName(), - mappingEntry.getKey(), - targetRepoDirectoryName); - }); + if (emitCompactRepoMapping) { + Entry lastEntry = null; + boolean reused = false; + ImmutableSet> sentinel = + ImmutableSet.of( + new AbstractMap.SimpleImmutableEntry<>( + RepositoryName.MAIN, + // The entries of this repo mapping never equal any real repo mapping's entries + // (never empty). + RepositoryMapping.create(ImmutableMap.of(), RepositoryName.MAIN))); + for (var repoAndMapping : Iterables.concat(sortedRepoMappings.entrySet(), sentinel)) { + // Module extension repos (and only those repos) all have reference equal repo mapping + // entries due to ModuleExtensionRepoMappingEntriesFunction. We only emit these identical + // mappings once, with a prefix match for the source repo name. + if (lastEntry != null + && repoAndMapping.getValue().entries() == lastEntry.getValue().entries()) { + reused = true; + } else { + if (lastEntry != null) { + String sourceRepo = lastEntry.getKey().getName(); + String prefix; + if (reused) { + prefix = sourceRepo.substring(0, sourceRepo.lastIndexOf('+') + 1) + "*"; + } else { + prefix = sourceRepo; + } + computeRelevantEntries(reposInRunfilesPaths, lastEntry.getValue().entries()) + .forEach( + mappingEntry -> + writeEntry( + writer, prefix, mappingEntry.getKey(), mappingEntry.getValue())); + } + reused = false; + lastEntry = repoAndMapping; + } + } + } else { + // All repositories generated by a module extension have the same Map instance as the + // entries of their RepositoryMapping, with every repo appearing as an entry. If a module + // extension generates N repos and all of them are in transitivePackages, iterating over the + // packages and then over each mapping's entries would thus require time quadratic in N. We + // prevent this by caching the relevant (target apparent name, target canonical name) pairs + // per entry map instance. + IdentityHashMap< + ImmutableMap, ImmutableList>> + cachedRelevantEntries = new IdentityHashMap<>(); + for (var repoAndMapping : sortedRepoMappings.entrySet()) { + cachedRelevantEntries + .computeIfAbsent( + repoAndMapping.getValue().entries(), + entries -> + computeRelevantEntries(reposInRunfilesPaths, entries) + .collect(toImmutableList())) + .forEach( + mappingEntry -> + writeEntry( + writer, + repoAndMapping.getKey().getName(), + mappingEntry.getKey(), + mappingEntry.getValue())); + } } writer.flush(); }; } - private ImmutableList> computeRelevantEntries( + private static Stream> computeRelevantEntries( ImmutableSet reposInRunfilesPaths, ImmutableMap mappingEntries) { // TODO: If this becomes a hotspot, consider iterating over reposInRunfilesPaths and looking @@ -238,7 +277,19 @@ private ImmutableList> computeRelevantEntries( .filter(mappingEntry -> !mappingEntry.getKey().isEmpty()) // We only write entries for repos whose canonical names appear in runfiles paths. .filter(entry -> reposInRunfilesPaths.contains(entry.getValue().getName())) - .sorted(Entry.comparingByKey()) - .collect(toImmutableList()); + .sorted(Entry.comparingByKey()); + } + + private void writeEntry( + PrintWriter writer, + String source, + String targetApparentName, + RepositoryName targetCanonicalName) { + // 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 = + targetCanonicalName.isMain() ? workspaceName : targetCanonicalName.getName(); + writer.format("%s,%s,%s\n", source, targetApparentName, 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 3bb810c34f5f46..d8f3a1d1c06eb2 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 @@ -32,6 +32,7 @@ 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.BuildConfigurationValue.RunfileSymlinksMode; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.RunUnder; import com.google.devtools.build.lib.analysis.config.RunUnder.LabelRunUnder; import com.google.devtools.build.lib.analysis.test.TestActionBuilder; @@ -713,7 +714,12 @@ private static Artifact createRepoMappingManifestAction( runfiles.getArtifacts(), runfiles.getSymlinks(), runfiles.getRootSymlinks(), - ruleContext.getWorkspaceName())); + ruleContext.getWorkspaceName(), + ruleContext + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .compactRepoMapping)); return repoMappingManifest; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index b4e663454c365c..58d0231c8df42c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -538,6 +538,18 @@ public ExecConfigurationDistinguisherSchemeConverter() { + "https://bazel.build/extending/rules#runfiles_features_to_avoid).") public boolean alwaysIncludeFilesToBuildInData; + @Option( + name = "incompatible_compact_repo_mapping", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If enabled, the .repo_mapping file emits a module extension's repo mapping " + + "only once instead of once for each repo generated by the extension that " + + "contributes runfiles.") + public boolean compactRepoMapping; + @Option( name = "run_under", defaultValue = "null", diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/BUILD b/src/main/java/com/google/devtools/build/lib/rules/python/BUILD index 69edc59da089de..aa2ed9e50c490d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/python/BUILD @@ -26,6 +26,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters", + "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java index c7677db68b5a5e..e6f25c7b68bb19 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction; import com.google.devtools.build.lib.analysis.actions.DeterministicWriter; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.Depset; @@ -232,7 +233,12 @@ public void repoMappingAction( runfiles.getArtifacts(), runfiles.getSymlinks(), runfiles.getRootSymlinks(), - ruleContext.getWorkspaceName())); + ruleContext.getWorkspaceName(), + ruleContext + .getConfiguration() + .getOptions() + .get(CoreOptions.class) + .compactRepoMapping)); } @StarlarkMethod( diff --git a/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl b/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl index daa0179f8274a4..fd0aa25a1e65e6 100644 --- a/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl +++ b/src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl @@ -274,6 +274,7 @@ bazel_fragments["CoreOptions"] = fragment( "//command_line_option:experimental_inprocess_symlink_creation", "//command_line_option:experimental_throttle_action_cache_check", "//command_line_option:experimental_use_platforms_in_output_dir_legacy_heuristic", + "//command_line_option:incompatible_compact_repo_mapping", ], inputs = ["//command_line_option:features"], outputs = [ 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 bc346ab74306d7..7752cd2851140d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -353,9 +353,11 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:repo_mapping_manifest_action", "//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/bazel/repository/starlark", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_executor_repository_helpers_holder", + "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", @@ -363,9 +365,11 @@ java_test( "//src/main/java/net/starlark/java/syntax", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", + "//src/test/java/com/google/devtools/build/lib/testutil", "//third_party:guava", "//third_party:junit4", "//third_party:truth", + "@maven//:com_google_testparameterinjector_test_parameter_injector", ], ) 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 4d9d3104872a91..1e2c336fc75bcd 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 @@ -22,21 +22,39 @@ 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.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryDirtinessChecker; import com.google.devtools.build.lib.skyframe.SkyframeExecutorRepositoryHelpersHolder; +import com.google.devtools.build.lib.starlarkbuildapi.repository.RepositoryBootstrap; +import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.util.Map.Entry; import net.starlark.java.eval.EvalException; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; /** Tests that the repo mapping manifest file is properly generated for runfiles. */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public class RunfilesRepoMappingManifestTest extends BuildViewTestCase { + private ConfiguredRuleClassProvider ruleProvider = null; + + @Override + protected ConfiguredRuleClassProvider createRuleClassProvider() { + // We inject the repository module in our test rule class provider. + if (ruleProvider == null) { + ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder(); + TestRuleClassProvider.addStandardRules(builder); + builder.addStarlarkBootstrap(new RepositoryBootstrap(new StarlarkRepositoryModule())); + ruleProvider = builder.build(); + } + return ruleProvider; + } + @Override protected SkyframeExecutorRepositoryHelpersHolder getRepositoryHelpersHolder() { // Transitive packages are needed for RepoMappingManifestAction and are only stored when @@ -417,6 +435,87 @@ def _get_repo_mapping_impl(ctx): .containsExactly(getRunfilesSupport("//:aaa").getRepoMappingManifest()); } + @Test + public void runfilesFromExtension(@TestParameter boolean compactRepoMapping) throws Exception { + if (compactRepoMapping) { + useConfiguration("--incompatible_compact_repo_mapping"); + } else { + useConfiguration("--noincompatible_compact_repo_mapping"); + } + + scratch.overwriteFile( + "MODULE.bazel", + """ + module(name='aaa',version='1.0') + bazel_dep(name='bare_rule',version='1.0') + deps = use_extension('//:deps.bzl', 'deps') + use_repo(deps, dep='dep1') + """); + + scratch.overwriteFile( + "BUILD", + """ + load('@bare_rule//:defs.bzl', 'bare_binary') + bare_binary(name='aaa',data=['@dep//:dep1']) + """); + scratch.overwriteFile( + "deps.bzl", + """ + def _deps_repo_impl(ctx): + ctx.file('BUILD', \""" + load('@bare_rule//:defs.bzl', 'bare_binary') + bare_binary( + name = 'dep' + str({count}), + data = ['@dep' + str({count} + 1)] if {count} < 3 else [], + visibility = ['//visibility:public'] + ) + \""".format(count = ctx.attr.count)) + _deps_repo = repository_rule(_deps_repo_impl, attrs = {'count': attr.int()}) + def _deps_impl(_): + _deps_repo(name = 'dep1', count = 1) + _deps_repo(name = 'dep2', count = 2) + _deps_repo(name = 'dep3', count = 3) + deps = module_extension(_deps_impl) + """); + + // Called last as it triggers package invalidation, which requires a valid MODULE.bazel setup. + invalidatePackages(); + + if (compactRepoMapping) { + assertThat(getRepoMappingManifestForTarget("//:aaa")) + .containsExactly( + ",aaa," + getRuleClassProvider().getRunfilesPrefix(), + ",dep,+deps+dep1", + "+deps+*,aaa," + getRuleClassProvider().getRunfilesPrefix(), + "+deps+*,dep,+deps+dep1", + "+deps+*,dep1,+deps+dep1", + "+deps+*,dep2,+deps+dep2", + "+deps+*,dep3,+deps+dep3") + .inOrder(); + } else { + assertThat(getRepoMappingManifestForTarget("//:aaa")) + .containsExactly( + ",aaa," + getRuleClassProvider().getRunfilesPrefix(), + ",dep,+deps+dep1", + "+deps+dep1,aaa," + getRuleClassProvider().getRunfilesPrefix(), + "+deps+dep1,dep,+deps+dep1", + "+deps+dep1,dep1,+deps+dep1", + "+deps+dep1,dep2,+deps+dep2", + "+deps+dep1,dep3,+deps+dep3", + "+deps+dep2,aaa," + getRuleClassProvider().getRunfilesPrefix(), + "+deps+dep2,dep,+deps+dep1", + "+deps+dep2,dep1,+deps+dep1", + "+deps+dep2,dep2,+deps+dep2", + "+deps+dep2,dep3,+deps+dep3", + "+deps+dep3,aaa," + getRuleClassProvider().getRunfilesPrefix(), + "+deps+dep3,dep,+deps+dep1", + "+deps+dep3,dep1,+deps+dep1", + "+deps+dep3,dep2,+deps+dep2", + "+deps+dep3,dep3,+deps+dep3") + .inOrder(); + } + } + /** * Similar to {@link BuildViewTestCase#rewriteWorkspace(String...)}, but does not call {@link * BuildViewTestCase#invalidatePackages()}.