From ac38dd0285e17ffbdb766b9dcaa484dc02605adc Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 09:29:27 +0200 Subject: [PATCH 01/19] WIP --- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 43 +++++++++++++++++++ .../lib/bazel/bzlmod/ModuleThreadContext.java | 6 +++ 2 files changed, 49 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index a0942ea77add66..2401d100888744 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -548,6 +548,11 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio proxyBuilder.addImport(localRepoName, exportedName); } + void addOverride(String localRepoName, String exportedName, Location location) + throws EvalException { + usageBuilder.addOverride(localRepoName, exportedName, location); + } + class TagCallable implements StarlarkValue { final String tagName; @@ -637,6 +642,44 @@ public void useRepo( } } + @StarlarkMethod( + name = "override_repo", + doc = + "Imports one or more repos generated by the given module extension into the scope of the" + + " current module.", + parameters = { + @Param( + name = "extension_proxy", + doc = "A module extension proxy object returned by a use_extension call."), + }, + extraPositionals = @Param(name = "args", doc = "The names of the repos to import."), + extraKeywords = + @Param( + name = "kwargs", + doc = + "Specifies certain repos to import into the scope of the current module with" + + " different names. The keys should be the name to use in the current scope," + + " whereas the values should be the original names exported by the module" + + " extension."), + useStarlarkThread = true) + public void overrideRepo( + ModuleExtensionProxy extensionProxy, + Tuple args, + Dict kwargs, + StarlarkThread thread) + throws EvalException { + ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "override_repo()"); + context.setNonModuleCalled(); + Location location = thread.getCallerLocation(); + for (String arg : Sequence.cast(args, String.class, "args")) { + extensionProxy.addOverride(arg, arg, location); + } + for (Map.Entry entry : + Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) { + extensionProxy.addOverride(entry.getKey(), entry.getValue(), location); + } + } + @StarlarkMethod( name = "use_repo_rule", doc = diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index a2cf0193451e08..4c5feb257994bf 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -171,6 +171,12 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio imports.put(localRepoName, exportedName); } + public void addOverride(String localRepoName, String exportedName, Location location) throws EvalException { + RepositoryName.validateUserProvidedRepoName(localRepoName); + RepositoryName.validateUserProvidedRepoName(exportedName); + context.add + } + void addTag(Tag tag) { tags.add(tag); } From e1155298086741486778b92950c91ee9f3c5b7a1 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 19:51:51 +0200 Subject: [PATCH 02/19] WIP --- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 9 ++++-- .../lib/bazel/bzlmod/ModuleThreadContext.java | 28 ++++++++++++++++--- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 2401d100888744..4518db1dbc0df0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -548,9 +548,9 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio proxyBuilder.addImport(localRepoName, exportedName); } - void addOverride(String localRepoName, String exportedName, Location location) + void addOverride(String extensionLocalName, String moduleLocalName, Location location) throws EvalException { - usageBuilder.addOverride(localRepoName, exportedName, location); + usageBuilder.addRepoOverride(extensionLocalName, moduleLocalName, location); } class TagCallable implements StarlarkValue { @@ -670,6 +670,11 @@ public void overrideRepo( throws EvalException { ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "override_repo()"); context.setNonModuleCalled(); + if (context.shouldIgnoreDevDeps()) { + // Ignore calls early as they may refer to repos that are dev dependencies (or this is not the + // root module). + return; + } Location location = thread.getCallerLocation(); for (String arg : Sequence.cast(args, String.class, "args")) { extensionProxy.addOverride(arg, arg, location); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 4c5feb257994bf..6bb65ccb400b9b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -121,12 +121,16 @@ List getExtensionUsageBuilders() { } static class ModuleExtensionUsageBuilder { + + private record ApparentNameAndLocation(String moduleLocalName, Location location) {} + private final ModuleThreadContext context; private final String extensionBzlFile; private final String extensionName; private final boolean isolate; private final ArrayList proxyBuilders; private final HashBiMap imports; + private final Map overrides; private final ImmutableList.Builder tags; ModuleExtensionUsageBuilder( @@ -140,6 +144,7 @@ static class ModuleExtensionUsageBuilder { this.isolate = isolate; this.proxyBuilders = new ArrayList<>(); this.imports = HashBiMap.create(); + this.overrides = HashBiMap.create(); this.tags = ImmutableList.builder(); } @@ -171,10 +176,21 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio imports.put(localRepoName, exportedName); } - public void addOverride(String localRepoName, String exportedName, Location location) throws EvalException { - RepositoryName.validateUserProvidedRepoName(localRepoName); - RepositoryName.validateUserProvidedRepoName(exportedName); - context.add + public void addRepoOverride(String extensionLocalName, String apparentName, Location location) + throws EvalException { + RepositoryName.validateUserProvidedRepoName(extensionLocalName); + RepositoryName.validateUserProvidedRepoName(apparentName); + if (overrides.containsKey(extensionLocalName)) { + var collision = overrides.get(extensionLocalName); + throw Starlark.errorf( + "The repo '%s' defined by extension '%s' from %s is already overridden with '%s' at %s", + extensionLocalName, + extensionBzlFile, + extensionName, + collision.moduleLocalName, + collision.location); + } + overrides.put(extensionLocalName, new ApparentNameAndLocation(apparentName, location)); } void addTag(Tag tag) { @@ -205,6 +221,10 @@ ModuleExtensionUsage buildUsage() throws EvalException { } return builder.build(); } + + ImmutableMap buildRepoOverrides() { + + } } public void include(String includeLabel, StarlarkThread thread) From d27f6d40c38d8e2632b1e7e566fc22c26571ccf7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 20:20:26 +0200 Subject: [PATCH 03/19] WIP --- .../lib/bazel/bzlmod/ModuleThreadContext.java | 47 +++++++++++++++---- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 6bb65ccb400b9b..bc4845f4f3744a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.bazel.bzlmod.InterimModule.DepSpec; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; @@ -122,7 +123,7 @@ List getExtensionUsageBuilders() { static class ModuleExtensionUsageBuilder { - private record ApparentNameAndLocation(String moduleLocalName, Location location) {} + private record ApparentNameAndLocation(String apparentName, Location location) {} private final ModuleThreadContext context; private final String extensionBzlFile; @@ -183,12 +184,8 @@ public void addRepoOverride(String extensionLocalName, String apparentName, Loca if (overrides.containsKey(extensionLocalName)) { var collision = overrides.get(extensionLocalName); throw Starlark.errorf( - "The repo '%s' defined by extension '%s' from %s is already overridden with '%s' at %s", - extensionLocalName, - extensionBzlFile, - extensionName, - collision.moduleLocalName, - collision.location); + "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at %s", + extensionLocalName, extensionName, collision.apparentName, collision.location); } overrides.put(extensionLocalName, new ApparentNameAndLocation(apparentName, location)); } @@ -222,8 +219,31 @@ ModuleExtensionUsage buildUsage() throws EvalException { return builder.build(); } - ImmutableMap buildRepoOverrides() { - + ImmutableMap buildRepoOverrides() throws EvalException { + for (var override : overrides.entrySet()) { + String extensionLocalName = override.getKey(); + String apparentName = override.getValue().apparentName; + if (!context.repoNameUsages.containsKey(apparentName)) { + throw Starlark.errorf( + "The repo exported as '%s' by module extension '%s' is overridden with '%s' at %s, but no repo is imported as '%s'", + extensionLocalName, + extensionName, + apparentName, + override.getValue().location, + apparentName); + } + if (imports.inverse().containsKey(extensionLocalName)) { + throw Starlark.errorf( + "The repo exported as '%s' by module extension '%s' is both overridden with '%s' at %s and imported at %s, which is not supported. Please use '%s' directly instead.", + extensionLocalName, + extensionName, + apparentName, + override.getValue().location, + context.repoNameUsages.get(imports.inverse().get(extensionLocalName)).where, + apparentName); + } + } + return ImmutableMap.copyOf(Maps.transformValues(overrides, v -> v.apparentName)); } } @@ -306,4 +326,13 @@ public ImmutableMap buildOverrides() { } return ImmutableMap.copyOf(overrides); } + + public ImmutableMap> buildRepoOverrides() + throws EvalException { + var result = new LinkedHashMap>(); + for (var extensionUsageBuilder : extensionUsageBuilders) { + result.put(extensionUsageBuilder.buildUsage(), extensionUsageBuilder.buildRepoOverrides()); + } + return ImmutableMap.copyOf(result); + } } From 3b7f1b642a6583291ba949cfbe4b77178bc326d3 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Sep 2024 10:33:40 +0200 Subject: [PATCH 04/19] WIp --- .../bazel/bzlmod/BazelDepGraphFunction.java | 33 ++++++++++++++++++- .../lib/bazel/bzlmod/BazelDepGraphValue.java | 33 +++++++++++++++---- ...leExtensionRepoMappingEntriesFunction.java | 1 + .../bazel/bzlmod/ModuleExtensionUsage.java | 9 +++++ .../lib/bazel/bzlmod/ModuleThreadContext.java | 8 ++--- .../bzlmod/SingleExtensionUsagesFunction.java | 3 +- .../bzlmod/SingleExtensionUsagesValue.java | 11 +++++-- 7 files changed, 82 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index 666b0e86cf7364..546d2fbb4f9ea1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -80,7 +80,12 @@ public SkyValue compute(SkyKey skyKey, Environment env) canonicalRepoNameLookup, depGraph.values().stream().map(AbridgedModule::from).collect(toImmutableList()), extensionUsagesById, - extensionUniqueNames.inverse()); + extensionUniqueNames.inverse(), + resolveRepoOverrides( + depGraph, + extensionUsagesById, + extensionUniqueNames.inverse(), + canonicalRepoNameLookup)); } private static ImmutableTable @@ -198,6 +203,32 @@ private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) + extensionNameDisambiguator); } + private static ImmutableTable resolveRepoOverrides( + ImmutableMap depGraph, + ImmutableTable extensionUsagesTable, + ImmutableMap extensionUniqueNames, + ImmutableBiMap canonicalRepoNameLookup) { + RepositoryMapping rootModuleMapping = + BazelDepGraphValue.getRepositoryMapping( + ModuleKey.ROOT, + depGraph, + extensionUsagesTable, + extensionUniqueNames, + canonicalRepoNameLookup); + ImmutableTable.Builder repoOverridesBuilder = + ImmutableTable.builder(); + for (var extensionId : extensionUsagesTable.rowKeySet()) { + var rootUsage = extensionUsagesTable.row(extensionId).get(ModuleKey.ROOT); + if (rootUsage != null) { + for (var override : rootUsage.getRepoOverrides().entrySet()) { + repoOverridesBuilder.put( + extensionId, override.getKey(), rootModuleMapping.get(override.getValue())); + } + } + } + return repoOverridesBuilder.buildOrThrow(); + } + static class BazelDepGraphFunctionException extends SkyFunctionException { BazelDepGraphFunctionException(ExternalDepsException e, Transience transience) { super(e, transience); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java index 17894f4123a4db..905bc4a1849eb6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java @@ -41,13 +41,15 @@ public static BazelDepGraphValue create( ImmutableMap canonicalRepoNameLookup, ImmutableList abridgedModules, ImmutableTable extensionUsagesTable, - ImmutableMap extensionUniqueNames) { + ImmutableMap extensionUniqueNames, + ImmutableTable repoOverrides) { return new AutoValue_BazelDepGraphValue( depGraph, ImmutableBiMap.copyOf(canonicalRepoNameLookup), abridgedModules, extensionUsagesTable, - extensionUniqueNames); + extensionUniqueNames, + repoOverrides); } public static BazelDepGraphValue createEmptyDepGraph() { @@ -71,7 +73,8 @@ public static BazelDepGraphValue createEmptyDepGraph() { canonicalRepoNameLookup, ImmutableList.of(), ImmutableTable.of(), - ImmutableMap.of()); + ImmutableMap.of(), + ImmutableTable.of()); } /** @@ -103,17 +106,33 @@ public static BazelDepGraphValue createEmptyDepGraph() { */ public abstract ImmutableMap getExtensionUniqueNames(); + public abstract ImmutableTable getRepoOverrides(); + /** * Returns the full {@link RepositoryMapping} for the given module, including repos from Bazel * module deps and module extensions. */ public final RepositoryMapping getFullRepoMapping(ModuleKey key) { + return getRepositoryMapping( + key, + getDepGraph(), + getExtensionUsagesTable(), + getExtensionUniqueNames(), + getCanonicalRepoNameLookup()); + } + + static RepositoryMapping getRepositoryMapping( + ModuleKey key, + ImmutableMap depGraph, + ImmutableTable extensionUsagesTable, + ImmutableMap extensionUniqueNames, + ImmutableBiMap canonicalRepoNameLookup) { ImmutableMap.Builder mapping = ImmutableMap.builder(); for (Map.Entry extIdAndUsage : - getExtensionUsagesTable().column(key).entrySet()) { + extensionUsagesTable.column(key).entrySet()) { ModuleExtensionId extensionId = extIdAndUsage.getKey(); ModuleExtensionUsage usage = extIdAndUsage.getValue(); - String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + "+"; + String repoNamePrefix = extensionUniqueNames.get(extensionId) + "+"; for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) { for (Map.Entry entry : proxy.getImports().entrySet()) { String canonicalRepoName = repoNamePrefix + entry.getValue(); @@ -121,9 +140,9 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) { } } } - return getDepGraph() + return depGraph .get(key) - .getRepoMappingWithBazelDepsOnly(getCanonicalRepoNameLookup().inverse()) + .getRepoMappingWithBazelDepsOnly(canonicalRepoNameLookup.inverse()) .withAdditionalMappings(mapping.buildOrThrow()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java index 9253d38813ea04..8e400bc37093cb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java @@ -74,6 +74,7 @@ private ModuleExtensionRepoMappingEntriesValue computeRepoMappingEntries( ImmutableMap.Builder entries = ImmutableMap.builder(); entries.putAll(bazelDepGraphValue.getFullRepoMapping(moduleKey).entries()); entries.putAll(extensionEvalValue.getCanonicalRepoNameToInternalNames().inverse()); + entries.putAll(bazelDepGraphValue.getRepoOverrides().row(extensionId)); return ModuleExtensionRepoMappingEntriesValue.create(entries.buildKeepingLast(), moduleKey); // LINT.ThenChange(//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java) } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index e5dc76f120e9fa..5e29367336e9bb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -19,6 +19,7 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; @@ -130,6 +131,8 @@ public final boolean getHasNonDevUseExtension() { return getProxies().stream().anyMatch(p -> !p.isDevDependency()); } + public abstract ImmutableMap getRepoOverrides(); + public abstract Builder toBuilder(); public static Builder builder() { @@ -152,6 +155,8 @@ ModuleExtensionUsage trimForEvaluation() { // Extension implementation functions do not see the imports, they are only validated // against the set of generated repos in a validation step that comes afterward. .setProxies(ImmutableList.of()) + // Tracked in SingleExtensionUsagesValue instead, using canonical instead of apparent names. + .setRepoOverrides(ImmutableMap.of()) .build(); } @@ -185,6 +190,10 @@ public Builder addTag(Tag value) { return this; } + @CanIgnoreReturnValue + public abstract Builder setRepoOverrides( + ImmutableMap extensionLocalToApparentName); + public abstract ModuleExtensionUsage build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index bc4845f4f3744a..68e1852e6ddac8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -216,10 +216,7 @@ ModuleExtensionUsage buildUsage() throws EvalException { } else { builder.setIsolationKey(Optional.empty()); } - return builder.build(); - } - ImmutableMap buildRepoOverrides() throws EvalException { for (var override : overrides.entrySet()) { String extensionLocalName = override.getKey(); String apparentName = override.getValue().apparentName; @@ -243,7 +240,10 @@ ImmutableMap buildRepoOverrides() throws EvalException { apparentName); } } - return ImmutableMap.copyOf(Maps.transformValues(overrides, v -> v.apparentName)); + builder.setRepoOverrides( + ImmutableMap.copyOf(Maps.transformValues(overrides, v -> v.apparentName))); + + return builder.build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java index 870eb6c276b39b..4a3ca17e13f372 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java @@ -61,6 +61,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept .collect(toImmutableList()), // TODO(wyv): Maybe cache these mappings? usagesTable.row(id).keySet().stream() - .collect(toImmutableMap(key -> key, bazelDepGraphValue::getFullRepoMapping))); + .collect(toImmutableMap(key -> key, bazelDepGraphValue::getFullRepoMapping)), + bazelDepGraphValue.getRepoOverrides().row(id)); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java index b75e3955b02aa7..6724f1a48a1a70 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java @@ -21,6 +21,7 @@ import com.google.common.collect.Maps; import com.google.common.hash.Hashing; import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -56,13 +57,16 @@ public abstract class SingleExtensionUsagesValue implements SkyValue { /** The repo mappings to use for each module that used this extension. */ public abstract ImmutableMap getRepoMappings(); + public abstract ImmutableMap getRepoOverrides(); + public static SingleExtensionUsagesValue create( ImmutableMap extensionUsages, String extensionUniqueName, ImmutableList abridgedModules, - ImmutableMap repoMappings) { + ImmutableMap repoMappings, + ImmutableMap repoOverrides) { return new AutoValue_SingleExtensionUsagesValue( - extensionUsages, extensionUniqueName, abridgedModules, repoMappings); + extensionUsages, extensionUniqueName, abridgedModules, repoMappings, repoOverrides); } /** @@ -89,7 +93,8 @@ SingleExtensionUsagesValue trimForEvaluation() { // repoMappings: The usage of repo mappings by the extension's implementation function is // tracked on the level of individual entries and all label attributes are provided as // `Label`, which exclusively reference canonical repository names. - ImmutableMap.of()); + ImmutableMap.of(), + getRepoOverrides()); } public static Key key(ModuleExtensionId id) { From 32a8b576ff02888e746634c68d4df23ada76ea92 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Sep 2024 22:22:32 +0200 Subject: [PATCH 05/19] WIP --- .../bazel/bzlmod/BazelDepGraphFunction.java | 6 +- .../lib/bazel/bzlmod/BazelDepGraphValue.java | 15 +- ...uleExtensionEvalStarlarkThreadContext.java | 13 +- .../lib/bazel/bzlmod/ModuleThreadContext.java | 29 ++-- .../bzlmod/RegularRunnableExtension.java | 1 + .../bazel/bzlmod/SingleExtensionFunction.java | 3 +- .../bzlmod/ModuleExtensionResolutionTest.java | 148 ++++++++++++++++++ 7 files changed, 186 insertions(+), 29 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index 546d2fbb4f9ea1..bc6dbe3b2cad25 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -214,7 +214,11 @@ private static ImmutableTable resolve depGraph, extensionUsagesTable, extensionUniqueNames, - canonicalRepoNameLookup); + canonicalRepoNameLookup, + // ModuleFileFunction ensures that repos that override other repos are not themselves + // overridden, so we can safely pass an empty table here instead of resolving chains + // of overrides. + ImmutableTable.of()); ImmutableTable.Builder repoOverridesBuilder = ImmutableTable.builder(); for (var extensionId : extensionUsagesTable.rowKeySet()) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java index 905bc4a1849eb6..9e52712dea6048 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java @@ -118,7 +118,8 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) { getDepGraph(), getExtensionUsagesTable(), getExtensionUniqueNames(), - getCanonicalRepoNameLookup()); + getCanonicalRepoNameLookup(), + getRepoOverrides()); } static RepositoryMapping getRepositoryMapping( @@ -126,7 +127,8 @@ static RepositoryMapping getRepositoryMapping( ImmutableMap depGraph, ImmutableTable extensionUsagesTable, ImmutableMap extensionUniqueNames, - ImmutableBiMap canonicalRepoNameLookup) { + ImmutableBiMap canonicalRepoNameLookup, + ImmutableTable repoOverrides) { ImmutableMap.Builder mapping = ImmutableMap.builder(); for (Map.Entry extIdAndUsage : extensionUsagesTable.column(key).entrySet()) { @@ -135,8 +137,13 @@ static RepositoryMapping getRepositoryMapping( String repoNamePrefix = extensionUniqueNames.get(extensionId) + "+"; for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) { for (Map.Entry entry : proxy.getImports().entrySet()) { - String canonicalRepoName = repoNamePrefix + entry.getValue(); - mapping.put(entry.getKey(), RepositoryName.createUnvalidated(canonicalRepoName)); + RepositoryName defaultCanonicalRepoName = + RepositoryName.createUnvalidated(repoNamePrefix + entry.getValue()); + mapping.put( + entry.getKey(), + repoOverrides + .row(extensionId) + .getOrDefault(entry.getValue(), defaultCanonicalRepoName)); } } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java index 58c43612090a29..1e1ab2c72e7522 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java @@ -71,6 +71,7 @@ record RepoRuleCall( private final String repoPrefix; private final PackageIdentifier basePackageId; private final RepositoryMapping baseRepoMapping; + private final ImmutableMap repoOverrides; private final BlazeDirectories directories; private final ExtendedEventHandler eventHandler; private final Map deferredRepos = new LinkedHashMap<>(); @@ -80,6 +81,7 @@ public ModuleExtensionEvalStarlarkThreadContext( String repoPrefix, PackageIdentifier basePackageId, RepositoryMapping baseRepoMapping, + ImmutableMap repoOverrides, RepositoryMapping mainRepoMapping, BlazeDirectories directories, ExtendedEventHandler eventHandler) { @@ -88,6 +90,7 @@ public ModuleExtensionEvalStarlarkThreadContext( this.repoPrefix = repoPrefix; this.basePackageId = basePackageId; this.baseRepoMapping = baseRepoMapping; + this.repoOverrides = repoOverrides; this.directories = directories; this.eventHandler = eventHandler; } @@ -133,13 +136,15 @@ public ImmutableMap createRepos(StarlarkSemantics starlarkSema // Make it possible to refer to extension repos in the label attributes of another extension // repo. Wrapping a label in Label(...) ensures that it is evaluated with respect to the // containing module's repo mapping instead. - var extensionRepos = + ImmutableMap.Builder entries = ImmutableMap.builder(); + entries.putAll(baseRepoMapping.entries()); + entries.putAll( Maps.asMap( deferredRepos.keySet(), - apparentName -> RepositoryName.createUnvalidated(repoPrefix + apparentName)); + apparentName -> RepositoryName.createUnvalidated(repoPrefix + apparentName))); + entries.putAll(repoOverrides); RepositoryMapping fullRepoMapping = - RepositoryMapping.create(extensionRepos, baseRepoMapping.ownerRepo()) - .withAdditionalMappings(baseRepoMapping); + RepositoryMapping.create(entries.buildKeepingLast(), baseRepoMapping.ownerRepo()); // LINT.ThenChange(//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java) ImmutableMap.Builder repoSpecs = ImmutableMap.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 68e1852e6ddac8..c2d8b2af9e0051 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -229,16 +229,16 @@ ModuleExtensionUsage buildUsage() throws EvalException { override.getValue().location, apparentName); } - if (imports.inverse().containsKey(extensionLocalName)) { - throw Starlark.errorf( - "The repo exported as '%s' by module extension '%s' is both overridden with '%s' at %s and imported at %s, which is not supported. Please use '%s' directly instead.", - extensionLocalName, - extensionName, - apparentName, - override.getValue().location, - context.repoNameUsages.get(imports.inverse().get(extensionLocalName)).where, - apparentName); - } +// if (imports.inverse().containsKey(extensionLocalName)) { +// throw Starlark.errorf( +// "The repo exported as '%s' by module extension '%s' is both overridden with '%s' at %s and imported at %s, which is not supported. Please use '%s' directly instead.", +// extensionLocalName, +// extensionName, +// apparentName, +// override.getValue().location, +// context.repoNameUsages.get(imports.inverse().get(extensionLocalName)).where, +// apparentName); +// } } builder.setRepoOverrides( ImmutableMap.copyOf(Maps.transformValues(overrides, v -> v.apparentName))); @@ -326,13 +326,4 @@ public ImmutableMap buildOverrides() { } return ImmutableMap.copyOf(overrides); } - - public ImmutableMap> buildRepoOverrides() - throws EvalException { - var result = new LinkedHashMap>(); - for (var extensionUsageBuilder : extensionUsageBuilders) { - result.put(extensionUsageBuilder.buildUsage(), extensionUsageBuilder.buildRepoOverrides()); - } - return ImmutableMap.copyOf(result); - } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java index d0ed560bf7568d..962ea1e03018a1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java @@ -256,6 +256,7 @@ private RunModuleExtensionResult runInternal( usagesValue.getExtensionUniqueName() + "+", extensionId.getBzlFileLabel().getPackageIdentifier(), BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(), + usagesValue.getRepoOverrides(), mainRepositoryMapping, directories, env.getListener()); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java index d0a8e98c8dc51d..49c8a6e4e7188c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java @@ -52,7 +52,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) { for (Entry repoImport : proxy.getImports().entrySet()) { - if (!evalOnlyValue.getGeneratedRepoSpecs().containsKey(repoImport.getValue())) { + if (!evalOnlyValue.getGeneratedRepoSpecs().containsKey(repoImport.getValue()) + && !usagesValue.getRepoOverrides().containsKey(repoImport.getValue())) { throw new SingleExtensionFunctionException( ExternalDepsException.withMessage( Code.INVALID_EXTENSION_IMPORT, diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 5ad8da2c8a2101..75766b7ef81beb 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -3098,4 +3098,152 @@ def _other_ext_impl(ctx): "@@+other_ext+other_bar//:bar") .inOrder(); } + + @Test + public void overrideRepo_replace() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + """ + bazel_dep(name = "data_repo", version = "1.0") + ext = use_extension("//:defs.bzl","ext") + use_repo(ext, "bar", module_foo = "foo") + data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") + data_repo(name = "override", data = "overridden_data") + override_repo(ext, foo = "override") + """); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + """ + load("@bar//:list.bzl", _bar_list = "list") + load("@override//:data.bzl", _override_data = "data") + load("@module_foo//:data.bzl", _foo_data = "data") + bar_list = _bar_list + foo_data = _foo_data + override_data = _override_data + """); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + """ + load("@data_repo//:defs.bzl", "data_repo") + def _list_repo_impl(ctx): + ctx.file("WORKSPACE") + ctx.file("BUILD") + labels = [str(Label(l)) for l in ctx.attr.labels] + labels += [str(Label("@module_foo//:target3"))] + ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") + list_repo = repository_rule( + implementation = _list_repo_impl, + attrs = { + "labels": attr.label_list(), + }, + ) + def _fail_repo_impl(ctx): + fail("This rule should not be evaluated") + fail_repo = repository_rule(implementation = _fail_repo_impl) + def _ext_impl(ctx): + fail_repo(name = "foo") + list_repo( + name = "bar", + labels = [ + # lazy extension implementation function repository mapping + "@foo//:target1", + # module repo repository mapping + "@module_foo//:target2", + ], + ) + ext = module_extension(implementation = _ext_impl) + """); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat((List) result.get(skyKey).getModule().getGlobal("bar_list")) + .containsExactly( + "@@+_repo_rules+override//:target1", + "@@+_repo_rules+override//:target2", + "@@+_repo_rules+override//:target3", + "@@+_repo_rules+override//:target4") + .inOrder(); + Object overrideData = result.get(skyKey).getModule().getGlobal("override_data"); + assertThat(overrideData).isInstanceOf(String.class); + assertThat(overrideData).isEqualTo("overridden_data"); + Object fooData = result.get(skyKey).getModule().getGlobal("foo_data"); + assertThat(fooData).isSameInstanceAs(overrideData); + } + + @Test + public void overrideRepo_add() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + """ + bazel_dep(name = "data_repo", version = "1.0") + ext = use_extension("//:defs.bzl","ext") + use_repo(ext, "bar", module_foo = "foo") + data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") + data_repo(name = "override", data = "overridden_data") + override_repo(ext, foo = "override") + """); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + """ + load("@bar//:list.bzl", _bar_list = "list") + load("@override//:data.bzl", _override_data = "data") + load("@module_foo//:data.bzl", _foo_data = "data") + bar_list = _bar_list + foo_data = _foo_data + override_data = _override_data + """); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + """ + load("@data_repo//:defs.bzl", "data_repo") + def _list_repo_impl(ctx): + ctx.file("WORKSPACE") + ctx.file("BUILD") + labels = [str(Label(l)) for l in ctx.attr.labels] + labels += [str(Label("@module_foo//:target3"))] + ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") + list_repo = repository_rule( + implementation = _list_repo_impl, + attrs = { + "labels": attr.label_list(), + }, + ) + def _ext_impl(ctx): + list_repo( + name = "bar", + labels = [ + # lazy extension implementation function repository mapping + "@foo//:target1", + # module repo repository mapping + "@module_foo//:target2", + ], + ) + ext = module_extension(implementation = _ext_impl) + """); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat((List) result.get(skyKey).getModule().getGlobal("bar_list")) + .containsExactly( + "@@+_repo_rules+override//:target1", + "@@+_repo_rules+override//:target2", + "@@+_repo_rules+override//:target3", + "@@+_repo_rules+override//:target4") + .inOrder(); + Object overrideData = result.get(skyKey).getModule().getGlobal("override_data"); + assertThat(overrideData).isInstanceOf(String.class); + assertThat(overrideData).isEqualTo("overridden_data"); + Object fooData = result.get(skyKey).getModule().getGlobal("foo_data"); + assertThat(fooData).isSameInstanceAs(overrideData); + } } From d1b0d731a8a3bbacf0de4e60f54013b0463c60e9 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Sep 2024 22:51:08 +0200 Subject: [PATCH 06/19] Add failure tests --- .../lib/bazel/bzlmod/ModuleFileFunction.java | 4 +- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 13 +++-- .../lib/bazel/bzlmod/ModuleThreadContext.java | 27 +++++----- .../bzlmod/ModuleExtensionResolutionTest.java | 26 +++++----- .../bazel/bzlmod/ModuleFileFunctionTest.java | 49 +++++++++++++++++++ 5 files changed, 86 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java index 77173e27108636..c9408c61e02fd2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java @@ -435,7 +435,7 @@ public static RootModuleFileValue evaluateRootModuleFile( try { module = moduleThreadContext.buildModule(/* registry= */ null); } catch (EvalException e) { - eventHandler.handle(Event.error(e.getMessageWithStack())); + eventHandler.handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack())); throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for the root module"); } for (ModuleExtensionUsage usage : module.getExtensionUsages()) { @@ -522,7 +522,7 @@ private static ModuleThreadContext execModuleFile( }); compiledRootModuleFile.runOnThread(thread); } catch (EvalException e) { - eventHandler.handle(Event.error(e.getMessageWithStack())); + eventHandler.handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack())); throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey); } return context; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 4518db1dbc0df0..c3b530746756a4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -548,9 +548,12 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio proxyBuilder.addImport(localRepoName, exportedName); } - void addOverride(String extensionLocalName, String moduleLocalName, Location location) + void addOverride( + String extensionLocalName, + String moduleLocalName, + ImmutableList stack) throws EvalException { - usageBuilder.addRepoOverride(extensionLocalName, moduleLocalName, location); + usageBuilder.addRepoOverride(extensionLocalName, moduleLocalName, stack); } class TagCallable implements StarlarkValue { @@ -675,13 +678,13 @@ public void overrideRepo( // root module). return; } - Location location = thread.getCallerLocation(); + ImmutableList stack = thread.getCallStack(); for (String arg : Sequence.cast(args, String.class, "args")) { - extensionProxy.addOverride(arg, arg, location); + extensionProxy.addOverride(arg, arg, stack); } for (Map.Entry entry : Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) { - extensionProxy.addOverride(entry.getKey(), entry.getValue(), location); + extensionProxy.addOverride(entry.getKey(), entry.getValue(), stack); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index c2d8b2af9e0051..a617dd8463edb5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -123,7 +123,8 @@ List getExtensionUsageBuilders() { static class ModuleExtensionUsageBuilder { - private record ApparentNameAndLocation(String apparentName, Location location) {} + private record ApparentNameAndStack( + String apparentName, ImmutableList stack) {} private final ModuleThreadContext context; private final String extensionBzlFile; @@ -131,7 +132,7 @@ private record ApparentNameAndLocation(String apparentName, Location location) { private final boolean isolate; private final ArrayList proxyBuilders; private final HashBiMap imports; - private final Map overrides; + private final Map overrides; private final ImmutableList.Builder tags; ModuleExtensionUsageBuilder( @@ -177,7 +178,10 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio imports.put(localRepoName, exportedName); } - public void addRepoOverride(String extensionLocalName, String apparentName, Location location) + public void addRepoOverride( + String extensionLocalName, + String apparentName, + ImmutableList stack) throws EvalException { RepositoryName.validateUserProvidedRepoName(extensionLocalName); RepositoryName.validateUserProvidedRepoName(apparentName); @@ -185,9 +189,13 @@ public void addRepoOverride(String extensionLocalName, String apparentName, Loca var collision = overrides.get(extensionLocalName); throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at %s", - extensionLocalName, extensionName, collision.apparentName, collision.location); + extensionLocalName, + extensionName, + collision.apparentName, + // Skip over the override_repo frame. + collision.stack.reverse().get(1).location); } - overrides.put(extensionLocalName, new ApparentNameAndLocation(apparentName, location)); + overrides.put(extensionLocalName, new ApparentNameAndStack(apparentName, stack)); } void addTag(Tag tag) { @@ -222,12 +230,9 @@ ModuleExtensionUsage buildUsage() throws EvalException { String apparentName = override.getValue().apparentName; if (!context.repoNameUsages.containsKey(apparentName)) { throw Starlark.errorf( - "The repo exported as '%s' by module extension '%s' is overridden with '%s' at %s, but no repo is imported as '%s'", - extensionLocalName, - extensionName, - apparentName, - override.getValue().location, - apparentName); + "The repo exported as '%s' by module extension '%s' is overridden with '%s', but no repo is imported under this name", + extensionLocalName, extensionName, apparentName) + .withCallStack(override.getValue().stack); } // if (imports.inverse().containsKey(extensionLocalName)) { // throw Starlark.errorf( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 75766b7ef81beb..4aa9eeb9c28044 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -3184,19 +3184,17 @@ public void overrideRepo_add() throws Exception { ext = use_extension("//:defs.bzl","ext") use_repo(ext, "bar", module_foo = "foo") data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") - data_repo(name = "override", data = "overridden_data") - override_repo(ext, foo = "override") + data_repo(name = "foo", data = "overridden_data") + override_repo(ext, "foo") """); scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); scratch.file( workspaceRoot.getRelative("data.bzl").getPathString(), """ load("@bar//:list.bzl", _bar_list = "list") - load("@override//:data.bzl", _override_data = "data") - load("@module_foo//:data.bzl", _foo_data = "data") + load("@foo//:data.bzl", _foo_data = "data") bar_list = _bar_list foo_data = _foo_data - override_data = _override_data """); scratch.file( workspaceRoot.getRelative("defs.bzl").getPathString(), @@ -3206,7 +3204,7 @@ def _list_repo_impl(ctx): ctx.file("WORKSPACE") ctx.file("BUILD") labels = [str(Label(l)) for l in ctx.attr.labels] - labels += [str(Label("@module_foo//:target3"))] + labels += [str(Label("@foo//:target3"))] ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") list_repo = repository_rule( implementation = _list_repo_impl, @@ -3221,7 +3219,7 @@ def _ext_impl(ctx): # lazy extension implementation function repository mapping "@foo//:target1", # module repo repository mapping - "@module_foo//:target2", + Label("@foo//:target2"), ], ) ext = module_extension(implementation = _ext_impl) @@ -3235,15 +3233,13 @@ def _ext_impl(ctx): } assertThat((List) result.get(skyKey).getModule().getGlobal("bar_list")) .containsExactly( - "@@+_repo_rules+override//:target1", - "@@+_repo_rules+override//:target2", - "@@+_repo_rules+override//:target3", - "@@+_repo_rules+override//:target4") + "@@+_repo_rules+foo//:target1", + "@@+_repo_rules+foo//:target2", + "@@+_repo_rules+foo//:target3", + "@@+_repo_rules+foo//:target4") .inOrder(); - Object overrideData = result.get(skyKey).getModule().getGlobal("override_data"); - assertThat(overrideData).isInstanceOf(String.class); - assertThat(overrideData).isEqualTo("overridden_data"); Object fooData = result.get(skyKey).getModule().getGlobal("foo_data"); - assertThat(fooData).isSameInstanceAs(overrideData); + assertThat(fooData).isInstanceOf(String.class); + assertThat(fooData).isEqualTo("overridden_data"); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index e8296c2c6abade..18e33ee543498e 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -1705,4 +1705,53 @@ public void testInvalidUseExtensionLabel() throws Exception { + " name 'foo/bar:extensions.bzl': repo names may contain only A-Z, a-z, 0-9, '-'," + " '_', '.' and '+'"); } + + @Test + public void testOverrideRepo_overridingRepoDoesntExist() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + ext = use_extension('//:defs.bzl', 'ext') + override_repo(ext, 'foo') + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:3:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 3, column 14, in + Error in override_repo: The repo exported as 'foo' by module extension 'ext' is overridden with 'foo', but no repo is imported under this name"""); + } + + @Test + public void testOverrideRepo_duplicateOverride() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + bazel_dep(name = "override1", version = "1.0") + bazel_dep(name = "override2", version = "1.0") + ext = use_extension('//:defs.bzl', 'ext') + override_repo(ext, foo = "override1") + override_repo(ext, foo = "override2") + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:6:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 6, column 14, in + Error in override_repo: The repo exported as 'foo' by module extension 'ext' is already overridden with 'override1' at /workspace/MODULE.bazel:5:14"""); + } } From e310f9b00aa02b6f20854bc4ee55628990277892 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 6 Sep 2024 09:41:34 +0200 Subject: [PATCH 07/19] Detect cycles --- .../lib/bazel/bzlmod/BazelDepGraphValue.java | 4 + .../bazel/bzlmod/ModuleExtensionUsage.java | 9 +- .../lib/bazel/bzlmod/ModuleThreadContext.java | 109 ++++++++++++------ .../bazel/bzlmod/ModuleFileFunctionTest.java | 105 +++++++++++++++++ 4 files changed, 192 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java index 9e52712dea6048..011202e9ad913a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java @@ -106,6 +106,10 @@ public static BazelDepGraphValue createEmptyDepGraph() { */ public abstract ImmutableMap getExtensionUniqueNames(); + /** + * For each module extension, a mapping from the name of the repo exported by the extension to the + * canonical repo name of the repo that should override it (if any). + */ public abstract ImmutableTable getRepoOverrides(); /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index 5e29367336e9bb..ab545db013c23d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -131,12 +131,19 @@ public final boolean getHasNonDevUseExtension() { return getProxies().stream().anyMatch(p -> !p.isDevDependency()); } + /** + * Maps repo names local to the extension to the apparent name in the using module of the repo + * they are overridden with. + * + *

This is only non-empty for root module usages and repos that override other repos are not + * themselves overridden. + */ public abstract ImmutableMap getRepoOverrides(); public abstract Builder toBuilder(); public static Builder builder() { - return new AutoValue_ModuleExtensionUsage.Builder(); + return new AutoValue_ModuleExtensionUsage.Builder().setRepoOverrides(ImmutableMap.of()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index a617dd8463edb5..78b7ed79fc24d5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -55,6 +55,11 @@ public class ModuleThreadContext extends StarlarkThreadContext { private final Map overrides = new LinkedHashMap<>(); private final Map repoNameUsages = new HashMap<>(); + private final Map reposSubjectToRepoOverride = + new HashMap<>(); + private final Map reposUsedAsRepoOverride = + new HashMap<>(); + public static ModuleThreadContext fromOrFail(StarlarkThread thread, String what) throws EvalException { StarlarkThreadContext context = thread.getThreadLocal(StarlarkThreadContext.class); @@ -123,8 +128,16 @@ List getExtensionUsageBuilders() { static class ModuleExtensionUsageBuilder { - private record ApparentNameAndStack( - String apparentName, ImmutableList stack) {} + private record RepoOverride( + String exportedName, + String localRepoName, + ModuleExtensionUsageBuilder extensionUsageBuilder, + ImmutableList stack) { + Location location() { + // Skip over the override_repo builtin frame. + return stack.reverse().get(1).location; + } + } private final ModuleThreadContext context; private final String extensionBzlFile; @@ -132,7 +145,7 @@ private record ApparentNameAndStack( private final boolean isolate; private final ArrayList proxyBuilders; private final HashBiMap imports; - private final Map overrides; + private final Map repoOverrides; private final ImmutableList.Builder tags; ModuleExtensionUsageBuilder( @@ -146,7 +159,7 @@ private record ApparentNameAndStack( this.isolate = isolate; this.proxyBuilders = new ArrayList<>(); this.imports = HashBiMap.create(); - this.overrides = HashBiMap.create(); + this.repoOverrides = HashBiMap.create(); this.tags = ImmutableList.builder(); } @@ -179,23 +192,20 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio } public void addRepoOverride( - String extensionLocalName, - String apparentName, + String exportedName, + String localRepoName, ImmutableList stack) throws EvalException { - RepositoryName.validateUserProvidedRepoName(extensionLocalName); - RepositoryName.validateUserProvidedRepoName(apparentName); - if (overrides.containsKey(extensionLocalName)) { - var collision = overrides.get(extensionLocalName); + RepositoryName.validateUserProvidedRepoName(exportedName); + RepositoryName.validateUserProvidedRepoName(localRepoName); + if (repoOverrides.containsKey(exportedName)) { + var collision = repoOverrides.get(exportedName); throw Starlark.errorf( - "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at %s", - extensionLocalName, - extensionName, - collision.apparentName, - // Skip over the override_repo frame. - collision.stack.reverse().get(1).location); + "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at" + + " %s", + exportedName, extensionName, collision.localRepoName, collision.location()); } - overrides.put(extensionLocalName, new ApparentNameAndStack(apparentName, stack)); + repoOverrides.put(exportedName, new RepoOverride(exportedName, localRepoName, this, stack)); } void addTag(Tag tag) { @@ -225,28 +235,24 @@ ModuleExtensionUsage buildUsage() throws EvalException { builder.setIsolationKey(Optional.empty()); } - for (var override : overrides.entrySet()) { - String extensionLocalName = override.getKey(); - String apparentName = override.getValue().apparentName; - if (!context.repoNameUsages.containsKey(apparentName)) { + for (var override : repoOverrides.entrySet()) { + String exportedName = override.getKey(); + String localRepoName = override.getValue().localRepoName; + if (!context.repoNameUsages.containsKey(localRepoName)) { throw Starlark.errorf( - "The repo exported as '%s' by module extension '%s' is overridden with '%s', but no repo is imported under this name", - extensionLocalName, extensionName, apparentName) + "The repo exported as '%s' by module extension '%s' is overridden with '%s', but" + + " no repo is imported under this name", + exportedName, extensionName, localRepoName) .withCallStack(override.getValue().stack); } -// if (imports.inverse().containsKey(extensionLocalName)) { -// throw Starlark.errorf( -// "The repo exported as '%s' by module extension '%s' is both overridden with '%s' at %s and imported at %s, which is not supported. Please use '%s' directly instead.", -// extensionLocalName, -// extensionName, -// apparentName, -// override.getValue().location, -// context.repoNameUsages.get(imports.inverse().get(extensionLocalName)).where, -// apparentName); -// } + String importedAs = imports.inverse().get(exportedName); + if (importedAs != null) { + context.reposSubjectToRepoOverride.put(importedAs, override.getValue()); + } + context.reposUsedAsRepoOverride.put(localRepoName, override.getValue()); } builder.setRepoOverrides( - ImmutableMap.copyOf(Maps.transformValues(overrides, v -> v.apparentName))); + ImmutableMap.copyOf(Maps.transformValues(repoOverrides, v -> v.localRepoName))); return builder.build(); } @@ -314,6 +320,41 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti } extensionUsages.add(extensionUsageBuilder.buildUsage()); } + // Check for chains in repo overrides: If A is overridden with B, and B is overridden with C, + // raise an error and suggest to directly override A with C. This ensures that repo overrides + // can be applied to repo mappings in a single step (and also prevents cycles). + Optional repoOverrideChainLink = + reposUsedAsRepoOverride.keySet().stream().filter(repoNameUsages::containsKey).findFirst(); + if (repoOverrideChainLink.isPresent()) { + var override = reposUsedAsRepoOverride.get(repoOverrideChainLink.get()); + var overrideOnOverride = reposSubjectToRepoOverride.get(repoOverrideChainLink.get()); + if (override.exportedName.equals( + override.extensionUsageBuilder.imports.get(overrideOnOverride.localRepoName))) { + throw Starlark.errorf( + "The repo '%s' used as an override for '%s' in module extension '%s' is itself" + + " overridden with '%s' at %s, which forms a cycle.", + override.localRepoName, + override.exportedName, + override.extensionUsageBuilder.extensionName, + overrideOnOverride.localRepoName, + overrideOnOverride.location()) + .withCallStack(override.stack); + } else { + throw Starlark.errorf( + "The repo '%s' used as an override for '%s' in module extension '%s' is itself" + + " overridden with '%s' at %s, which is not supported. Please directly" + + " override '%s' with '%s' instead.", + override.localRepoName, + override.exportedName, + override.extensionUsageBuilder.extensionName, + overrideOnOverride.localRepoName, + overrideOnOverride.location(), + override.exportedName, + overrideOnOverride.localRepoName) + .withCallStack(override.stack); + } + } + return module .setRegistry(registry) .setDeps(ImmutableMap.copyOf(deps)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index 18e33ee543498e..72464e44f89244 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -1754,4 +1754,109 @@ public void testOverrideRepo_duplicateOverride() throws Exception { \tFile "/workspace/MODULE.bazel", line 6, column 14, in Error in override_repo: The repo exported as 'foo' by module extension 'ext' is already overridden with 'override1' at /workspace/MODULE.bazel:5:14"""); } + + @Test + public void testOverrideRepo_chain_singleExtension() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + bazel_dep(name = "override", version = "1.0") + ext = use_extension('//:defs.bzl', 'ext') + use_repo(ext, bar = "foo") + override_repo(ext, baz = "bar") + override_repo(ext, foo = "override") + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 5, column 14, in + Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); + } + + @Test + public void testOverrideRepo_chain_multipleExtensions() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + bazel_dep(name = "override", version = "1.0") + ext1 = use_extension('//:defs.bzl', 'ext1') + ext2 = use_extension('//:defs.bzl', 'ext2') + override_repo(ext1, baz = "bar") + override_repo(ext2, foo = "override") + use_repo(ext2, bar = "foo") + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 5, column 14, in + Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); + } + + @Test + public void testOverrideRepo_cycle_singleExtension() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + bazel_dep(name = "override", version = "1.0") + ext = use_extension('//:defs.bzl', 'ext') + use_repo(ext, my_foo = "foo", my_bar = "bar") + override_repo(ext, foo = "my_bar", bar = "my_foo") + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 5, column 14, in + Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which forms a cycle."""); + } + + @Test + public void testOverrideRepo_cycle_multipleExtensions() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + ext1 = use_extension('//:defs.bzl', 'ext1') + ext2 = use_extension('//:defs.bzl', 'ext2') + override_repo(ext1, foo = "my_bar") + override_repo(ext2, bar = "my_foo") + use_repo(ext1, my_foo = "foo") + use_repo(ext2, my_bar = "bar") + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 5, column 14, in + Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which forms a cycle."""); + } } From 8f1ed03bc55641627ad468c0d980032c4b09137e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 6 Sep 2024 09:51:55 +0200 Subject: [PATCH 08/19] Docs --- site/en/external/extension.md | 7 ++++++ .../lib/bazel/bzlmod/ModuleFileGlobals.java | 22 ++++++++++++------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/site/en/external/extension.md b/site/en/external/extension.md index 5ba774961105ba..3d37cc7e062d25 100644 --- a/site/en/external/extension.md +++ b/site/en/external/extension.md @@ -174,6 +174,13 @@ several repo visibility rules: the repo visible to the module instead of the extension-generated repo of the same name. +### Overriding and adding repos + +The root module can use +[`override_repo`](/rules/lib/globals/module#override_repo) to override or add +repos to a module extension. This is especially useful when patches are applied +to the repos generated by the extension. + ## Best practices This section describes best practices when writing extensions so they are diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index c3b530746756a4..89cb956f50c9e2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.bazel.bzlmod; - import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; @@ -648,22 +647,29 @@ public void useRepo( @StarlarkMethod( name = "override_repo", doc = - "Imports one or more repos generated by the given module extension into the scope of the" - + " current module.", + "Makes one or more repos in the scope of the current module visible in the scope of the" + + " given module extension under the specified names. This can be used both to make" + + " new repos visible to other repos generated by the extension and to override repos" + + " generated by the extension.", parameters = { @Param( name = "extension_proxy", doc = "A module extension proxy object returned by a use_extension call."), }, - extraPositionals = @Param(name = "args", doc = "The names of the repos to import."), + extraPositionals = + @Param( + name = "args", + doc = + "The names of the repos to make visible to the other repositories generated by" + + " the extension."), extraKeywords = @Param( name = "kwargs", doc = - "Specifies certain repos to import into the scope of the current module with" - + " different names. The keys should be the name to use in the current scope," - + " whereas the values should be the original names exported by the module" - + " extension."), + "The names of the repos to make visible to the other repositories generated by" + + " the extension, where the values are the names of repos in the scope of" + + " the current module and the keys are the names under which they will be" + + " visible in the scope of the extension."), useStarlarkThread = true) public void overrideRepo( ModuleExtensionProxy extensionProxy, From 5d6c3990a2de1cb8ba4b4fd31c165e2a51969340 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 6 Sep 2024 09:55:30 +0200 Subject: [PATCH 09/19] Rename --- .../build/lib/bazel/bzlmod/ModuleFileGlobals.java | 6 +++--- src/test/tools/bzlmod/MODULE.bazel.lock | 14 +++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 89cb956f50c9e2..99a31818efffda 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -548,11 +548,11 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio } void addOverride( - String extensionLocalName, - String moduleLocalName, + String exportedName, + String localRepoName, ImmutableList stack) throws EvalException { - usageBuilder.addRepoOverride(extensionLocalName, moduleLocalName, stack); + usageBuilder.addRepoOverride(exportedName, localRepoName, stack); } class TagCallable implements StarlarkValue { diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index 54502b8d16d3fe..98d8c1f3434f79 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -4,6 +4,7 @@ "https://bcr.bazel.build/bazel_registry.json": "8a28e4aff06ee60aed2a8c281907fb8bcbf3b753c91fb5a5c57da3215d5b3497", "https://bcr.bazel.build/modules/abseil-cpp/20210324.2/MODULE.bazel": "7cd0312e064fde87c8d1cd79ba06c876bd23630c83466e9500321be55c96ace2", "https://bcr.bazel.build/modules/abseil-cpp/20211102.0/MODULE.bazel": "70390338f7a5106231d20620712f7cccb659cd0e9d073d1991c038eb9fc57589", + "https://bcr.bazel.build/modules/abseil-cpp/20211102.0/source.json": "7e3a9adf473e9af076ae485ed649d5641ad50ec5c11718103f34de03170d94ad", "https://bcr.bazel.build/modules/abseil-cpp/20230125.1/MODULE.bazel": "89047429cb0207707b2dface14ba7f8df85273d484c2572755be4bab7ce9c3a0", "https://bcr.bazel.build/modules/abseil-cpp/20230802.1/MODULE.bazel": "fa92e2eb41a04df73cdabeec37107316f7e5272650f81d6cc096418fe647b915", "https://bcr.bazel.build/modules/abseil-cpp/20230802.1/source.json": "035b6f1911e17340db722bbc9158f830ee6d5dedba4cb3bcb9e25e590808a32c", @@ -18,6 +19,7 @@ "https://bcr.bazel.build/modules/buildozer/7.1.2/MODULE.bazel": "2e8dd40ede9c454042645fd8d8d0cd1527966aa5c919de86661e62953cd73d84", "https://bcr.bazel.build/modules/buildozer/7.1.2/source.json": "c9028a501d2db85793a6996205c8de120944f50a0d570438fcae0457a5f9d1f8", "https://bcr.bazel.build/modules/googletest/1.11.0/MODULE.bazel": "3a83f095183f66345ca86aa13c58b59f9f94a2f81999c093d4eeaa2d262d12f4", + "https://bcr.bazel.build/modules/googletest/1.11.0/source.json": "c73d9ef4268c91bd0c1cd88f1f9dfa08e814b1dbe89b5f594a9f08ba0244d206", "https://bcr.bazel.build/modules/googletest/1.14.0/MODULE.bazel": "cfbcbf3e6eac06ef9d85900f64424708cc08687d1b527f0ef65aa7517af8118f", "https://bcr.bazel.build/modules/googletest/1.14.0/source.json": "2478949479000fdd7de9a3d0107ba2c85bb5f961c3ecb1aa448f52549ce310b5", "https://bcr.bazel.build/modules/platforms/0.0.4/MODULE.bazel": "9b328e31ee156f53f3c416a64f8491f7eb731742655a47c9eec4703a71644aee", @@ -39,6 +41,8 @@ "https://bcr.bazel.build/modules/rules_java/4.0.0/MODULE.bazel": "5a78a7ae82cd1a33cef56dc578c7d2a46ed0dca12643ee45edbb8417899e6f74", "https://bcr.bazel.build/modules/rules_java/7.11.1/MODULE.bazel": "b4782e019dd0b0151bd49fd8929136fd4441f527eb208fbd991b77e480b7236e", "https://bcr.bazel.build/modules/rules_java/7.11.1/source.json": "94b8c8bc691357f1f0bf630f09010d734d081caea8c82d5457e56ee4659101a1", + "https://bcr.bazel.build/modules/rules_java/7.9.0/MODULE.bazel": "aa1ebea5c5e2a4b31fbabe07bf3e9951ccc0f4e92499c42d325580f86c456020", + "https://bcr.bazel.build/modules/rules_java/7.9.0/source.json": "41ba541cffadec937a872c9cdfe67e2936afcb560b6957d60c2bf313a868e4b2", "https://bcr.bazel.build/modules/rules_jvm_external/4.4.2/MODULE.bazel": "a56b85e418c83eb1839819f0b515c431010160383306d13ec21959ac412d2fe7", "https://bcr.bazel.build/modules/rules_jvm_external/4.4.2/source.json": "a075731e1b46bc8425098512d038d416e966ab19684a10a34f4741295642fc35", "https://bcr.bazel.build/modules/rules_license/0.0.3/MODULE.bazel": "627e9ab0247f7d1e05736b59dbb1b6871373de5ad31c3011880b4133cafd4bd0", @@ -67,7 +71,7 @@ "@@platforms//host:extension.bzl%host_platform": { "general": { "bzlTransitiveDigest": "xelQcPZH8+tmuOHVjL9vDxMnnQNMlwj0SlvgoqBkm4U=", - "usagesDigest": "BsEIFMvXBOQ6RUjjEzt6CU2+w+vAjFwqp58drqCa2jo=", + "usagesDigest": "ydv+KRQbS945mVH4SXXBNPop//5o22lzFtSP+dEqsAA=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -84,7 +88,7 @@ "@@rules_jvm_external+//:extensions.bzl%maven": { "general": { "bzlTransitiveDigest": "tgBpQFC46MaT8n2UeSnG4GNQ8M01bKKTeEWQX+cuoSA=", - "usagesDigest": "sl3uEZVp3ixjaQzCfAguinRv5zcKQM68YRioCHBX0cE=", + "usagesDigest": "OIrESCiASnWOQs/5hTLrvqtstJVja1Z5Rj39l/uNopA=", "recordedFileInputs": { "@@rules_jvm_external+//rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" }, @@ -1108,7 +1112,7 @@ "@@rules_jvm_external+//:non-module-deps.bzl%non_module_deps": { "general": { "bzlTransitiveDigest": "7dxAT2NQpsWXJNV5Sy6o7E78vgRhVEN1vlZdrexTMZY=", - "usagesDigest": "xWSELLxz/xEmLvX/KEddTujJEVrOIQ4j7cs6BkF2J4s=", + "usagesDigest": "cIAFEuTK+KbiJYZiahtuMtOP4aLrhgmzwc0RV4B/AcQ=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1136,7 +1140,7 @@ "@@rules_python+//python/extensions:python.bzl%python": { "general": { "bzlTransitiveDigest": "z35lvtk23Cj8pA0OHXIWJQ+sP4WORVrujhMDtWGyqo8=", - "usagesDigest": "tmZ6sMJ23K1YYNMIBcEX0sXPX3I2o7ICQethLTZQvuM=", + "usagesDigest": "F7QPs51Dklu8NxhCATWMWwHvMVKN2BtM0N02S6yUDO4=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1166,7 +1170,7 @@ "@@rules_python+//python/extensions/private:internal_deps.bzl%internal_deps": { "general": { "bzlTransitiveDigest": "59ucGaK2UFzBbVlrE4x4O1jM4EWKd3zAXRUH63T4j68=", - "usagesDigest": "T+GX/uEOF49O6I6zgLru1E6pJLzL0nzFKcKsMpGGm4I=", + "usagesDigest": "cbWLCQxHNEsgEWEV/0QSykRfx7eGGm+eQb8Dxws/Vjk=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, From 4d7e6d727a3b2f7724c6831235ca7a4b60d20a66 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 6 Sep 2024 13:08:47 +0200 Subject: [PATCH 10/19] Update docs --- .../build/lib/bazel/bzlmod/ModuleFileGlobals.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 99a31818efffda..d6cf1a69553ef0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -841,7 +841,10 @@ public void include(String label, StarlarkThread thread) doc = "A list of labels pointing to patch files to apply for this module. The patch files" + " must exist in the source tree of the top level project. They are applied in" - + " the list order.", + + " the list order." + + "" + + "

If a patch makes changes to the MODULE.bazel file, these changes will" + + " only be effective if the patch file is provided by the root module.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, @@ -849,7 +852,9 @@ public void include(String label, StarlarkThread thread) @Param( name = "patch_cmds", doc = - "Sequence of Bash commands to be applied on Linux/Macos after patches are applied.", + "Sequence of Bash commands to be applied on Linux/Macos after patches are applied." + + "" + + "

Changes to the MODULE.bazel file will not be effective.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, From a1090d75d46d853c3593181a04d8ec9ea6d6589d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 6 Sep 2024 16:57:44 +0200 Subject: [PATCH 11/19] Fix test --- .../devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 78b7ed79fc24d5..b2d719fedbf234 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -324,7 +324,9 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti // raise an error and suggest to directly override A with C. This ensures that repo overrides // can be applied to repo mappings in a single step (and also prevents cycles). Optional repoOverrideChainLink = - reposUsedAsRepoOverride.keySet().stream().filter(repoNameUsages::containsKey).findFirst(); + reposUsedAsRepoOverride.keySet().stream() + .filter(reposSubjectToRepoOverride::containsKey) + .findFirst(); if (repoOverrideChainLink.isPresent()) { var override = reposUsedAsRepoOverride.get(repoOverrideChainLink.get()); var overrideOnOverride = reposSubjectToRepoOverride.get(repoOverrideChainLink.get()); From 3b9279214989d857d5c36fb25b7c3101fc849f4e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 15 Sep 2024 12:33:53 +0200 Subject: [PATCH 12/19] Address some comments --- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 15 +-- .../lib/bazel/bzlmod/ModuleThreadContext.java | 96 +++++++++---------- .../bazel/bzlmod/ModuleFileFunctionTest.java | 2 +- src/test/tools/bzlmod/MODULE.bazel.lock | 14 +-- 4 files changed, 58 insertions(+), 69 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index d6cf1a69553ef0..0824b21a690813 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -548,11 +548,11 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio } void addOverride( - String exportedName, - String localRepoName, + String overriddenRepoName, + String overridingRepoName, ImmutableList stack) throws EvalException { - usageBuilder.addRepoOverride(exportedName, localRepoName, stack); + usageBuilder.addRepoOverride(overriddenRepoName, overridingRepoName, stack); } class TagCallable implements StarlarkValue { @@ -841,10 +841,7 @@ public void include(String label, StarlarkThread thread) doc = "A list of labels pointing to patch files to apply for this module. The patch files" + " must exist in the source tree of the top level project. They are applied in" - + " the list order." - + "" - + "

If a patch makes changes to the MODULE.bazel file, these changes will" - + " only be effective if the patch file is provided by the root module.", + + " the list order.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, @@ -852,9 +849,7 @@ public void include(String label, StarlarkThread thread) @Param( name = "patch_cmds", doc = - "Sequence of Bash commands to be applied on Linux/Macos after patches are applied." - + "" - + "

Changes to the MODULE.bazel file will not be effective.", + "Sequence of Bash commands to be applied on Linux/Macos after patches are applied.", allowedTypes = {@ParamType(type = Iterable.class, generic1 = String.class)}, named = true, positional = false, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index b2d719fedbf234..18a28b8981f8c1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -55,10 +55,8 @@ public class ModuleThreadContext extends StarlarkThreadContext { private final Map overrides = new LinkedHashMap<>(); private final Map repoNameUsages = new HashMap<>(); - private final Map reposSubjectToRepoOverride = - new HashMap<>(); - private final Map reposUsedAsRepoOverride = - new HashMap<>(); + private final Map overriddenRepos = new HashMap<>(); + private final Map overridingRepos = new HashMap<>(); public static ModuleThreadContext fromOrFail(StarlarkThread thread, String what) throws EvalException { @@ -81,6 +79,17 @@ public ModuleThreadContext( this.includeLabelToCompiledModuleFile = includeLabelToCompiledModuleFile; } + record RepoOverride( + String overriddenRepoName, + String overridingRepoName, + ModuleExtensionUsageBuilder extensionUsageBuilder, + ImmutableList stack) { + Location location() { + // Skip over the override_repo builtin frame. + return stack.reverse().get(1).location; + } + } + record RepoNameUsage(String how, Location where) {} public void addRepoNameUsage(String repoName, String how, Location where) throws EvalException { @@ -128,24 +137,13 @@ List getExtensionUsageBuilders() { static class ModuleExtensionUsageBuilder { - private record RepoOverride( - String exportedName, - String localRepoName, - ModuleExtensionUsageBuilder extensionUsageBuilder, - ImmutableList stack) { - Location location() { - // Skip over the override_repo builtin frame. - return stack.reverse().get(1).location; - } - } - private final ModuleThreadContext context; private final String extensionBzlFile; private final String extensionName; private final boolean isolate; private final ArrayList proxyBuilders; private final HashBiMap imports; - private final Map repoOverrides; + private final Map repoOverrides; private final ImmutableList.Builder tags; ModuleExtensionUsageBuilder( @@ -192,20 +190,23 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio } public void addRepoOverride( - String exportedName, - String localRepoName, + String overriddenRepoName, + String overridingRepoName, ImmutableList stack) throws EvalException { - RepositoryName.validateUserProvidedRepoName(exportedName); - RepositoryName.validateUserProvidedRepoName(localRepoName); - if (repoOverrides.containsKey(exportedName)) { - var collision = repoOverrides.get(exportedName); + RepositoryName.validateUserProvidedRepoName(overriddenRepoName); + RepositoryName.validateUserProvidedRepoName(overridingRepoName); + RepoOverride collision = + repoOverrides.put( + overriddenRepoName, + new ModuleThreadContext.RepoOverride( + overriddenRepoName, overridingRepoName, this, stack)); + if (collision != null) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at" + " %s", - exportedName, extensionName, collision.localRepoName, collision.location()); + overriddenRepoName, extensionName, collision.overridingRepoName, collision.location()); } - repoOverrides.put(exportedName, new RepoOverride(exportedName, localRepoName, this, stack)); } void addTag(Tag tag) { @@ -237,22 +238,22 @@ ModuleExtensionUsage buildUsage() throws EvalException { for (var override : repoOverrides.entrySet()) { String exportedName = override.getKey(); - String localRepoName = override.getValue().localRepoName; + String localRepoName = override.getValue().overridingRepoName; if (!context.repoNameUsages.containsKey(localRepoName)) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is overridden with '%s', but" - + " no repo is imported under this name", + + " no repo is visible under this name", exportedName, extensionName, localRepoName) .withCallStack(override.getValue().stack); } String importedAs = imports.inverse().get(exportedName); if (importedAs != null) { - context.reposSubjectToRepoOverride.put(importedAs, override.getValue()); + context.overriddenRepos.put(importedAs, override.getValue()); } - context.reposUsedAsRepoOverride.put(localRepoName, override.getValue()); + context.overridingRepos.put(localRepoName, override.getValue()); } builder.setRepoOverrides( - ImmutableMap.copyOf(Maps.transformValues(repoOverrides, v -> v.localRepoName))); + ImmutableMap.copyOf(Maps.transformValues(repoOverrides, v -> v.overridingRepoName))); return builder.build(); } @@ -320,25 +321,22 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti } extensionUsages.add(extensionUsageBuilder.buildUsage()); } - // Check for chains in repo overrides: If A is overridden with B, and B is overridden with C, - // raise an error and suggest to directly override A with C. This ensures that repo overrides - // can be applied to repo mappings in a single step (and also prevents cycles). - Optional repoOverrideChainLink = - reposUsedAsRepoOverride.keySet().stream() - .filter(reposSubjectToRepoOverride::containsKey) - .findFirst(); - if (repoOverrideChainLink.isPresent()) { - var override = reposUsedAsRepoOverride.get(repoOverrideChainLink.get()); - var overrideOnOverride = reposSubjectToRepoOverride.get(repoOverrideChainLink.get()); - if (override.exportedName.equals( - override.extensionUsageBuilder.imports.get(overrideOnOverride.localRepoName))) { + // A repo cannot be both overriding and overridden. This ensures that repo overrides can be + // applied to repo mappings in a single step (and also prevents cycles). + Optional overridingAndOverridden = + overridingRepos.keySet().stream().filter(overriddenRepos::containsKey).findFirst(); + if (overridingAndOverridden.isPresent()) { + var override = overridingRepos.get(overridingAndOverridden.get()); + var overrideOnOverride = overriddenRepos.get(overridingAndOverridden.get()); + if (override.overriddenRepoName.equals( + override.extensionUsageBuilder.imports.get(overrideOnOverride.overridingRepoName))) { throw Starlark.errorf( "The repo '%s' used as an override for '%s' in module extension '%s' is itself" + " overridden with '%s' at %s, which forms a cycle.", - override.localRepoName, - override.exportedName, + override.overridingRepoName, + override.overriddenRepoName, override.extensionUsageBuilder.extensionName, - overrideOnOverride.localRepoName, + overrideOnOverride.overridingRepoName, overrideOnOverride.location()) .withCallStack(override.stack); } else { @@ -346,13 +344,13 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti "The repo '%s' used as an override for '%s' in module extension '%s' is itself" + " overridden with '%s' at %s, which is not supported. Please directly" + " override '%s' with '%s' instead.", - override.localRepoName, - override.exportedName, + override.overridingRepoName, + override.overriddenRepoName, override.extensionUsageBuilder.extensionName, - overrideOnOverride.localRepoName, + overrideOnOverride.overridingRepoName, overrideOnOverride.location(), - override.exportedName, - overrideOnOverride.localRepoName) + override.overriddenRepoName, + overrideOnOverride.overridingRepoName) .withCallStack(override.stack); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index 72464e44f89244..b453f53ac8308d 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -1726,7 +1726,7 @@ public void testOverrideRepo_overridingRepoDoesntExist() throws Exception { """ ERROR /workspace/MODULE.bazel:3:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 3, column 14, in - Error in override_repo: The repo exported as 'foo' by module extension 'ext' is overridden with 'foo', but no repo is imported under this name"""); + Error in override_repo: The repo exported as 'foo' by module extension 'ext' is overridden with 'foo', but no repo is visible under this name"""); } @Test diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index 98d8c1f3434f79..03ed8d97af56c4 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -4,7 +4,6 @@ "https://bcr.bazel.build/bazel_registry.json": "8a28e4aff06ee60aed2a8c281907fb8bcbf3b753c91fb5a5c57da3215d5b3497", "https://bcr.bazel.build/modules/abseil-cpp/20210324.2/MODULE.bazel": "7cd0312e064fde87c8d1cd79ba06c876bd23630c83466e9500321be55c96ace2", "https://bcr.bazel.build/modules/abseil-cpp/20211102.0/MODULE.bazel": "70390338f7a5106231d20620712f7cccb659cd0e9d073d1991c038eb9fc57589", - "https://bcr.bazel.build/modules/abseil-cpp/20211102.0/source.json": "7e3a9adf473e9af076ae485ed649d5641ad50ec5c11718103f34de03170d94ad", "https://bcr.bazel.build/modules/abseil-cpp/20230125.1/MODULE.bazel": "89047429cb0207707b2dface14ba7f8df85273d484c2572755be4bab7ce9c3a0", "https://bcr.bazel.build/modules/abseil-cpp/20230802.1/MODULE.bazel": "fa92e2eb41a04df73cdabeec37107316f7e5272650f81d6cc096418fe647b915", "https://bcr.bazel.build/modules/abseil-cpp/20230802.1/source.json": "035b6f1911e17340db722bbc9158f830ee6d5dedba4cb3bcb9e25e590808a32c", @@ -19,7 +18,6 @@ "https://bcr.bazel.build/modules/buildozer/7.1.2/MODULE.bazel": "2e8dd40ede9c454042645fd8d8d0cd1527966aa5c919de86661e62953cd73d84", "https://bcr.bazel.build/modules/buildozer/7.1.2/source.json": "c9028a501d2db85793a6996205c8de120944f50a0d570438fcae0457a5f9d1f8", "https://bcr.bazel.build/modules/googletest/1.11.0/MODULE.bazel": "3a83f095183f66345ca86aa13c58b59f9f94a2f81999c093d4eeaa2d262d12f4", - "https://bcr.bazel.build/modules/googletest/1.11.0/source.json": "c73d9ef4268c91bd0c1cd88f1f9dfa08e814b1dbe89b5f594a9f08ba0244d206", "https://bcr.bazel.build/modules/googletest/1.14.0/MODULE.bazel": "cfbcbf3e6eac06ef9d85900f64424708cc08687d1b527f0ef65aa7517af8118f", "https://bcr.bazel.build/modules/googletest/1.14.0/source.json": "2478949479000fdd7de9a3d0107ba2c85bb5f961c3ecb1aa448f52549ce310b5", "https://bcr.bazel.build/modules/platforms/0.0.4/MODULE.bazel": "9b328e31ee156f53f3c416a64f8491f7eb731742655a47c9eec4703a71644aee", @@ -41,8 +39,6 @@ "https://bcr.bazel.build/modules/rules_java/4.0.0/MODULE.bazel": "5a78a7ae82cd1a33cef56dc578c7d2a46ed0dca12643ee45edbb8417899e6f74", "https://bcr.bazel.build/modules/rules_java/7.11.1/MODULE.bazel": "b4782e019dd0b0151bd49fd8929136fd4441f527eb208fbd991b77e480b7236e", "https://bcr.bazel.build/modules/rules_java/7.11.1/source.json": "94b8c8bc691357f1f0bf630f09010d734d081caea8c82d5457e56ee4659101a1", - "https://bcr.bazel.build/modules/rules_java/7.9.0/MODULE.bazel": "aa1ebea5c5e2a4b31fbabe07bf3e9951ccc0f4e92499c42d325580f86c456020", - "https://bcr.bazel.build/modules/rules_java/7.9.0/source.json": "41ba541cffadec937a872c9cdfe67e2936afcb560b6957d60c2bf313a868e4b2", "https://bcr.bazel.build/modules/rules_jvm_external/4.4.2/MODULE.bazel": "a56b85e418c83eb1839819f0b515c431010160383306d13ec21959ac412d2fe7", "https://bcr.bazel.build/modules/rules_jvm_external/4.4.2/source.json": "a075731e1b46bc8425098512d038d416e966ab19684a10a34f4741295642fc35", "https://bcr.bazel.build/modules/rules_license/0.0.3/MODULE.bazel": "627e9ab0247f7d1e05736b59dbb1b6871373de5ad31c3011880b4133cafd4bd0", @@ -71,7 +67,7 @@ "@@platforms//host:extension.bzl%host_platform": { "general": { "bzlTransitiveDigest": "xelQcPZH8+tmuOHVjL9vDxMnnQNMlwj0SlvgoqBkm4U=", - "usagesDigest": "ydv+KRQbS945mVH4SXXBNPop//5o22lzFtSP+dEqsAA=", + "usagesDigest": "dV0XZuBT6Bl4dbmmjYrJStsZAD/hV31fLkUkDHmbHIM=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -88,7 +84,7 @@ "@@rules_jvm_external+//:extensions.bzl%maven": { "general": { "bzlTransitiveDigest": "tgBpQFC46MaT8n2UeSnG4GNQ8M01bKKTeEWQX+cuoSA=", - "usagesDigest": "OIrESCiASnWOQs/5hTLrvqtstJVja1Z5Rj39l/uNopA=", + "usagesDigest": "gwSjMkWKurhylh2CEkjffLf0iUOR4Ra7XvJyx5l1ECc=", "recordedFileInputs": { "@@rules_jvm_external+//rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" }, @@ -1112,7 +1108,7 @@ "@@rules_jvm_external+//:non-module-deps.bzl%non_module_deps": { "general": { "bzlTransitiveDigest": "7dxAT2NQpsWXJNV5Sy6o7E78vgRhVEN1vlZdrexTMZY=", - "usagesDigest": "cIAFEuTK+KbiJYZiahtuMtOP4aLrhgmzwc0RV4B/AcQ=", + "usagesDigest": "lOaODXUC3oEqMd8/8lv63loQ2pQB4QFWNzo9pqgifck=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1140,7 +1136,7 @@ "@@rules_python+//python/extensions:python.bzl%python": { "general": { "bzlTransitiveDigest": "z35lvtk23Cj8pA0OHXIWJQ+sP4WORVrujhMDtWGyqo8=", - "usagesDigest": "F7QPs51Dklu8NxhCATWMWwHvMVKN2BtM0N02S6yUDO4=", + "usagesDigest": "u8OFDhNDDCVxFb/U4yT0JRRUDiGNgTPY/lKnK3FcvdM=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1170,7 +1166,7 @@ "@@rules_python+//python/extensions/private:internal_deps.bzl%internal_deps": { "general": { "bzlTransitiveDigest": "59ucGaK2UFzBbVlrE4x4O1jM4EWKd3zAXRUH63T4j68=", - "usagesDigest": "cbWLCQxHNEsgEWEV/0QSykRfx7eGGm+eQb8Dxws/Vjk=", + "usagesDigest": "7eZGJGF9wrvQzVaMSqT5s+PIScCmeQUQ61hx1hyUvNI=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, From 05f1826d6c4136a10a6af86147186c7ea400d83a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 15 Sep 2024 13:34:32 +0200 Subject: [PATCH 13/19] Split into override_repo and inject_repo --- site/en/external/extension.md | 58 +++++++- .../bazel/bzlmod/BazelDepGraphFunction.java | 4 +- .../bazel/bzlmod/ModuleExtensionUsage.java | 18 ++- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 79 ++++++++-- .../lib/bazel/bzlmod/ModuleThreadContext.java | 11 +- .../bazel/bzlmod/SingleExtensionFunction.java | 32 +++++ .../bzlmod/ModuleExtensionResolutionTest.java | 136 +++++++++++++++++- 7 files changed, 312 insertions(+), 26 deletions(-) diff --git a/site/en/external/extension.md b/site/en/external/extension.md index 3d37cc7e062d25..cbaebb51f45fe8 100644 --- a/site/en/external/extension.md +++ b/site/en/external/extension.md @@ -174,12 +174,62 @@ several repo visibility rules: the repo visible to the module instead of the extension-generated repo of the same name. -### Overriding and adding repos +### Overriding and injecting module extension repos The root module can use -[`override_repo`](/rules/lib/globals/module#override_repo) to override or add -repos to a module extension. This is especially useful when patches are applied -to the repos generated by the extension. +[`override_repo`](/rules/lib/globals/module#override_repo) and +[`inject_repo`](/rules/lib/globals/module#inject_repo) to override or inject +module extension repos. + +#### Example: Replacing `rules_java`'s `java_tools` with a vendored copy + +```python +# MODULE.bazel +local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository") +local_repository( + name = "my_java_tools", + path = "vendor/java_tools", +) + +bazel_dep(name = "rules_java", version = "7.11.1") +java_toolchains = use_extension("@rules_java//java:extension.bzl", "toolchains") + +override_repo(java_toolchains, remote_java_tools = "my_java_tools") +``` + +#### Example: Patch a Go dependency to depend on `@zlib` instead of the system zlib + +```python +# MODULE.bazel +bazel_dep(name = "gazelle", version = "0.38.0") +bazel_dep(name = "zlib", version = "1.3.1.bcr.3") + +go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps") +go_deps.from_file(go_mod = "//:go.mod") +go_deps.module_override( + patches = [ + "//patches:my_module_zlib.patch", + ], + path = "example.com/my_module", +) +use_repo(go_deps, ...) + +inject_repo(go_deps, "zlib") +``` + +```diff +# patches/my_module_zlib.patch +--- a/BUILD.bazel ++++ b/BUILD.bazel +@@ -1,6 +1,6 @@ + go_binary( + name = "my_module", + importpath = "example.com/my_module", + srcs = ["my_module.go"], +- copts = ["-lz"], ++ cdeps = ["@zlib"], + ) +``` ## Best practices diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index bc6dbe3b2cad25..c4bde42e1e3d99 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -226,7 +226,9 @@ private static ImmutableTable resolve if (rootUsage != null) { for (var override : rootUsage.getRepoOverrides().entrySet()) { repoOverridesBuilder.put( - extensionId, override.getKey(), rootModuleMapping.get(override.getValue())); + extensionId, + override.getKey(), + rootModuleMapping.get(override.getValue().overridingRepoName())); } } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index ab545db013c23d..2bb65bc350b70b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -131,14 +131,21 @@ public final boolean getHasNonDevUseExtension() { return getProxies().stream().anyMatch(p -> !p.isDevDependency()); } + @GenerateTypeAdapter + public record RepoOverride( + // The apparent name of the overriding repo in the root module. + String overridingRepoName, + // Whether this override should apply to an existing repo. + boolean mustExist, + Location location) {} + /** - * Maps repo names local to the extension to the apparent name in the using module of the repo - * they are overridden with. + * Maps repo names local to the extension to repo they are overridden by. * *

This is only non-empty for root module usages and repos that override other repos are not * themselves overridden. */ - public abstract ImmutableMap getRepoOverrides(); + public abstract ImmutableMap getRepoOverrides(); public abstract Builder toBuilder(); @@ -163,6 +170,9 @@ ModuleExtensionUsage trimForEvaluation() { // against the set of generated repos in a validation step that comes afterward. .setProxies(ImmutableList.of()) // Tracked in SingleExtensionUsagesValue instead, using canonical instead of apparent names. + // Whether this override must apply to an existing repo as well as its source location also + // don't influence the evaluation of the extension as they are checked in + // SingleExtensionFunction. .setRepoOverrides(ImmutableMap.of()) .build(); } @@ -199,7 +209,7 @@ public Builder addTag(Tag value) { @CanIgnoreReturnValue public abstract Builder setRepoOverrides( - ImmutableMap extensionLocalToApparentName); + ImmutableMap extensionLocalToApparentName); public abstract ModuleExtensionUsage build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 0824b21a690813..ef582e69efdf27 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -550,9 +550,10 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio void addOverride( String overriddenRepoName, String overridingRepoName, + boolean mustExist, ImmutableList stack) throws EvalException { - usageBuilder.addRepoOverride(overriddenRepoName, overridingRepoName, stack); + usageBuilder.addRepoOverride(overriddenRepoName, overridingRepoName, mustExist, stack); } class TagCallable implements StarlarkValue { @@ -647,10 +648,12 @@ public void useRepo( @StarlarkMethod( name = "override_repo", doc = - "Makes one or more repos in the scope of the current module visible in the scope of the" - + " given module extension under the specified names. This can be used both to make" - + " new repos visible to other repos generated by the extension and to override repos" - + " generated by the extension.", + "Overrides one or more repos defined by the given module extension with the given repos" + + " visible to the current module. This is ignored if the current module is not the" + + " root module or `--ignore_dev_dependency` is enabled." + + "" + + "

Use inject_repo instead to add a new" + + " repo.", parameters = { @Param( name = "extension_proxy", @@ -660,16 +663,15 @@ public void useRepo( @Param( name = "args", doc = - "The names of the repos to make visible to the other repositories generated by" - + " the extension."), + "The repos in the extension that should be overridden with the repos of the same" + + " name in the current module."), extraKeywords = @Param( name = "kwargs", doc = - "The names of the repos to make visible to the other repositories generated by" - + " the extension, where the values are the names of repos in the scope of" - + " the current module and the keys are the names under which they will be" - + " visible in the scope of the extension."), + "The overrides to apply to the repos generated by the extension, where the values" + + " are the names of repos in the scope of the current module and the keys" + + " are the names of the repos they will override in the extension."), useStarlarkThread = true) public void overrideRepo( ModuleExtensionProxy extensionProxy, @@ -686,11 +688,62 @@ public void overrideRepo( } ImmutableList stack = thread.getCallStack(); for (String arg : Sequence.cast(args, String.class, "args")) { - extensionProxy.addOverride(arg, arg, stack); + extensionProxy.addOverride(arg, arg, /* mustExist= */ true, stack); } for (Map.Entry entry : Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) { - extensionProxy.addOverride(entry.getKey(), entry.getValue(), stack); + extensionProxy.addOverride(entry.getKey(), entry.getValue(), /* mustExist= */ true, stack); + } + } + + @StarlarkMethod( + name = "inject_repo", + doc = + "Injects one or more new repos into the given module extension." + + " This is ignored if the current module is not the root module or" + + " `--ignore_dev_dependency` is enabled." + + "" + + "

Use override_repo instead to" + + " override an existing repo.", + parameters = { + @Param( + name = "extension_proxy", + doc = "A module extension proxy object returned by a use_extension call."), + }, + extraPositionals = + @Param( + name = "args", + doc = + "The repos visible to the current module that should be injected into the" + + " extension under the same name."), + extraKeywords = + @Param( + name = "kwargs", + doc = + "The new repos to inject into the extension, where the values are the names of" + + " repos in the scope of the current module and the keys are the name they" + + " will be visible under in the extension."), + useStarlarkThread = true) + public void injectRepo( + ModuleExtensionProxy extensionProxy, + Tuple args, + Dict kwargs, + StarlarkThread thread) + throws EvalException { + ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "inject_repo()"); + context.setNonModuleCalled(); + if (context.shouldIgnoreDevDeps()) { + // Ignore calls early as they may refer to repos that are dev dependencies (or this is not the + // root module). + return; + } + ImmutableList stack = thread.getCallStack(); + for (String arg : Sequence.cast(args, String.class, "args")) { + extensionProxy.addOverride(arg, arg, /* mustExist= */ false, stack); + } + for (Map.Entry entry : + Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) { + extensionProxy.addOverride(entry.getKey(), entry.getValue(), /* mustExist= */ false, stack); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 18a28b8981f8c1..e756e910b78d33 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -82,6 +82,7 @@ public ModuleThreadContext( record RepoOverride( String overriddenRepoName, String overridingRepoName, + boolean mustExist, ModuleExtensionUsageBuilder extensionUsageBuilder, ImmutableList stack) { Location location() { @@ -192,6 +193,7 @@ void addImport(String localRepoName, String exportedName, String byWhat, Locatio public void addRepoOverride( String overriddenRepoName, String overridingRepoName, + boolean mustExist, ImmutableList stack) throws EvalException { RepositoryName.validateUserProvidedRepoName(overriddenRepoName); @@ -200,7 +202,7 @@ public void addRepoOverride( repoOverrides.put( overriddenRepoName, new ModuleThreadContext.RepoOverride( - overriddenRepoName, overridingRepoName, this, stack)); + overriddenRepoName, overridingRepoName, mustExist, this, stack)); if (collision != null) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at" @@ -253,7 +255,12 @@ ModuleExtensionUsage buildUsage() throws EvalException { context.overridingRepos.put(localRepoName, override.getValue()); } builder.setRepoOverrides( - ImmutableMap.copyOf(Maps.transformValues(repoOverrides, v -> v.overridingRepoName))); + ImmutableMap.copyOf( + Maps.transformValues( + repoOverrides, + v -> + new ModuleExtensionUsage.RepoOverride( + v.overridingRepoName, v.mustExist, v.location())))); return builder.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java index 49c8a6e4e7188c..fbce3dfd7dac3b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java @@ -72,6 +72,38 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } + // Check that repo overrides apply as declared. + for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { + for (var override : usage.getRepoOverrides().entrySet()) { + boolean repoExists = evalOnlyValue.getGeneratedRepoSpecs().containsKey(override.getKey()); + if (repoExists && !override.getValue().mustExist()) { + throw new SingleExtensionFunctionException( + ExternalDepsException.withMessage( + Code.INVALID_EXTENSION_IMPORT, + "module extension \"%s\" from \"%s\" generates repository \"%s\", yet" + + " it is injected via inject_repo() at %s. Use override_repo() instead to" + + " override an existing repository.", + extensionId.getExtensionName(), + extensionId.getBzlFileLabel(), + override.getKey(), + override.getValue().location()), + Transience.PERSISTENT); + } else if (!repoExists && override.getValue().mustExist()) { + throw new SingleExtensionFunctionException( + ExternalDepsException.withMessage( + Code.INVALID_EXTENSION_IMPORT, + "module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet" + + " it is overridden via override_repo() at %s. Use inject_repo() instead to" + + " inject a new repository.", + extensionId.getExtensionName(), + extensionId.getBzlFileLabel(), + override.getKey(), + override.getValue().location()), + Transience.PERSISTENT); + } + } + } + return evalOnlyValue; } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 4aa9eeb9c28044..7cf39c6df46600 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -3100,7 +3100,7 @@ def _other_ext_impl(ctx): } @Test - public void overrideRepo_replace() throws Exception { + public void overrideRepo_override() throws Exception { scratch.file( workspaceRoot.getRelative("MODULE.bazel").getPathString(), """ @@ -3176,7 +3176,7 @@ def _ext_impl(ctx): } @Test - public void overrideRepo_add() throws Exception { + public void overrideRepo_override_onNonExistentRepoFails() throws Exception { scratch.file( workspaceRoot.getRelative("MODULE.bazel").getPathString(), """ @@ -3225,6 +3225,69 @@ def _ext_impl(ctx): ext = module_extension(implementation = _ext_impl) """); + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()) + .hasMessageThat() + .isEqualTo( + "module extension \"ext\" from \"//:defs.bzl\" does not generate repository \"foo\"," + + " yet it is overridden via override_repo() at /ws/MODULE.bazel:6:14. Use" + + " inject_repo() instead to inject a new repository."); + } + + @Test + public void overrideRepo_inject() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + """ + bazel_dep(name = "data_repo", version = "1.0") + ext = use_extension("//:defs.bzl","ext") + use_repo(ext, "bar", module_foo = "foo") + data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") + data_repo(name = "foo", data = "overridden_data") + inject_repo(ext, "foo") + """); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + """ + load("@bar//:list.bzl", _bar_list = "list") + load("@foo//:data.bzl", _foo_data = "data") + bar_list = _bar_list + foo_data = _foo_data + """); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + """ + load("@data_repo//:defs.bzl", "data_repo") + def _list_repo_impl(ctx): + ctx.file("WORKSPACE") + ctx.file("BUILD") + labels = [str(Label(l)) for l in ctx.attr.labels] + labels += [str(Label("@foo//:target3"))] + ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") + list_repo = repository_rule( + implementation = _list_repo_impl, + attrs = { + "labels": attr.label_list(), + }, + ) + def _ext_impl(ctx): + list_repo( + name = "bar", + labels = [ + # lazy extension implementation function repository mapping + "@foo//:target1", + # module repo repository mapping + Label("@foo//:target2"), + ], + ) + ext = module_extension(implementation = _ext_impl) + """); + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); @@ -3242,4 +3305,73 @@ def _ext_impl(ctx): assertThat(fooData).isInstanceOf(String.class); assertThat(fooData).isEqualTo("overridden_data"); } + + @Test + public void overrideRepo_inject_onExistingRepoFails() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + """ + bazel_dep(name = "data_repo", version = "1.0") + ext = use_extension("//:defs.bzl","ext") + use_repo(ext, "bar", module_foo = "foo") + data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") + data_repo(name = "override", data = "overridden_data") + inject_repo(ext, foo = "override") + """); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + """ + load("@bar//:list.bzl", _bar_list = "list") + load("@override//:data.bzl", _override_data = "data") + load("@module_foo//:data.bzl", _foo_data = "data") + bar_list = _bar_list + foo_data = _foo_data + override_data = _override_data + """); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + """ + load("@data_repo//:defs.bzl", "data_repo") + def _list_repo_impl(ctx): + ctx.file("WORKSPACE") + ctx.file("BUILD") + labels = [str(Label(l)) for l in ctx.attr.labels] + labels += [str(Label("@module_foo//:target3"))] + ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") + list_repo = repository_rule( + implementation = _list_repo_impl, + attrs = { + "labels": attr.label_list(), + }, + ) + def _fail_repo_impl(ctx): + fail("This rule should not be evaluated") + fail_repo = repository_rule(implementation = _fail_repo_impl) + def _ext_impl(ctx): + fail_repo(name = "foo") + list_repo( + name = "bar", + labels = [ + # lazy extension implementation function repository mapping + "@foo//:target1", + # module repo repository mapping + "@module_foo//:target2", + ], + ) + ext = module_extension(implementation = _ext_impl) + """); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()) + .hasMessageThat() + .isEqualTo( + "module extension \"ext\" from \"//:defs.bzl\" generates repository \"foo\", yet it is" + + " injected via inject_repo() at /ws/MODULE.bazel:6:12. Use override_repo()" + + " instead to override an existing repository."); + } } From 427b71f5906f9aa3c3c60629d5e4896448a4235b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 15 Sep 2024 13:43:58 +0200 Subject: [PATCH 14/19] Fix source file snippets in eval exceptions --- .../bazel/bzlmod/ModuleFileFunctionTest.java | 6 ++++++ .../google/devtools/build/lib/testutil/BUILD | 1 + .../build/lib/testutil/FoundationTestCase.java | 17 +++++++++++++++++ 3 files changed, 24 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index b453f53ac8308d..a824ac6e331054 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -1726,6 +1726,7 @@ public void testOverrideRepo_overridingRepoDoesntExist() throws Exception { """ ERROR /workspace/MODULE.bazel:3:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 3, column 14, in + \t\toverride_repo(ext, 'foo') Error in override_repo: The repo exported as 'foo' by module extension 'ext' is overridden with 'foo', but no repo is visible under this name"""); } @@ -1752,6 +1753,7 @@ public void testOverrideRepo_duplicateOverride() throws Exception { """ ERROR /workspace/MODULE.bazel:6:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 6, column 14, in + \t\toverride_repo(ext, foo = "override2") Error in override_repo: The repo exported as 'foo' by module extension 'ext' is already overridden with 'override1' at /workspace/MODULE.bazel:5:14"""); } @@ -1778,6 +1780,7 @@ public void testOverrideRepo_chain_singleExtension() throws Exception { """ ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in + \t\toverride_repo(ext, baz = "bar") Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); } @@ -1805,6 +1808,7 @@ public void testOverrideRepo_chain_multipleExtensions() throws Exception { """ ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in + \t\toverride_repo(ext1, baz = "bar") Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); } @@ -1830,6 +1834,7 @@ public void testOverrideRepo_cycle_singleExtension() throws Exception { """ ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in + \t\toverride_repo(ext, foo = "my_bar", bar = "my_foo") Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which forms a cycle."""); } @@ -1857,6 +1862,7 @@ public void testOverrideRepo_cycle_multipleExtensions() throws Exception { """ ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in + \t\toverride_repo(ext2, bar = "my_foo") Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which forms a cycle."""); } } diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/testutil/BUILD index 199065a6cf6f47..bdf9e30ce7b839 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/BUILD +++ b/src/test/java/com/google/devtools/build/lib/testutil/BUILD @@ -75,6 +75,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", + "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", "//src/main/protobuf:failure_details_java_proto", "//third_party:error_prone_annotations", diff --git a/src/test/java/com/google/devtools/build/lib/testutil/FoundationTestCase.java b/src/test/java/com/google/devtools/build/lib/testutil/FoundationTestCase.java index 848adffb09f15e..b1f9098fc74ece 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/FoundationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/FoundationTestCase.java @@ -13,8 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.testutil; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.fail; +import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.Event; @@ -24,11 +27,13 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.util.Set; import java.util.regex.Pattern; +import net.starlark.java.eval.EvalException; import org.junit.After; import org.junit.Before; @@ -78,6 +83,18 @@ public final void initializeFileSystemAndDirectories() throws Exception { scratch.file(rootDirectory.getRelative("WORKSPACE").getPathString()); scratch.file(rootDirectory.getRelative("MODULE.bazel").getPathString()); root = Root.fromPath(rootDirectory); + + // Let the Starlark interpreter know how to read source files. + EvalException.setSourceReaderSupplier( + () -> + loc -> { + try { + String content = FileSystemUtils.readContent(fileSystem.getPath(loc.file()), UTF_8); + return Iterables.get(Splitter.on("\n").split(content), loc.line() - 1, null); + } catch (Throwable ignored) { + } + return null; + }); } @Before From 94cdfb833f7a924b99b163abe164aa693bcc4d6c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 16 Sep 2024 13:05:26 +0200 Subject: [PATCH 15/19] Fix test failures --- .../build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 7cf39c6df46600..668ea6b79d2a7e 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -1431,6 +1431,7 @@ public void nonVisibleLabelInLabelAttr() throws Exception { """ ERROR /ws/defs.bzl:9:12: Traceback (most recent call last): \tFile "/ws/defs.bzl", line 9, column 12, in _ext_impl + \t\tdata_repo(name='ext',data='@not_other_repo//:foo') Error in repository_rule: no repository visible as '@not_other_repo' in \ the extension '@@//:defs.bzl%ext', but referenced by label \ '@not_other_repo//:foo' in attribute 'data' of data_repo 'ext'."""); @@ -1475,6 +1476,7 @@ public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception { Traceback (most recent call last): \tFile "/usr/local/google/_blaze_jrluser/FAKEMD5/external/ext_module+/defs.bzl", \ line 9, column 12, in _ext_impl + \t\tdata_repo(name='ext',data='@not_other_repo//:foo') Error in repository_rule: no repository visible as '@not_other_repo' in the extension \ '@@ext_module+//:defs.bzl%ext', but referenced by label '@not_other_repo//:foo' in \ attribute 'data' of data_repo 'ext'."""); @@ -1553,6 +1555,7 @@ public void nonVisibleLabelInLabelListAttr() throws Exception { """ ERROR /ws/defs.bzl:9:12: Traceback (most recent call last): \tFile "/ws/defs.bzl", line 9, column 12, in _ext_impl + \t\tdata_repo(name='ext',data=['@not_other_repo//:foo']) Error in repository_rule: no repository visible as '@not_other_repo' \ in the extension '@@//:defs.bzl%ext', but referenced by label \ '@not_other_repo//:foo' in attribute 'data' of data_repo 'ext'."""); @@ -1589,6 +1592,7 @@ public void nonVisibleLabelInLabelKeyedStringDictAttr() throws Exception { """ ERROR /ws/defs.bzl:9:12: Traceback (most recent call last): \tFile "/ws/defs.bzl", line 9, column 12, in _ext_impl + \t\tdata_repo(name='ext',data={'@not_other_repo//:foo':'bar'}) Error in repository_rule: no repository visible as '@not_other_repo' \ in the extension '@@//:defs.bzl%ext', but referenced by label \ '@not_other_repo//:foo' in attribute 'data' of data_repo 'ext'."""); From e89f6e00395264021afea511115bcbd47d088c6d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 16 Sep 2024 13:45:55 +0200 Subject: [PATCH 16/19] Fix other test --- .../lib/starlark/StarlarkDefinedAspectsTest.java | 1 + .../lib/starlark/StarlarkIntegrationTest.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java index 73e76918d4384a..bbca344aac45a2 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java @@ -871,6 +871,7 @@ def _impl(target, ctx): + "//test:aspect.bzl%MyAspect aspect on java_library rule //test:xxx: \n" + "Traceback (most recent call last):\n" + "\tFile \"/workspace/test/aspect.bzl\", line 2, column 13, in _impl\n" + + "\t\treturn 1 // 0\n" + "Error: integer division by zero"); } diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java index b973a92225bde3..3bde346e2e6314 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java @@ -480,17 +480,17 @@ protected void runStackTraceTest(String expr, String errorMessage) throws Except "Traceback (most recent call last):", "\tFile \"/workspace/test/starlark/extension.bzl\", line 6, column 6, in" + " custom_rule_impl", - // "\t\tfoo()", + "\t\tfoo()", "\tFile \"/workspace/test/starlark/extension.bzl\", line 9, column 6, in foo", - // "\t\tbar(2, 4)", + "\t\tbar(2, 4)", "\tFile \"/workspace/test/starlark/extension.bzl\", line 11, column 8, in bar", - // "\t\tfirst(x, y, z)", + "\t\tfirst(x, y, z)", "\tFile \"/workspace/test/starlark/functions.bzl\", line 2, column 9, in first", - // "\t\tsecond(a, b)", + "\t\tsecond(a, b)", "\tFile \"/workspace/test/starlark/functions.bzl\", line 5, column 8, in second", - // "\t\tthird(\"legal\")", + "\t\tthird('legal')", "\tFile \"/workspace/test/starlark/functions.bzl\", line 7, column 12, in third", - // ... + "\t\t" + expr.stripLeading(), errorMessage); scratch.file( "test/starlark/extension.bzl", @@ -503,9 +503,9 @@ def custom_rule_impl(ctx): foo() return [MyInfo(provider_key = ftb)] def foo(): - bar(2,4) + bar(2, 4) def bar(x,y,z=1): - first(x,y, z) + first(x, y, z) custom_rule = rule(implementation = custom_rule_impl, attrs = {'attr1': attr.label_list(mandatory=True, allow_files=True)}) """); From 44ae64b73321b7756a0d1e907f04fdf06bf7475b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 19 Sep 2024 14:46:45 +0200 Subject: [PATCH 17/19] Address comments --- .../lib/bazel/bzlmod/ModuleThreadContext.java | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index e756e910b78d33..100f869d58b5ef 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -144,7 +144,7 @@ static class ModuleExtensionUsageBuilder { private final boolean isolate; private final ArrayList proxyBuilders; private final HashBiMap imports; - private final Map repoOverrides; + private final Map repoOverrides; private final ImmutableList.Builder tags; ModuleExtensionUsageBuilder( @@ -158,7 +158,7 @@ static class ModuleExtensionUsageBuilder { this.isolate = isolate; this.proxyBuilders = new ArrayList<>(); this.imports = HashBiMap.create(); - this.repoOverrides = HashBiMap.create(); + this.repoOverrides = new HashMap<>(); this.tags = ImmutableList.builder(); } @@ -201,8 +201,7 @@ public void addRepoOverride( RepoOverride collision = repoOverrides.put( overriddenRepoName, - new ModuleThreadContext.RepoOverride( - overriddenRepoName, overridingRepoName, mustExist, this, stack)); + new RepoOverride(overriddenRepoName, overridingRepoName, mustExist, this, stack)); if (collision != null) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at" @@ -239,20 +238,20 @@ ModuleExtensionUsage buildUsage() throws EvalException { } for (var override : repoOverrides.entrySet()) { - String exportedName = override.getKey(); - String localRepoName = override.getValue().overridingRepoName; - if (!context.repoNameUsages.containsKey(localRepoName)) { + String overriddenRepoName = override.getKey(); + String overridingRepoName = override.getValue().overridingRepoName; + if (!context.repoNameUsages.containsKey(overridingRepoName)) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is overridden with '%s', but" + " no repo is visible under this name", - exportedName, extensionName, localRepoName) + overriddenRepoName, extensionName, overridingRepoName) .withCallStack(override.getValue().stack); } - String importedAs = imports.inverse().get(exportedName); + String importedAs = imports.inverse().get(overriddenRepoName); if (importedAs != null) { context.overriddenRepos.put(importedAs, override.getValue()); } - context.overridingRepos.put(localRepoName, override.getValue()); + context.overridingRepos.put(overridingRepoName, override.getValue()); } builder.setRepoOverrides( ImmutableMap.copyOf( From 0b9fab0f7ec816c46aff4e51dec62a8ca1c5450c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 19 Sep 2024 18:19:48 +0200 Subject: [PATCH 18/19] Address comments --- .../bazel/bzlmod/BazelDepGraphFunction.java | 4 +- .../lib/bazel/bzlmod/BazelDepGraphValue.java | 2 +- .../bazel/bzlmod/ModuleExtensionUsage.java | 24 ++++----- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 49 ++++++++++--------- .../lib/bazel/bzlmod/ModuleThreadContext.java | 39 +++++---------- .../bzlmod/SingleExtensionUsagesValue.java | 1 + .../bzlmod/BazelDepGraphFunctionTest.java | 1 + .../bazel/bzlmod/ModuleFileFunctionTest.java | 14 ++++-- .../bazel/bzlmod/StarlarkBazelModuleTest.java | 3 +- .../bzlmod/modcommand/ModExecutorTest.java | 4 ++ 10 files changed, 73 insertions(+), 68 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index c4bde42e1e3d99..3845c0881ec0d0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -208,7 +208,7 @@ private static ImmutableTable resolve ImmutableTable extensionUsagesTable, ImmutableMap extensionUniqueNames, ImmutableBiMap canonicalRepoNameLookup) { - RepositoryMapping rootModuleMapping = + RepositoryMapping rootModuleMappingWithoutOverrides = BazelDepGraphValue.getRepositoryMapping( ModuleKey.ROOT, depGraph, @@ -228,7 +228,7 @@ private static ImmutableTable resolve repoOverridesBuilder.put( extensionId, override.getKey(), - rootModuleMapping.get(override.getValue().overridingRepoName())); + rootModuleMappingWithoutOverrides.get(override.getValue().overridingRepoName())); } } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java index 011202e9ad913a..4a060d45a7bd98 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java @@ -108,7 +108,7 @@ public static BazelDepGraphValue createEmptyDepGraph() { /** * For each module extension, a mapping from the name of the repo exported by the extension to the - * canonical repo name of the repo that should override it (if any). + * canonical name of the repo that should override it (if any). */ public abstract ImmutableTable getRepoOverrides(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index 2bb65bc350b70b..046e2c6d9f72c4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -131,26 +131,29 @@ public final boolean getHasNonDevUseExtension() { return getProxies().stream().anyMatch(p -> !p.isDevDependency()); } + /** + * Represents a repo that overrides another repo within the scope of the extension. + * + * @param overridingRepoName The apparent name of the overriding repo in the root module. + * @param mustExist Whether this override should apply to an existing repo. + * @param location The location of the {@code override_repo} or {@code inject_repo} call. + */ @GenerateTypeAdapter - public record RepoOverride( - // The apparent name of the overriding repo in the root module. - String overridingRepoName, - // Whether this override should apply to an existing repo. - boolean mustExist, - Location location) {} + public record RepoOverride(String overridingRepoName, boolean mustExist, Location location) {} /** - * Maps repo names local to the extension to repo they are overridden by. + * Contains information about overrides that apply to repos generated by this extension. Keyed by + * the extension-local repo name. * *

This is only non-empty for root module usages and repos that override other repos are not - * themselves overridden. + * themselves overridden, so overrides do need to be applied recursively. */ public abstract ImmutableMap getRepoOverrides(); public abstract Builder toBuilder(); public static Builder builder() { - return new AutoValue_ModuleExtensionUsage.Builder().setRepoOverrides(ImmutableMap.of()); + return new AutoValue_ModuleExtensionUsage.Builder(); } /** @@ -208,8 +211,7 @@ public Builder addTag(Tag value) { } @CanIgnoreReturnValue - public abstract Builder setRepoOverrides( - ImmutableMap extensionLocalToApparentName); + public abstract Builder setRepoOverrides(ImmutableMap repoOverrides); public abstract ModuleExtensionUsage build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index ef582e69efdf27..42859959887c66 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -648,12 +648,13 @@ public void useRepo( @StarlarkMethod( name = "override_repo", doc = - "Overrides one or more repos defined by the given module extension with the given repos" - + " visible to the current module. This is ignored if the current module is not the" - + " root module or `--ignore_dev_dependency` is enabled." - + "" - + "

Use inject_repo instead to add a new" - + " repo.", + """ + Overrides one or more repos defined by the given module extension with the given repos + visible to the current module. This is ignored if the current module is not the root + module or `--ignore_dev_dependency` is enabled. + +

Use inject_repo instead to add a new repo. + """, parameters = { @Param( name = "extension_proxy", @@ -663,15 +664,17 @@ public void useRepo( @Param( name = "args", doc = - "The repos in the extension that should be overridden with the repos of the same" - + " name in the current module."), + """ + The repos in the extension that should be overridden with the repos of the same + name in the current module."""), extraKeywords = @Param( name = "kwargs", doc = - "The overrides to apply to the repos generated by the extension, where the values" - + " are the names of repos in the scope of the current module and the keys" - + " are the names of the repos they will override in the extension."), + """ + The overrides to apply to the repos generated by the extension, where the values + are the names of repos in the scope of the current module and the keys are the + names of the repos they will override in the extension."""), useStarlarkThread = true) public void overrideRepo( ModuleExtensionProxy extensionProxy, @@ -699,12 +702,12 @@ public void overrideRepo( @StarlarkMethod( name = "inject_repo", doc = - "Injects one or more new repos into the given module extension." - + " This is ignored if the current module is not the root module or" - + " `--ignore_dev_dependency` is enabled." - + "" - + "

Use override_repo instead to" - + " override an existing repo.", + """ + Injects one or more new repos into the given module extension. + This is ignored if the current module is not the root module or `--ignore_dev_dependency` + is enabled. +

Use override_repo instead to override an + existing repo.""", parameters = { @Param( name = "extension_proxy", @@ -714,15 +717,17 @@ public void overrideRepo( @Param( name = "args", doc = - "The repos visible to the current module that should be injected into the" - + " extension under the same name."), + """ + The repos visible to the current module that should be injected into the + extension under the same name."""), extraKeywords = @Param( name = "kwargs", doc = - "The new repos to inject into the extension, where the values are the names of" - + " repos in the scope of the current module and the keys are the name they" - + " will be visible under in the extension."), + """ + The new repos to inject into the extension, where the values are the names of + repos in the scope of the current module and the keys are the name they will be + visible under in the extension."""), useStarlarkThread = true) public void injectRepo( ModuleExtensionProxy extensionProxy, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 100f869d58b5ef..8d149e6cfa9417 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -83,7 +83,7 @@ record RepoOverride( String overriddenRepoName, String overridingRepoName, boolean mustExist, - ModuleExtensionUsageBuilder extensionUsageBuilder, + String extensionName, ImmutableList stack) { Location location() { // Skip over the override_repo builtin frame. @@ -201,7 +201,8 @@ public void addRepoOverride( RepoOverride collision = repoOverrides.put( overriddenRepoName, - new RepoOverride(overriddenRepoName, overridingRepoName, mustExist, this, stack)); + new RepoOverride( + overriddenRepoName, overridingRepoName, mustExist, extensionName, stack)); if (collision != null) { throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at" @@ -334,31 +335,15 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti if (overridingAndOverridden.isPresent()) { var override = overridingRepos.get(overridingAndOverridden.get()); var overrideOnOverride = overriddenRepos.get(overridingAndOverridden.get()); - if (override.overriddenRepoName.equals( - override.extensionUsageBuilder.imports.get(overrideOnOverride.overridingRepoName))) { - throw Starlark.errorf( - "The repo '%s' used as an override for '%s' in module extension '%s' is itself" - + " overridden with '%s' at %s, which forms a cycle.", - override.overridingRepoName, - override.overriddenRepoName, - override.extensionUsageBuilder.extensionName, - overrideOnOverride.overridingRepoName, - overrideOnOverride.location()) - .withCallStack(override.stack); - } else { - throw Starlark.errorf( - "The repo '%s' used as an override for '%s' in module extension '%s' is itself" - + " overridden with '%s' at %s, which is not supported. Please directly" - + " override '%s' with '%s' instead.", - override.overridingRepoName, - override.overriddenRepoName, - override.extensionUsageBuilder.extensionName, - overrideOnOverride.overridingRepoName, - overrideOnOverride.location(), - override.overriddenRepoName, - overrideOnOverride.overridingRepoName) - .withCallStack(override.stack); - } + throw Starlark.errorf( + "The repo '%s' used as an override for '%s' in module extension '%s' is itself" + + " overridden with '%s' at %s, which is not supported.", + override.overridingRepoName, + override.overriddenRepoName, + override.extensionName, + overrideOnOverride.overridingRepoName, + overrideOnOverride.location()) + .withCallStack(override.stack); } return module diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java index 6724f1a48a1a70..a812b25f906a84 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java @@ -57,6 +57,7 @@ public abstract class SingleExtensionUsagesValue implements SkyValue { /** The repo mappings to use for each module that used this extension. */ public abstract ImmutableMap getRepoMappings(); + /** Maps an extension-local repo name to the canonical name of the repo it is overridden with. */ public abstract ImmutableMap getRepoOverrides(); public static SingleExtensionUsagesValue create( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java index 763be302dd8ab1..218b298eecaa9f 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java @@ -220,6 +220,7 @@ private static ModuleExtensionUsage createModuleExtensionUsage( .setExtensionBzlFile(bzlFile) .setExtensionName(name) .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setDevDependency(false) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index a824ac6e331054..b90da7aad52d18 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -776,6 +776,7 @@ public void testModuleExtensions_good() throws Exception { .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext1") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -806,6 +807,7 @@ public void testModuleExtensions_good() throws Exception { .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext2") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -850,6 +852,7 @@ public void testModuleExtensions_good() throws Exception { .setExtensionBzlFile("@rules_jvm_external//:defs.bzl") .setExtensionName("maven") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -927,6 +930,7 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { .setExtensionBzlFile("@//:defs.bzl") .setExtensionName("myext") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -1058,6 +1062,7 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -1177,6 +1182,7 @@ public void testModuleExtensions_innate() throws Exception { .setExtensionBzlFile("//:MODULE.bazel") .setExtensionName("_repo_rules") .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation( @@ -1781,7 +1787,7 @@ public void testOverrideRepo_chain_singleExtension() throws Exception { ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in \t\toverride_repo(ext, baz = "bar") - Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); + Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported."""); } @Test @@ -1809,7 +1815,7 @@ public void testOverrideRepo_chain_multipleExtensions() throws Exception { ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in \t\toverride_repo(ext1, baz = "bar") - Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead."""); + Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported."""); } @Test @@ -1835,7 +1841,7 @@ public void testOverrideRepo_cycle_singleExtension() throws Exception { ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in \t\toverride_repo(ext, foo = "my_bar", bar = "my_foo") - Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which forms a cycle."""); + Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which is not supported."""); } @Test @@ -1863,6 +1869,6 @@ public void testOverrideRepo_cycle_multipleExtensions() throws Exception { ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last): \tFile "/workspace/MODULE.bazel", line 5, column 14, in \t\toverride_repo(ext2, bar = "my_foo") - Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which forms a cycle."""); + Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which is not supported."""); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java index 3d0adf3bbaeb84..439f0c8cf449f4 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java @@ -46,7 +46,8 @@ private static ModuleExtensionUsage.Builder getBaseUsageBuilder() { return ModuleExtensionUsage.builder() .setExtensionBzlFile("//:rje.bzl") .setExtensionName("maven") - .setIsolationKey(Optional.empty()); + .setIsolationKey(Optional.empty()) + .setRepoOverrides(ImmutableMap.of()); } /** A builder for ModuleExtension that sets all the mandatory but irrelevant fields. */ diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand/ModExecutorTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand/ModExecutorTest.java index 6ac2a9d7714c4b..9deb60bf20849e 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand/ModExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand/ModExecutorTest.java @@ -607,6 +607,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("//extensions:extensions.bzl") .setExtensionName("maven") + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation(Location.fromFileLineColumn("C@1.0/MODULE.bazel", 2, 23)) @@ -621,6 +622,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("//extensions:extensions.bzl") .setExtensionName("maven") + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation(Location.fromFileLineColumn("D@1.0/MODULE.bazel", 1, 10)) @@ -635,6 +637,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("//extensions:extensions.bzl") .setExtensionName("gradle") + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation(Location.fromFileLineColumn("Y@2.0/MODULE.bazel", 2, 13)) @@ -649,6 +652,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("//extensions:extensions.bzl") .setExtensionName("maven") + .setRepoOverrides(ImmutableMap.of()) .addProxy( ModuleExtensionUsage.Proxy.builder() .setLocation(Location.fromFileLineColumn("Y@2.0/MODULE.bazel", 13, 10)) From a4cc4fd0a8480ac7def160327224545257ceb52d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 19 Sep 2024 22:26:34 +0200 Subject: [PATCH 19/19] Simplify comment --- .../devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index 046e2c6d9f72c4..f4fc11683fe970 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -145,8 +145,7 @@ public record RepoOverride(String overridingRepoName, boolean mustExist, Locatio * Contains information about overrides that apply to repos generated by this extension. Keyed by * the extension-local repo name. * - *

This is only non-empty for root module usages and repos that override other repos are not - * themselves overridden, so overrides do need to be applied recursively. + *

This is only non-empty for root module usages. */ public abstract ImmutableMap getRepoOverrides();