From 3c3b1ec9b355fcffc3fc00617bbbfb14eecc7c95 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Thu, 13 Jul 2023 09:43:28 -0700 Subject: [PATCH] Identify isolated extensions by exported name (#18923) Instead of making a running counter and the `dev_dependency` bit the identifier of an isolated module extension usage within a module file, use the exported name of the usage. This still results in a stable name even with `--ignore_dev_dependency` flips, but makes the canonical repository name more recognizable and allows for less verbose buildozer fixup commands. Closes #18840. PiperOrigin-RevId: 547587673 Change-Id: I2137ed83f1600c00f73539c8a3f002268e4c0476 Co-authored-by: Fabian Meumertzheim --- .../bazel/bzlmod/BazelDepGraphFunction.java | 9 +-- .../lib/bazel/bzlmod/ModuleExtensionId.java | 19 ++--- .../bazel/bzlmod/ModuleExtensionMetadata.java | 25 ++++--- .../lib/bazel/bzlmod/ModuleFileFunction.java | 26 ++++++- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 70 ++++++++++--------- .../bzlmod/ModuleExtensionResolutionTest.java | 17 ++--- .../bazel/bzlmod/ModuleFileFunctionTest.java | 17 ++++- 7 files changed, 102 insertions(+), 81 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 9e3ed795734e92..eaf358a011d05b 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 @@ -257,22 +257,17 @@ private ImmutableBiMap calculateUniqueNameForUsedExte // When using a namespace, prefix the extension name with "_" to distinguish the prefix from // those generated by non-namespaced extension usages. Extension names are identified by their // Starlark identifier, which in the case of an exported symbol cannot start with "_". - // We also include whether the isolated usage is a dev usage as well as its index in the - // MODULE.bazel file to ensure that canonical repository names don't change depending on - // whether dev dependencies are ignored. This removes potential for confusion and also - // prevents unnecessary refetches when --ignore_dev_dependency is toggled. String bestName = id.getIsolationKey() .map( namespace -> String.format( - "%s~_%s~%s~%s~%s%d", + "%s~_%s~%s~%s~%s", nonEmptyRepoPart, id.getExtensionName(), namespace.getModule().getName(), namespace.getModule().getVersion(), - namespace.isDevUsage() ? "dev" : "", - namespace.getIsolatedUsageIndex())) + namespace.getUsageExportedName())) .orElse(nonEmptyRepoPart + "~" + id.getExtensionName()); if (extensionUniqueNames.putIfAbsent(bestName, id) == null) { continue; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java index 7448a0ced179ff..7690cf0ef884f5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.bazel.bzlmod; import static com.google.common.collect.Comparators.emptiesFirst; -import static com.google.common.primitives.Booleans.falseFirst; import static java.util.Comparator.comparing; import com.google.auto.value.AutoValue; @@ -39,24 +38,16 @@ public abstract class ModuleExtensionId { abstract static class IsolationKey { static final Comparator LEXICOGRAPHIC_COMPARATOR = comparing(IsolationKey::getModule, ModuleKey.LEXICOGRAPHIC_COMPARATOR) - .thenComparing(IsolationKey::isDevUsage, falseFirst()) - .thenComparing(IsolationKey::getIsolatedUsageIndex); + .thenComparing(IsolationKey::getUsageExportedName); /** The module which contains this isolated usage of a module extension. */ public abstract ModuleKey getModule(); - /** Whether this isolated usage specified {@code dev_dependency = True}. */ - public abstract boolean isDevUsage(); + /** The exported name of the first extension proxy for this usage. */ + public abstract String getUsageExportedName(); - /** - * The 0-based index of this isolated usage within the module's isolated usages of the same - * module extension and with the same {@link #isDevUsage()} value. - */ - public abstract int getIsolatedUsageIndex(); - - public static IsolationKey create( - ModuleKey module, boolean isDevUsage, int isolatedUsageIndex) { - return new AutoValue_ModuleExtensionId_IsolationKey(module, isDevUsage, isolatedUsageIndex); + public static IsolationKey create(ModuleKey module, String usageExportedName) { + return new AutoValue_ModuleExtensionId_IsolationKey(module, usageExportedName); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index 2bdd9f4e08abee..f71986f509c8c9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; import java.util.Optional; @@ -341,22 +342,20 @@ private static Optional makeUseRepoCommand( return Optional.empty(); } - String extensionUsageIdentifier = extensionName; + var commandParts = new ArrayList(); + commandParts.add(cmd); if (isolationKey.isPresent()) { - // We verified in create() that the extension did not report root module deps of a kind that - // does not match the isolated (and hence only) usage. It also isn't possible for users to - // specify repo usages of the wrong kind, so we can't get here. - Preconditions.checkState(isolationKey.get().isDevUsage() == devDependency); - extensionUsageIdentifier += ":" + isolationKey.get().getIsolatedUsageIndex(); + commandParts.add(isolationKey.get().getUsageExportedName()); + } else { + if (devDependency) { + commandParts.add("dev"); + } + commandParts.add(extensionBzlFile); + commandParts.add(extensionName); } + commandParts.addAll(repos); return Optional.of( - String.format( - "buildozer '%s%s %s %s %s' //MODULE.bazel:all", - cmd, - devDependency ? " dev" : "", - extensionBzlFile, - extensionUsageIdentifier, - String.join(" ", repos))); + String.format("buildozer '%s' //MODULE.bazel:all", String.join(" ", commandParts))); } private Optional> getRootModuleDirectDeps(Set allRepos) 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 d45a255f6945ea..89792cb41bec0c 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 @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.packages.StarlarkExportable; import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code; import com.google.devtools.build.lib.skyframe.ClientEnvironmentFunction; @@ -147,7 +148,13 @@ public SkyValue compute(SkyKey skyKey, Environment env) env); // Perform some sanity checks. - InterimModule module = moduleFileGlobals.buildModule(); + InterimModule module; + try { + module = moduleFileGlobals.buildModule(); + } catch (EvalException e) { + env.getListener().handle(Event.error(e.getMessageWithStack())); + throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey); + } if (!module.getName().equals(moduleKey.getName())) { throw errorf( Code.BAD_MODULE, @@ -203,7 +210,13 @@ private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Envir /* printIsNoop= */ false, starlarkSemantics, env); - InterimModule module = moduleFileGlobals.buildModule(); + InterimModule module; + try { + module = moduleFileGlobals.buildModule(); + } catch (EvalException e) { + env.getListener().handle(Event.error(e.getMessageWithStack())); + throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for the root module"); + } for (ModuleExtensionUsage usage : module.getExtensionUsages()) { if (usage.getIsolationKey().isPresent() && usage.getImports().isEmpty()) { throw errorf( @@ -271,6 +284,15 @@ private ModuleFileGlobals execModuleFile( } else { thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); } + thread.setPostAssignHook( + (name, value) -> { + if (value instanceof StarlarkExportable) { + StarlarkExportable exportable = (StarlarkExportable) value; + if (!exportable.isExported()) { + exportable.export(env.getListener(), null, name); + } + } + }); Starlark.execFileProgram(program, predeclaredEnv, thread); } catch (SyntaxError.Exception e) { Event.replayEventsOn(env.getListener(), e.errors()); 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 cce6323dca9002..3c45f89a4f17db 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,8 +15,6 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import static com.google.common.collect.ImmutableList.toImmutableList; - import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.HashBiMap; @@ -29,7 +27,10 @@ import com.google.devtools.build.lib.bazel.bzlmod.InterimModule.DepSpec; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.packages.StarlarkExportable; import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; @@ -71,8 +72,6 @@ public class ModuleFileGlobals { private final List extensionUsageBuilders = new ArrayList<>(); private final Map overrides = new HashMap<>(); private final Map repoNameUsages = new HashMap<>(); - private int nextIsolatedNonDevUsageIndex = 0; - private int nextIsolatedDevUsageIndex = 0; public ModuleFileGlobals( ImmutableMap builtinModules, @@ -451,22 +450,9 @@ public ModuleExtensionProxy useExtension( hadNonModuleCall = true; String extensionBzlFile = normalizeLabelString(rawExtensionBzlFile); - - Optional isolationKey; - if (isolate) { - isolationKey = - Optional.of( - ModuleExtensionId.IsolationKey.create( - module.getKey(), - devDependency, - devDependency ? nextIsolatedDevUsageIndex++ : nextIsolatedNonDevUsageIndex++)); - } else { - isolationKey = Optional.empty(); - } - ModuleExtensionUsageBuilder newUsageBuilder = new ModuleExtensionUsageBuilder( - extensionBzlFile, extensionName, isolationKey, thread.getCallerLocation()); + extensionBzlFile, extensionName, isolate, thread.getCallerLocation()); if (ignoreDevDeps && devDependency) { // This is a no-op proxy. @@ -479,7 +465,7 @@ public ModuleExtensionProxy useExtension( for (ModuleExtensionUsageBuilder usageBuilder : extensionUsageBuilders) { if (usageBuilder.extensionBzlFile.equals(extensionBzlFile) && usageBuilder.extensionName.equals(extensionName) - && usageBuilder.isolationKey.isEmpty()) { + && !usageBuilder.isolate) { return usageBuilder.getProxy(devDependency); } } @@ -509,7 +495,7 @@ private String normalizeLabelString(String rawExtensionBzlFile) { class ModuleExtensionUsageBuilder { private final String extensionBzlFile; private final String extensionName; - private final Optional isolationKey; + private final boolean isolate; private final Location location; private final HashBiMap imports; private final ImmutableSet.Builder devImports; @@ -517,27 +503,24 @@ class ModuleExtensionUsageBuilder { private boolean hasNonDevUseExtension; private boolean hasDevUseExtension; + private String exportedName; ModuleExtensionUsageBuilder( - String extensionBzlFile, - String extensionName, - Optional isolationKey, - Location location) { + String extensionBzlFile, String extensionName, boolean isolate, Location location) { this.extensionBzlFile = extensionBzlFile; this.extensionName = extensionName; - this.isolationKey = isolationKey; + this.isolate = isolate; this.location = location; this.imports = HashBiMap.create(); this.devImports = ImmutableSet.builder(); this.tags = ImmutableList.builder(); } - ModuleExtensionUsage buildUsage() { + ModuleExtensionUsage buildUsage() throws EvalException { var builder = ModuleExtensionUsage.builder() .setExtensionBzlFile(extensionBzlFile) .setExtensionName(extensionName) - .setIsolationKey(isolationKey) .setUsingModule(module.getKey()) .setLocation(location) .setImports(ImmutableBiMap.copyOf(imports)) @@ -545,6 +528,16 @@ ModuleExtensionUsage buildUsage() { .setHasDevUseExtension(hasDevUseExtension) .setHasNonDevUseExtension(hasNonDevUseExtension) .setTags(tags.build()); + if (isolate) { + if (exportedName == null) { + throw Starlark.errorf( + "Isolated extension usage at %s must be assigned to a top-level variable", location); + } + builder.setIsolationKey( + Optional.of(ModuleExtensionId.IsolationKey.create(module.getKey(), exportedName))); + } else { + builder.setIsolationKey(Optional.empty()); + } return builder.build(); } @@ -562,7 +555,7 @@ ModuleExtensionProxy getProxy(boolean devDependency) { } @StarlarkBuiltin(name = "module_extension_proxy", documented = false) - class ModuleExtensionProxy implements Structure { + class ModuleExtensionProxy implements Structure, StarlarkExportable { private final boolean devDependency; @@ -619,6 +612,16 @@ public ImmutableCollection getFieldNames() { public String getErrorMessageForUnknownField(String field) { return null; } + + @Override + public boolean isExported() { + return exportedName != null; + } + + @Override + public void export(EventHandler handler, Label bzlFileLabel, String name) { + exportedName = name; + } } } @@ -977,14 +980,15 @@ public void localPathOverride(String moduleName, String path) throws EvalExcepti addOverride(moduleName, LocalPathOverride.create(path)); } - public InterimModule buildModule() { + public InterimModule buildModule() throws EvalException { + var extensionUsages = ImmutableList.builder(); + for (var extensionUsageBuilder : extensionUsageBuilders) { + extensionUsages.add(extensionUsageBuilder.buildUsage()); + } return module .setDeps(ImmutableMap.copyOf(deps)) .setOriginalDeps(ImmutableMap.copyOf(deps)) - .setExtensionUsages( - extensionUsageBuilders.stream() - .map(ModuleExtensionUsageBuilder::buildUsage) - .collect(toImmutableList())) + .setExtensionUsages(extensionUsages.build()) .build(); } 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 264c0d91168314..3e8bb0920f5b2a 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 @@ -2129,9 +2129,8 @@ public void extensionMetadata_isolated() throws Exception { + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + " issues:\033[0m\n" + "\n" - + "buildozer 'use_repo_add @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'" - + " //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove @ext//:defs.bzl ext:0 indirect_dep' //MODULE.bazel:all", + + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all", ImmutableSet.of(EventKind.WARNING)); assertContainsEvent( "WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl" @@ -2144,8 +2143,7 @@ public void extensionMetadata_isolated() throws Exception { + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + " issues:\033[0m\n" + "\n" - + "buildozer 'use_repo_add @ext//:defs.bzl ext:1 missing_direct_dep'" - + " //MODULE.bazel:all", + + "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all", ImmutableSet.of(EventKind.WARNING)); } @@ -2215,10 +2213,8 @@ public void extensionMetadata_isolatedDev() throws Exception { + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + " issues:\033[0m\n" + "\n" - + "buildozer 'use_repo_add dev @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'" - + " //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext:0 indirect_dep'" - + " //MODULE.bazel:all", + + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all", ImmutableSet.of(EventKind.WARNING)); assertContainsEvent( "WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl" @@ -2231,8 +2227,7 @@ public void extensionMetadata_isolatedDev() throws Exception { + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + " issues:\033[0m\n" + "\n" - + "buildozer 'use_repo_add dev @ext//:defs.bzl ext:1 missing_direct_dep'" - + " //MODULE.bazel:all", + + "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all", ImmutableSet.of(EventKind.WARNING)); } 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 f265942feb76b4..3ee5f0d59ff4be 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 @@ -1048,7 +1048,7 @@ public void module_calledLate() throws Exception { } @Test - public void isolatedExtensionWithoutImports() throws Exception { + public void isolatedUsageWithoutImports() throws Exception { scratch.file( rootDirectory.getRelative("MODULE.bazel").getPathString(), "isolated_ext = use_extension('//:extensions.bzl', 'my_ext', isolate = True)"); @@ -1066,4 +1066,19 @@ public void isolatedExtensionWithoutImports() throws Exception { + "Either import one or more repositories generated by the extension with " + "use_repo or remove the usage."); } + + @Test + public void isolatedUsageNotExported() throws Exception { + scratch.file( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + "use_extension('//:extensions.bzl', 'my_ext', isolate = True)"); + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); + + reporter.removeHandler(failFastHandler); // expect failures + evaluator.evaluate(ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertContainsEvent( + "Isolated extension usage at /workspace/MODULE.bazel:1:14 must be assigned to a " + + "top-level variable"); + } }