Skip to content

Commit eb2638e

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Identify isolated extensions by exported name
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
1 parent 61a2cbb commit eb2638e

7 files changed

Lines changed: 102 additions & 81 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -257,22 +257,17 @@ private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExte
257257
// When using a namespace, prefix the extension name with "_" to distinguish the prefix from
258258
// those generated by non-namespaced extension usages. Extension names are identified by their
259259
// Starlark identifier, which in the case of an exported symbol cannot start with "_".
260-
// We also include whether the isolated usage is a dev usage as well as its index in the
261-
// MODULE.bazel file to ensure that canonical repository names don't change depending on
262-
// whether dev dependencies are ignored. This removes potential for confusion and also
263-
// prevents unnecessary refetches when --ignore_dev_dependency is toggled.
264260
String bestName =
265261
id.getIsolationKey()
266262
.map(
267263
namespace ->
268264
String.format(
269-
"%s~_%s~%s~%s~%s%d",
265+
"%s~_%s~%s~%s~%s",
270266
nonEmptyRepoPart,
271267
id.getExtensionName(),
272268
namespace.getModule().getName(),
273269
namespace.getModule().getVersion(),
274-
namespace.isDevUsage() ? "dev" : "",
275-
namespace.getIsolatedUsageIndex()))
270+
namespace.getUsageExportedName()))
276271
.orElse(nonEmptyRepoPart + "~" + id.getExtensionName());
277272
if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
278273
continue;

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

1818
import static com.google.common.collect.Comparators.emptiesFirst;
19-
import static com.google.common.primitives.Booleans.falseFirst;
2019
import static java.util.Comparator.comparing;
2120

2221
import com.google.auto.value.AutoValue;
@@ -39,24 +38,16 @@ public abstract class ModuleExtensionId {
3938
abstract static class IsolationKey {
4039
static final Comparator<IsolationKey> LEXICOGRAPHIC_COMPARATOR =
4140
comparing(IsolationKey::getModule, ModuleKey.LEXICOGRAPHIC_COMPARATOR)
42-
.thenComparing(IsolationKey::isDevUsage, falseFirst())
43-
.thenComparing(IsolationKey::getIsolatedUsageIndex);
41+
.thenComparing(IsolationKey::getUsageExportedName);
4442

4543
/** The module which contains this isolated usage of a module extension. */
4644
public abstract ModuleKey getModule();
4745

48-
/** Whether this isolated usage specified {@code dev_dependency = True}. */
49-
public abstract boolean isDevUsage();
46+
/** The exported name of the first extension proxy for this usage. */
47+
public abstract String getUsageExportedName();
5048

51-
/**
52-
* The 0-based index of this isolated usage within the module's isolated usages of the same
53-
* module extension and with the same {@link #isDevUsage()} value.
54-
*/
55-
public abstract int getIsolatedUsageIndex();
56-
57-
public static IsolationKey create(
58-
ModuleKey module, boolean isDevUsage, int isolatedUsageIndex) {
59-
return new AutoValue_ModuleExtensionId_IsolationKey(module, isDevUsage, isolatedUsageIndex);
49+
public static IsolationKey create(ModuleKey module, String usageExportedName) {
50+
return new AutoValue_ModuleExtensionId_IsolationKey(module, usageExportedName);
6051
}
6152
}
6253

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.devtools.build.lib.cmdline.RepositoryName;
2727
import com.google.devtools.build.lib.events.Event;
2828
import com.google.devtools.build.lib.events.EventHandler;
29+
import java.util.ArrayList;
2930
import java.util.Collection;
3031
import java.util.LinkedHashSet;
3132
import java.util.Optional;
@@ -319,22 +320,20 @@ private static Optional<String> makeUseRepoCommand(
319320
return Optional.empty();
320321
}
321322

322-
String extensionUsageIdentifier = extensionName;
323+
var commandParts = new ArrayList<String>();
324+
commandParts.add(cmd);
323325
if (isolationKey.isPresent()) {
324-
// We verified in create() that the extension did not report root module deps of a kind that
325-
// does not match the isolated (and hence only) usage. It also isn't possible for users to
326-
// specify repo usages of the wrong kind, so we can't get here.
327-
Preconditions.checkState(isolationKey.get().isDevUsage() == devDependency);
328-
extensionUsageIdentifier += ":" + isolationKey.get().getIsolatedUsageIndex();
326+
commandParts.add(isolationKey.get().getUsageExportedName());
327+
} else {
328+
if (devDependency) {
329+
commandParts.add("dev");
330+
}
331+
commandParts.add(extensionBzlFile);
332+
commandParts.add(extensionName);
329333
}
334+
commandParts.addAll(repos);
330335
return Optional.of(
331-
String.format(
332-
"buildozer '%s%s %s %s %s' //MODULE.bazel:all",
333-
cmd,
334-
devDependency ? " dev" : "",
335-
extensionBzlFile,
336-
extensionUsageIdentifier,
337-
String.join(" ", repos)));
336+
String.format("buildozer '%s' //MODULE.bazel:all", String.join(" ", commandParts)));
338337
}
339338

340339
private Optional<ImmutableSet<String>> getRootModuleDirectDeps(Set<String> allRepos)

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.google.devtools.build.lib.cmdline.LabelConstants;
2828
import com.google.devtools.build.lib.cmdline.RepositoryName;
2929
import com.google.devtools.build.lib.events.Event;
30+
import com.google.devtools.build.lib.packages.StarlarkExportable;
3031
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
3132
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
3233
import com.google.devtools.build.lib.skyframe.ClientEnvironmentFunction;
@@ -147,7 +148,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
147148
env);
148149

149150
// Perform some sanity checks.
150-
InterimModule module = moduleFileGlobals.buildModule();
151+
InterimModule module;
152+
try {
153+
module = moduleFileGlobals.buildModule();
154+
} catch (EvalException e) {
155+
env.getListener().handle(Event.error(e.getMessageWithStack()));
156+
throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey);
157+
}
151158
if (!module.getName().equals(moduleKey.getName())) {
152159
throw errorf(
153160
Code.BAD_MODULE,
@@ -203,7 +210,13 @@ private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Envir
203210
/* printIsNoop= */ false,
204211
starlarkSemantics,
205212
env);
206-
InterimModule module = moduleFileGlobals.buildModule();
213+
InterimModule module;
214+
try {
215+
module = moduleFileGlobals.buildModule();
216+
} catch (EvalException e) {
217+
env.getListener().handle(Event.error(e.getMessageWithStack()));
218+
throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for the root module");
219+
}
207220
for (ModuleExtensionUsage usage : module.getExtensionUsages()) {
208221
if (usage.getIsolationKey().isPresent() && usage.getImports().isEmpty()) {
209222
throw errorf(
@@ -271,6 +284,15 @@ private ModuleFileGlobals execModuleFile(
271284
} else {
272285
thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener()));
273286
}
287+
thread.setPostAssignHook(
288+
(name, value) -> {
289+
if (value instanceof StarlarkExportable) {
290+
StarlarkExportable exportable = (StarlarkExportable) value;
291+
if (!exportable.isExported()) {
292+
exportable.export(env.getListener(), null, name);
293+
}
294+
}
295+
});
274296
Starlark.execFileProgram(program, predeclaredEnv, thread);
275297
} catch (SyntaxError.Exception e) {
276298
Event.replayEventsOn(env.getListener(), e.errors());

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java‎

Lines changed: 37 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515

1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

18-
import static com.google.common.collect.ImmutableList.toImmutableList;
19-
2018
import com.google.auto.value.AutoValue;
2119
import com.google.common.annotations.VisibleForTesting;
2220
import com.google.common.collect.HashBiMap;
@@ -30,7 +28,10 @@
3028
import com.google.devtools.build.lib.bazel.bzlmod.InterimModule.DepSpec;
3129
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy;
3230
import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
31+
import com.google.devtools.build.lib.cmdline.Label;
3332
import com.google.devtools.build.lib.cmdline.RepositoryName;
33+
import com.google.devtools.build.lib.events.EventHandler;
34+
import com.google.devtools.build.lib.packages.StarlarkExportable;
3435
import java.util.ArrayList;
3536
import java.util.HashMap;
3637
import java.util.LinkedHashMap;
@@ -72,8 +73,6 @@ public class ModuleFileGlobals {
7273
private final List<ModuleExtensionUsageBuilder> extensionUsageBuilders = new ArrayList<>();
7374
private final Map<String, ModuleOverride> overrides = new HashMap<>();
7475
private final Map<String, RepoNameUsage> repoNameUsages = new HashMap<>();
75-
private int nextIsolatedNonDevUsageIndex = 0;
76-
private int nextIsolatedDevUsageIndex = 0;
7776

7877
public ModuleFileGlobals(
7978
ImmutableMap<String, NonRegistryOverride> builtinModules,
@@ -457,22 +456,9 @@ public ModuleExtensionProxy useExtension(
457456
hadNonModuleCall = true;
458457

459458
String extensionBzlFile = normalizeLabelString(rawExtensionBzlFile);
460-
461-
Optional<ModuleExtensionId.IsolationKey> isolationKey;
462-
if (isolate) {
463-
isolationKey =
464-
Optional.of(
465-
ModuleExtensionId.IsolationKey.create(
466-
module.getKey(),
467-
devDependency,
468-
devDependency ? nextIsolatedDevUsageIndex++ : nextIsolatedNonDevUsageIndex++));
469-
} else {
470-
isolationKey = Optional.empty();
471-
}
472-
473459
ModuleExtensionUsageBuilder newUsageBuilder =
474460
new ModuleExtensionUsageBuilder(
475-
extensionBzlFile, extensionName, isolationKey, thread.getCallerLocation());
461+
extensionBzlFile, extensionName, isolate, thread.getCallerLocation());
476462

477463
if (ignoreDevDeps && devDependency) {
478464
// This is a no-op proxy.
@@ -485,7 +471,7 @@ public ModuleExtensionProxy useExtension(
485471
for (ModuleExtensionUsageBuilder usageBuilder : extensionUsageBuilders) {
486472
if (usageBuilder.extensionBzlFile.equals(extensionBzlFile)
487473
&& usageBuilder.extensionName.equals(extensionName)
488-
&& usageBuilder.isolationKey.isEmpty()) {
474+
&& !usageBuilder.isolate) {
489475
return usageBuilder.getProxy(devDependency);
490476
}
491477
}
@@ -515,42 +501,49 @@ private String normalizeLabelString(String rawExtensionBzlFile) {
515501
class ModuleExtensionUsageBuilder {
516502
private final String extensionBzlFile;
517503
private final String extensionName;
518-
private final Optional<ModuleExtensionId.IsolationKey> isolationKey;
504+
private final boolean isolate;
519505
private final Location location;
520506
private final HashBiMap<String, String> imports;
521507
private final ImmutableSet.Builder<String> devImports;
522508
private final ImmutableList.Builder<Tag> tags;
523509

524510
private boolean hasNonDevUseExtension;
525511
private boolean hasDevUseExtension;
512+
private String exportedName;
526513

527514
ModuleExtensionUsageBuilder(
528-
String extensionBzlFile,
529-
String extensionName,
530-
Optional<ModuleExtensionId.IsolationKey> isolationKey,
531-
Location location) {
515+
String extensionBzlFile, String extensionName, boolean isolate, Location location) {
532516
this.extensionBzlFile = extensionBzlFile;
533517
this.extensionName = extensionName;
534-
this.isolationKey = isolationKey;
518+
this.isolate = isolate;
535519
this.location = location;
536520
this.imports = HashBiMap.create();
537521
this.devImports = ImmutableSet.builder();
538522
this.tags = ImmutableList.builder();
539523
}
540524

541-
ModuleExtensionUsage buildUsage() {
525+
ModuleExtensionUsage buildUsage() throws EvalException {
542526
var builder =
543527
ModuleExtensionUsage.builder()
544528
.setExtensionBzlFile(extensionBzlFile)
545529
.setExtensionName(extensionName)
546-
.setIsolationKey(isolationKey)
547530
.setUsingModule(module.getKey())
548531
.setLocation(location)
549532
.setImports(ImmutableBiMap.copyOf(imports))
550533
.setDevImports(devImports.build())
551534
.setHasDevUseExtension(hasDevUseExtension)
552535
.setHasNonDevUseExtension(hasNonDevUseExtension)
553536
.setTags(tags.build());
537+
if (isolate) {
538+
if (exportedName == null) {
539+
throw Starlark.errorf(
540+
"Isolated extension usage at %s must be assigned to a top-level variable", location);
541+
}
542+
builder.setIsolationKey(
543+
Optional.of(ModuleExtensionId.IsolationKey.create(module.getKey(), exportedName)));
544+
} else {
545+
builder.setIsolationKey(Optional.empty());
546+
}
554547
return builder.build();
555548
}
556549

@@ -568,7 +561,7 @@ ModuleExtensionProxy getProxy(boolean devDependency) {
568561
}
569562

570563
@StarlarkBuiltin(name = "module_extension_proxy", documented = false)
571-
class ModuleExtensionProxy implements Structure {
564+
class ModuleExtensionProxy implements Structure, StarlarkExportable {
572565

573566
private final boolean devDependency;
574567

@@ -625,6 +618,16 @@ public ImmutableCollection<String> getFieldNames() {
625618
public String getErrorMessageForUnknownField(String field) {
626619
return null;
627620
}
621+
622+
@Override
623+
public boolean isExported() {
624+
return exportedName != null;
625+
}
626+
627+
@Override
628+
public void export(EventHandler handler, Label bzlFileLabel, String name) {
629+
exportedName = name;
630+
}
628631
}
629632
}
630633

@@ -982,14 +985,15 @@ public void localPathOverride(String moduleName, String path) throws EvalExcepti
982985
addOverride(moduleName, LocalPathOverride.create(path));
983986
}
984987

985-
public InterimModule buildModule() {
988+
public InterimModule buildModule() throws EvalException {
989+
var extensionUsages = ImmutableList.<ModuleExtensionUsage>builder();
990+
for (var extensionUsageBuilder : extensionUsageBuilders) {
991+
extensionUsages.add(extensionUsageBuilder.buildUsage());
992+
}
986993
return module
987994
.setDeps(ImmutableMap.copyOf(deps))
988995
.setOriginalDeps(ImmutableMap.copyOf(deps))
989-
.setExtensionUsages(
990-
extensionUsageBuilders.stream()
991-
.map(ModuleExtensionUsageBuilder::buildUsage)
992-
.collect(toImmutableList()))
996+
.setExtensionUsages(extensionUsages.build())
993997
.build();
994998
}
995999

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2111,9 +2111,8 @@ public void extensionMetadata_isolated() throws Exception {
21112111
+ "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
21122112
+ " issues:\033[0m\n"
21132113
+ "\n"
2114-
+ "buildozer 'use_repo_add @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'"
2115-
+ " //MODULE.bazel:all\n"
2116-
+ "buildozer 'use_repo_remove @ext//:defs.bzl ext:0 indirect_dep' //MODULE.bazel:all",
2114+
+ "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n"
2115+
+ "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all",
21172116
ImmutableSet.of(EventKind.WARNING));
21182117
assertContainsEvent(
21192118
"WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl"
@@ -2126,8 +2125,7 @@ public void extensionMetadata_isolated() throws Exception {
21262125
+ "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
21272126
+ " issues:\033[0m\n"
21282127
+ "\n"
2129-
+ "buildozer 'use_repo_add @ext//:defs.bzl ext:1 missing_direct_dep'"
2130-
+ " //MODULE.bazel:all",
2128+
+ "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all",
21312129
ImmutableSet.of(EventKind.WARNING));
21322130
}
21332131

@@ -2197,10 +2195,8 @@ public void extensionMetadata_isolatedDev() throws Exception {
21972195
+ "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
21982196
+ " issues:\033[0m\n"
21992197
+ "\n"
2200-
+ "buildozer 'use_repo_add dev @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'"
2201-
+ " //MODULE.bazel:all\n"
2202-
+ "buildozer 'use_repo_remove dev @ext//:defs.bzl ext:0 indirect_dep'"
2203-
+ " //MODULE.bazel:all",
2198+
+ "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n"
2199+
+ "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all",
22042200
ImmutableSet.of(EventKind.WARNING));
22052201
assertContainsEvent(
22062202
"WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl"
@@ -2213,8 +2209,7 @@ public void extensionMetadata_isolatedDev() throws Exception {
22132209
+ "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
22142210
+ " issues:\033[0m\n"
22152211
+ "\n"
2216-
+ "buildozer 'use_repo_add dev @ext//:defs.bzl ext:1 missing_direct_dep'"
2217-
+ " //MODULE.bazel:all",
2212+
+ "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all",
22182213
ImmutableSet.of(EventKind.WARNING));
22192214
}
22202215

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1048,7 +1048,7 @@ public void module_calledLate() throws Exception {
10481048
}
10491049

10501050
@Test
1051-
public void isolatedExtensionWithoutImports() throws Exception {
1051+
public void isolatedUsageWithoutImports() throws Exception {
10521052
scratch.file(
10531053
rootDirectory.getRelative("MODULE.bazel").getPathString(),
10541054
"isolated_ext = use_extension('//:extensions.bzl', 'my_ext', isolate = True)");
@@ -1066,4 +1066,19 @@ public void isolatedExtensionWithoutImports() throws Exception {
10661066
+ "Either import one or more repositories generated by the extension with "
10671067
+ "use_repo or remove the usage.");
10681068
}
1069+
1070+
@Test
1071+
public void isolatedUsageNotExported() throws Exception {
1072+
scratch.file(
1073+
rootDirectory.getRelative("MODULE.bazel").getPathString(),
1074+
"use_extension('//:extensions.bzl', 'my_ext', isolate = True)");
1075+
FakeRegistry registry = registryFactory.newFakeRegistry("/foo");
1076+
ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
1077+
1078+
reporter.removeHandler(failFastHandler); // expect failures
1079+
evaluator.evaluate(ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
1080+
assertContainsEvent(
1081+
"Isolated extension usage at /workspace/MODULE.bazel:1:14 must be assigned to a "
1082+
+ "top-level variable");
1083+
}
10691084
}

0 commit comments

Comments
 (0)