diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index f35ea95e08397d..6c1d900a94fc36 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -60,6 +60,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", "//third_party:auto_value", @@ -300,6 +301,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//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/skyframe:skyframe-objects", "//third_party:auto_value", "//third_party:guava", @@ -374,6 +376,7 @@ java_library( "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", @@ -391,6 +394,7 @@ java_library( ], deps = [ ":module_extension", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java index 6545dce9fb2586..bd2a41da6fc8e9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java @@ -18,7 +18,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; import com.google.devtools.build.lib.cmdline.Label; @@ -83,12 +82,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } ImmutableSet extensionsUsedByRootModule = - depGraphValue - .getExtensionUsagesTable() - .columnMap() - .getOrDefault(ModuleKey.ROOT, ImmutableMap.of()) - .keySet() - .stream() + depGraphValue.getExtensionUsagesTable().column(ModuleKey.ROOT).keySet().stream() // Use the eval-only key to avoid errors caused by incorrect imports - we can fix them. .map(SingleExtensionValue::evalKey) .collect(toImmutableSet()); @@ -108,8 +102,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) } return BazelModTidyValue.create( - fixups.build(), - buildozer.asPath(), - rootModuleFileValue.getIncludeLabelToCompiledModuleFile()); + fixups.build(), buildozer.asPath(), rootModuleFileValue.getModuleFilePaths()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java index d476c9481c6f00..b5dd58f6d1569a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java @@ -17,10 +17,11 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.List; @@ -37,13 +38,14 @@ public abstract class BazelModTidyValue implements SkyValue { /** The path of the buildozer binary provided by the "buildozer" module. */ public abstract Path buildozer(); - public abstract ImmutableMap includeLabelToCompiledModuleFile(); + /** The set of paths to the root MODULE.bazel file and all its includes. */ + public abstract ImmutableSet moduleFilePaths(); static BazelModTidyValue create( List fixups, Path buildozer, - ImmutableMap includeLabelToCompiledModuleFile) { + ImmutableSet moduleFilePaths) { return new AutoValue_BazelModTidyValue( - ImmutableList.copyOf(fixups), buildozer, includeLabelToCompiledModuleFile); + ImmutableList.copyOf(fixups), buildozer, moduleFilePaths); } } 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 454c1ec8a81e38..56fdadb6beb368 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 @@ -14,26 +14,26 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtensionUsage.Proxy; 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 com.google.devtools.build.lib.vfs.PathFragment; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; import java.util.Optional; import java.util.Set; -import java.util.stream.Stream; import javax.annotation.Nullable; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.eval.EvalException; @@ -171,20 +171,8 @@ static ModuleExtensionMetadata create( } public Optional generateFixup( - Collection usages, Set allRepos, EventHandler eventHandler) + ModuleExtensionUsage rootUsage, Set allRepos, EventHandler eventHandler) throws EvalException { - var rootUsages = - usages.stream() - .filter(usage -> usage.getUsingModule().equals(ModuleKey.ROOT)) - .collect(toImmutableList()); - if (rootUsages.isEmpty()) { - // The root module doesn't use the current extension. Do not suggest fixes as the user isn't - // expected to modify any other module's MODULE.bazel file. - return Optional.empty(); - } - // Every module only has at most a single usage of a given extension. - ModuleExtensionUsage rootUsage = Iterables.getOnlyElement(rootUsages); - var rootModuleDirectDevDeps = getRootModuleDirectDevDeps(allRepos); var rootModuleDirectDeps = getRootModuleDirectDeps(allRepos); if (rootModuleDirectDevDeps.isEmpty() && rootModuleDirectDeps.isEmpty()) { @@ -311,73 +299,52 @@ private static Optional generateFixup( message += "Fix the use_repo calls by running 'bazel mod tidy'."; - var buildozerCommands = - Stream.of( - makeUseRepoCommand( - "use_repo_add", - false, - importsToAdd, - extensionBzlFile, - extensionName, - rootUsage.getIsolationKey()), - makeUseRepoCommand( - "use_repo_remove", - false, - importsToRemove, - extensionBzlFile, - extensionName, - rootUsage.getIsolationKey()), - makeUseRepoCommand( - "use_repo_add", - true, - devImportsToAdd, - extensionBzlFile, - extensionName, - rootUsage.getIsolationKey()), - makeUseRepoCommand( - "use_repo_remove", - true, - devImportsToRemove, - extensionBzlFile, - extensionName, - rootUsage.getIsolationKey())) - .flatMap(Optional::stream) - .collect(toImmutableList()); + var moduleFilePathToCommandsBuilder = ImmutableListMultimap.builder(); + // Repos to add are easy: always add them to the first proxy of the correct type. + if (!importsToAdd.isEmpty()) { + Proxy firstNonDevProxy = + rootUsage.getProxies().stream().filter(p -> !p.isDevDependency()).findFirst().get(); + moduleFilePathToCommandsBuilder.put( + firstNonDevProxy.getContainingModuleFilePath(), + makeUseRepoCommand("use_repo_add", firstNonDevProxy.getProxyName(), importsToAdd)); + } + if (!devImportsToAdd.isEmpty()) { + Proxy firstDevProxy = + rootUsage.getProxies().stream().filter(p -> p.isDevDependency()).findFirst().get(); + moduleFilePathToCommandsBuilder.put( + firstDevProxy.getContainingModuleFilePath(), + makeUseRepoCommand("use_repo_add", firstDevProxy.getProxyName(), devImportsToAdd)); + } + // Repos to remove are a bit trickier: remove them from the proxy that actually imported them. + for (Proxy proxy : rootUsage.getProxies()) { + var toRemove = + ImmutableSortedSet.copyOf( + Sets.intersection( + proxy.getImports().values(), + proxy.isDevDependency() ? devImportsToRemove : importsToRemove)); + if (!toRemove.isEmpty()) { + moduleFilePathToCommandsBuilder.put( + proxy.getContainingModuleFilePath(), + makeUseRepoCommand("use_repo_remove", proxy.getProxyName(), toRemove)); + } + } eventHandler.handle(Event.warn(rootUsage.getProxies().getFirst().getLocation(), message)); - return Optional.of(new RootModuleFileFixup(buildozerCommands, rootUsage)); + return Optional.of(new RootModuleFileFixup(moduleFilePathToCommandsBuilder.build(), rootUsage)); } - private static Optional makeUseRepoCommand( - String cmd, - boolean devDependency, - Collection repos, - String extensionBzlFile, - String extensionName, - Optional isolationKey) { - if (repos.isEmpty()) { - return Optional.empty(); - } - + private static String makeUseRepoCommand(String cmd, String proxyName, Collection repos) { var commandParts = new ArrayList(); commandParts.add(cmd); - if (isolationKey.isPresent()) { - commandParts.add(isolationKey.get().getUsageExportedName()); - } else { - if (devDependency) { - commandParts.add("dev"); - } - commandParts.add(extensionBzlFile); - commandParts.add(extensionName); - } + commandParts.add(proxyName.isEmpty() ? "_unnamed_usage" : proxyName); commandParts.addAll(repos); - return Optional.of(String.join(" ", commandParts)); + return String.join(" ", commandParts); } private Optional> getRootModuleDirectDeps(Set allRepos) throws EvalException { - switch (getUseAllRepos()) { - case NO: + return switch (getUseAllRepos()) { + case NO -> { if (getExplicitRootModuleDirectDeps() != null) { Set invalidRepos = Sets.difference(getExplicitRootModuleDirectDeps(), allRepos); if (!invalidRepos.isEmpty()) { @@ -387,13 +354,11 @@ private Optional> getRootModuleDirectDeps(Set allRe String.join(", ", invalidRepos)); } } - return Optional.ofNullable(getExplicitRootModuleDirectDeps()); - case REGULAR: - return Optional.of(ImmutableSet.copyOf(allRepos)); - case DEV: - return Optional.of(ImmutableSet.of()); - } - throw new IllegalStateException("not reached"); + yield Optional.ofNullable(getExplicitRootModuleDirectDeps()); + } + case REGULAR -> Optional.of(ImmutableSet.copyOf(allRepos)); + case DEV -> Optional.of(ImmutableSet.of()); + }; } private Optional> getRootModuleDirectDevDeps(Set allRepos) 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 2dbfd1620e353b..ddc39abf6a4bdf 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 @@ -17,8 +17,9 @@ import static com.google.common.collect.ImmutableList.toImmutableList; 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; import java.util.Optional; @@ -63,6 +64,12 @@ public abstract static class Proxy { */ public abstract String getProxyName(); + /** + * The path to the MODULE.bazel file (or one of its includes) that contains this proxy object. + * This path should be relative to the workspace root. + */ + public abstract PathFragment getContainingModuleFilePath(); + /** Whether {@code dev_dependency} is set to true. */ public abstract boolean isDevDependency(); @@ -71,7 +78,7 @@ public abstract static class Proxy { * current module. The key is the local repo name (in the scope of the current module), and the * value is the name exported by the module extension. */ - public abstract ImmutableMap getImports(); + public abstract ImmutableBiMap getImports(); public static Builder builder() { return new AutoValue_ModuleExtensionUsage_Proxy.Builder().setProxyName(""); @@ -86,11 +93,13 @@ public abstract static class Builder { public abstract Builder setProxyName(String value); + public abstract Builder setContainingModuleFilePath(PathFragment value); + public abstract boolean isDevDependency(); public abstract Builder setDevDependency(boolean value); - abstract ImmutableMap.Builder importsBuilder(); + abstract ImmutableBiMap.Builder importsBuilder(); @CanIgnoreReturnValue public final Builder addImport(String key, String value) { @@ -98,7 +107,7 @@ public final Builder addImport(String key, String value) { return this; } - public abstract Builder setImports(ImmutableMap value); + public abstract Builder setImports(ImmutableBiMap value); public abstract Proxy build(); } 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 60f09174bd7d2f..fd042048ff459a 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 @@ -70,6 +70,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.stream.Stream; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Mutability; @@ -476,12 +477,18 @@ public static RootModuleFileValue evaluateRootModuleFile( name -> ModuleKey.create(name, Version.EMPTY).getCanonicalRepoNameWithoutVersion(), name -> name)); + ImmutableSet moduleFilePaths = + Stream.concat( + Stream.of(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME), + includeLabelToCompiledModuleFile.keySet().stream() + .map(label -> Label.parseCanonicalUnchecked(label).toPathFragment())) + .collect(toImmutableSet()); return RootModuleFileValue.create( module, moduleFileHash, overrides, nonRegistryOverrideCanonicalRepoNameLookup, - includeLabelToCompiledModuleFile); + moduleFilePaths); } private static ModuleThreadContext execModuleFile( 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 525dead381f0c7..f02f213d81ef5b 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 @@ -430,7 +430,8 @@ public ModuleExtensionProxy useExtension( var proxyBuilder = ModuleExtensionUsage.Proxy.builder() .setLocation(thread.getCallerLocation()) - .setDevDependency(devDependency); + .setDevDependency(devDependency) + .setContainingModuleFilePath(context.getCurrentModuleFilePath()); String extensionBzlFile = normalizeLabelString(context.getModuleBuilder(), rawExtensionBzlFile); var newUsageBuilder = @@ -692,7 +693,9 @@ public void call( usageBuilder, ModuleExtensionUsage.Proxy.builder() .setDevDependency(devDependency) - .setLocation(thread.getCallerLocation())); + .setLocation(thread.getCallerLocation()) + .setContainingModuleFilePath( + usageBuilder.getContext().getCurrentModuleFilePath())); extensionProxy.getValue(tagName).call(kwargs, thread); extensionProxy.addImport(name, name, "by a repo rule", thread.getCallerLocation()); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java index c6a8d08bea9ad1..3b94cd81c506e7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java @@ -18,10 +18,12 @@ import com.google.auto.value.AutoValue; import com.google.auto.value.extension.memoized.Memoized; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -82,12 +84,10 @@ public abstract static class RootModuleFileValue extends ModuleFileValue { getNonRegistryOverrideCanonicalRepoNameLookup(); /** - * TODO: This field is a hack. It's not needed by anything other than {@code ModCommand}, during - * the {@code bazel mod tidy} command. Doing it this way assumes that {@code bazel mod tidy} - * cannot touch any included segments. This is unsatisfactory; we should do it properly at some - * point, although that seems quite difficult. + * The set of relative paths to the root MODULE.bazel file itself and all its transitive + * includes. */ - public abstract ImmutableMap getIncludeLabelToCompiledModuleFile(); + public abstract ImmutableSet getModuleFilePaths(); @Override public ImmutableMap> getRegistryFileHashes() { @@ -100,13 +100,13 @@ public static RootModuleFileValue create( String moduleFileHash, ImmutableMap overrides, ImmutableMap nonRegistryOverrideCanonicalRepoNameLookup, - ImmutableMap includeLabelToCompiledModuleFile) { + ImmutableSet moduleFilePaths) { return new AutoValue_ModuleFileValue_RootModuleFileValue( module, moduleFileHash, overrides, nonRegistryOverrideCanonicalRepoNameLookup, - includeLabelToCompiledModuleFile); + moduleFilePaths); } } 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 19ba4f7caa153f..c01358de8aa9dc 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 @@ -22,7 +22,10 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; 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; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; @@ -39,6 +42,8 @@ public class ModuleThreadContext { private boolean moduleCalled = false; private boolean hadNonModuleCall = false; + private PathFragment currentModuleFilePath = LabelConstants.MODULE_DOT_BAZEL_FILE_NAME; + private final boolean ignoreDevDeps; private final InterimModule.Builder module; private final ImmutableMap builtinModules; @@ -216,7 +221,14 @@ public void include(String includeLabel, StarlarkThread thread) // compiled before evaluation started. throw Starlark.errorf("internal error; included file %s not compiled", includeLabel); } + PathFragment includer = currentModuleFilePath; + currentModuleFilePath = Label.parseCanonicalUnchecked(includeLabel).toPathFragment(); compiledModuleFile.runOnThread(thread); + currentModuleFilePath = includer; + } + + public PathFragment getCurrentModuleFilePath() { + return currentModuleFilePath; } public void addOverride(String moduleName, ModuleOverride override) throws EvalException { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java index 4182219aa2a77b..bef0e3b4c4f6f5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java @@ -15,18 +15,21 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.devtools.build.lib.vfs.PathFragment; /** - * Generated when incorrect use_repo calls are detected in the root module file according to {@link - * ModuleExtensionMetadata} and contains the buildozer commands required to bring the root module - * file into the expected state. + * Generated when incorrect use_repo calls are detected in the root module file and its includes + * according to {@link ModuleExtensionMetadata}, and contains the buildozer commands required to + * bring them into the expected state. * - * @param buildozerCommands The buildozer commands required to bring the root module file into the - * expected state. + * @param moduleFilePathToBuildozerCommands the keys are the paths to the root MODULE.bazel file (or + * its includes); the values are the buildozer commands required to bring the keyed file into + * the expected state. */ public record RootModuleFileFixup( - ImmutableList buildozerCommands, ModuleExtensionUsage usage) { + ImmutableListMultimap moduleFilePathToBuildozerCommands, + ModuleExtensionUsage usage) { /** A human-readable message describing the fixup after it has been applied. */ public String getSuccessMessage() { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 9d18f1c62344ca..34834715c7fa28 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -441,7 +441,8 @@ private SingleExtensionValue createSingleExtensionValue( Environment env) throws SingleExtensionEvalFunctionException { Optional fixup = Optional.empty(); - if (moduleExtensionMetadata.isPresent()) { + if (moduleExtensionMetadata.isPresent() + && usagesValue.getExtensionUsages().containsKey(ModuleKey.ROOT)) { try { // TODO: ModuleExtensionMetadata#generateFixup should throw ExternalDepsException instead of // EvalException. @@ -449,7 +450,7 @@ private SingleExtensionValue createSingleExtensionValue( moduleExtensionMetadata .get() .generateFixup( - usagesValue.getExtensionUsages().values(), + usagesValue.getExtensionUsages().get(ModuleKey.ROOT), generatedRepoSpecs.keySet(), env.getListener()); } catch (EvalException e) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java index f82f882854e43b..dc8a13ed71d538 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.bazel.commands; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.bazel.bzlmod.modcommand.ModOptions.Charset.UTF8; @@ -24,10 +25,12 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; +import com.google.common.io.CharSource; import com.google.devtools.build.lib.analysis.NoBuildEvent; import com.google.devtools.build.lib.analysis.NoBuildRequestFinishedEvent; import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue; @@ -71,6 +74,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.InterruptedFailureDetails; import com.google.devtools.build.lib.util.MaybeCompleteSet; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.EvaluationContext; import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyKey; @@ -83,14 +87,12 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.io.Writer; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Optional; import java.util.stream.IntStream; -import java.util.stream.Stream; import javax.annotation.Nullable; /** Queries the Bzlmod external dependency graph. */ @@ -554,21 +556,29 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe } private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue modTidyValue) { - CommandBuilder buildozerCommand = - new CommandBuilder() - .setWorkingDir(env.getWorkspace()) - .addArg(modTidyValue.buildozer().getPathString()) - .addArgs( - Stream.concat( - modTidyValue.fixups().stream() - .map(RootModuleFileFixup::buildozerCommands) - .flatMap(Collection::stream), - Stream.of("format")) - .collect(toImmutableList())) - .addArg("MODULE.bazel:all"); - try { - buildozerCommand.build().execute(); - } catch (InterruptedException | CommandException e) { + ImmutableListMultimap allCommandsPerFile = + modTidyValue.fixups().stream() + .flatMap(fixup -> fixup.moduleFilePathToBuildozerCommands().entries().stream()) + .collect(toImmutableListMultimap(Entry::getKey, Entry::getValue)); + StringBuilder buildozerInput = new StringBuilder(); + for (PathFragment moduleFilePath : modTidyValue.moduleFilePaths()) { + buildozerInput.append("//").append(moduleFilePath).append(":all|"); + for (String command : allCommandsPerFile.get(moduleFilePath)) { + buildozerInput.append(command).append('|'); + } + buildozerInput.append("format\n"); + } + + try (var stdin = CharSource.wrap(buildozerInput).asByteSource(UTF_8).openStream()) { + new CommandBuilder() + .setWorkingDir(env.getWorkspace()) + .addArg(modTidyValue.buildozer().getPathString()) + .addArg("-f") + .addArg("-") + .build() + .executeAsync(stdin, /* killSubprocessOnInterrupt= */ true) + .get(); + } catch (InterruptedException | CommandException | IOException e) { String suffix = ""; if (e instanceof AbnormalTerminationException) { if (((AbnormalTerminationException) e).getResult().getTerminationStatus().getRawExitCode() 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 f8852f22cfc8d6..b1ff500c1c4906 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 @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -224,6 +225,7 @@ private static ModuleExtensionUsage createModuleExtensionUsage( .setDevDependency(false) .setLocation(Location.BUILTIN) .setImports(importsBuilder.buildOrThrow()) + .setContainingModuleFilePath(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .setUsingModule(ModuleKey.ROOT) .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 00c98c14b42309..e722cae92b417e 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 @@ -2107,13 +2107,147 @@ public void extensionMetadata() throws Exception { ModuleExtensionId.create( Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); assertThat(evalValue.getFixup()).isPresent(); - assertThat(evalValue.getFixup().get().buildozerCommands()) + assertThat(evalValue.getFixup().get().moduleFilePathToBuildozerCommands()) .containsExactly( - "use_repo_add @ext//:defs.bzl ext missing_direct_dep non_dev_as_dev_dep", - "use_repo_remove @ext//:defs.bzl ext dev_as_non_dev_dep indirect_dep invalid_dep", - "use_repo_add dev @ext//:defs.bzl ext dev_as_non_dev_dep missing_direct_dev_dep", - "use_repo_remove dev @ext//:defs.bzl ext indirect_dev_dep invalid_dev_dep" - + " non_dev_as_dev_dep"); + PathFragment.create("MODULE.bazel"), + "use_repo_add ext missing_direct_dep non_dev_as_dev_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext dev_as_non_dev_dep indirect_dep invalid_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_add ext_dev dev_as_non_dev_dep missing_direct_dev_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext_dev indirect_dev_dep invalid_dev_dep non_dev_as_dev_dep"); + assertThat(evalValue.getFixup().get().getSuccessMessage()) + .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); + } + + @Test + public void extensionMetadata_includes() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "include('//:firstProd.MODULE.bazel')", + "include('//:second.MODULE.bazel')"); + scratch.file( + workspaceRoot.getRelative("firstProd.MODULE.bazel").getPathString(), + "ext = use_extension('@ext//:defs.bzl', 'ext')", + "use_repo(", + " ext,", + " 'indirect_dep',", + " 'invalid_dep',", + " 'dev_as_non_dev_dep',", + " my_direct_dep = 'direct_dep',", + ")", + "include('//:firstDev.MODULE.bazel')"); + scratch.file( + workspaceRoot.getRelative("firstDev.MODULE.bazel").getPathString(), + "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(", + " ext_dev,", + " 'indirect_dev_dep',", + " 'invalid_dev_dep',", + " 'non_dev_as_dev_dep',", + " my_direct_dev_dep = 'direct_dev_dep',", + ")"); + scratch.file( + workspaceRoot.getRelative("second.MODULE.bazel").getPathString(), + "ext = use_extension('@ext//:defs.bzl', 'ext')", + "use_repo(ext, 'invalid_dep2')", + "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'invalid_dev_dep2')"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@my_direct_dep//:data.bzl', direct_dep_data='data')", + "data = direct_dep_data"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')", + "ext_dev = use_extension('//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'indirect_dev_dep')"); + scratch.file(modulesRoot.getRelative("ext~v1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~v1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~v1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_repo(name='direct_dep')", + " data_repo(name='direct_dev_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='missing_direct_dev_dep')", + " data_repo(name='indirect_dep')", + " data_repo(name='indirect_dev_dep')", + " data_repo(name='dev_as_non_dev_dep')", + " data_repo(name='non_dev_as_dev_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps=['direct_dep', 'missing_direct_dep', 'non_dev_as_dev_dep'],", + " root_module_direct_dev_deps=['direct_dev_dep', 'missing_direct_dev_dep'," + + " 'dev_as_non_dev_dep'],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + // Evaluation fails due to the import of a repository not generated by the extension, but we + // only want to assert that the warning is emitted. + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertEventCount(1, eventCollector); + assertContainsEvent( + """ + WARNING /ws/firstProd.MODULE.bazel:1:20: The module extension ext defined in \ + @ext//:defs.bzl reported incorrect imports of repositories via use_repo(): + + Imported, but not created by the extension (will cause the build to fail): + invalid_dep, invalid_dep2, invalid_dev_dep, invalid_dev_dep2 + + Not imported, but reported as direct dependencies by the extension (may cause the\ + build to fail): + missing_direct_dep, missing_direct_dev_dep + + Imported as a regular dependency, but reported as a dev dependency by the\ + extension (may cause the build to fail when used by other modules): + dev_as_non_dev_dep + + Imported as a dev dependency, but reported as a regular dependency by the\ + extension (may cause the build to fail when used by other modules): + non_dev_as_dev_dep + + Imported, but reported as indirect dependencies by the extension: + indirect_dep, indirect_dev_dep + + Fix the use_repo calls by running 'bazel mod tidy'.""", + ImmutableSet.of(EventKind.WARNING)); + SingleExtensionValue evalValue = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); + assertThat(evalValue.getFixup()).isPresent(); + assertThat(evalValue.getFixup().get().moduleFilePathToBuildozerCommands()) + .containsExactly( + PathFragment.create("firstProd.MODULE.bazel"), + "use_repo_add ext missing_direct_dep non_dev_as_dev_dep", + PathFragment.create("firstProd.MODULE.bazel"), + "use_repo_remove ext dev_as_non_dev_dep indirect_dep invalid_dep", + PathFragment.create("second.MODULE.bazel"), + "use_repo_remove ext invalid_dep2", + PathFragment.create("firstDev.MODULE.bazel"), + "use_repo_add ext_dev dev_as_non_dev_dep missing_direct_dev_dep", + PathFragment.create("firstDev.MODULE.bazel"), + "use_repo_remove ext_dev indirect_dev_dep invalid_dev_dep non_dev_as_dev_dep", + PathFragment.create("second.MODULE.bazel"), + "use_repo_remove ext_dev invalid_dev_dep2"); assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2199,13 +2333,15 @@ public void extensionMetadata_all() throws Exception { ModuleExtensionId.create( Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); assertThat(evalValue.getFixup()).isPresent(); - assertThat(evalValue.getFixup().get().buildozerCommands()) + assertThat(evalValue.getFixup().get().moduleFilePathToBuildozerCommands()) .containsExactly( - "use_repo_add @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep missing_direct_dep" + PathFragment.create("MODULE.bazel"), + "use_repo_add ext direct_dev_dep indirect_dev_dep missing_direct_dep" + " missing_direct_dev_dep", - "use_repo_remove @ext//:defs.bzl ext invalid_dep", - "use_repo_remove dev @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep" - + " invalid_dev_dep"); + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext invalid_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext_dev direct_dev_dep indirect_dev_dep invalid_dev_dep"); assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2293,12 +2429,15 @@ public void extensionMetadata_allDev() throws Exception { ModuleExtensionId.create( Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); assertThat(evalValue.getFixup()).isPresent(); - assertThat(evalValue.getFixup().get().buildozerCommands()) + assertThat(evalValue.getFixup().get().moduleFilePathToBuildozerCommands()) .containsExactly( - "use_repo_remove @ext//:defs.bzl ext direct_dep indirect_dep invalid_dep", - "use_repo_add dev @ext//:defs.bzl ext direct_dep indirect_dep missing_direct_dep" + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext direct_dep indirect_dep invalid_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_add ext_dev direct_dep indirect_dep missing_direct_dep" + " missing_direct_dev_dep", - "use_repo_remove dev @ext//:defs.bzl ext invalid_dev_dep"); + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext_dev invalid_dev_dep"); assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2445,9 +2584,12 @@ public void extensionMetadata_isolated() throws Exception { Optional.of( ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext1"))))); assertThat(ext1Value.getFixup()).isPresent(); - assertThat(ext1Value.getFixup().get().buildozerCommands()) + assertThat(ext1Value.getFixup().get().moduleFilePathToBuildozerCommands()) .containsExactly( - "use_repo_add ext1 direct_dep missing_direct_dep", "use_repo_remove ext1 indirect_dep"); + PathFragment.create("MODULE.bazel"), + "use_repo_add ext1 direct_dep missing_direct_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext1 indirect_dep"); assertThat(ext1Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext1' of @ext//:defs.bzl%ext"); SingleExtensionValue ext2Value = @@ -2462,8 +2604,9 @@ public void extensionMetadata_isolated() throws Exception { Optional.of( ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext2"))))); assertThat(ext2Value.getFixup()).isPresent(); - assertThat(ext2Value.getFixup().get().buildozerCommands()) - .containsExactly("use_repo_add ext2 missing_direct_dep"); + assertThat(ext2Value.getFixup().get().moduleFilePathToBuildozerCommands()) + .containsExactly( + PathFragment.create("MODULE.bazel"), "use_repo_add ext2 missing_direct_dep"); assertThat(ext2Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext2' of @ext//:defs.bzl%ext"); } @@ -2555,9 +2698,12 @@ public void extensionMetadata_isolatedDev() throws Exception { Optional.of( ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext1"))))); assertThat(ext1Value.getFixup()).isPresent(); - assertThat(ext1Value.getFixup().get().buildozerCommands()) + assertThat(ext1Value.getFixup().get().moduleFilePathToBuildozerCommands()) .containsExactly( - "use_repo_add ext1 direct_dep missing_direct_dep", "use_repo_remove ext1 indirect_dep"); + PathFragment.create("MODULE.bazel"), + "use_repo_add ext1 direct_dep missing_direct_dep", + PathFragment.create("MODULE.bazel"), + "use_repo_remove ext1 indirect_dep"); assertThat(ext1Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext1' of @ext//:defs.bzl%ext"); SingleExtensionValue ext2Value = @@ -2572,8 +2718,9 @@ public void extensionMetadata_isolatedDev() throws Exception { Optional.of( ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext2"))))); assertThat(ext2Value.getFixup()).isPresent(); - assertThat(ext2Value.getFixup().get().buildozerCommands()) - .containsExactly("use_repo_add ext2 missing_direct_dep"); + assertThat(ext2Value.getFixup().get().moduleFilePathToBuildozerCommands()) + .containsExactly( + PathFragment.create("MODULE.bazel"), "use_repo_add ext2 missing_direct_dep"); assertThat(ext2Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext2' of @ext//:defs.bzl%ext"); } 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 dcab466f8b2908..68d44e0c408ea7 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 @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule; import com.google.devtools.build.lib.clock.BlazeClock; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -765,6 +766,8 @@ public void testModuleExtensions_good() throws Exception { .setDevDependency(false) .setProxyName("myext1") .setImports(ImmutableBiMap.of("repo1", "repo1")) + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() @@ -795,6 +798,8 @@ public void testModuleExtensions_good() throws Exception { .setProxyName("myext2") .setImports( ImmutableBiMap.of("other_repo1", "repo1", "repo2", "repo2")) + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() @@ -839,6 +844,8 @@ public void testModuleExtensions_good() throws Exception { .setImports( ImmutableBiMap.of( "mvn", "maven", "junit", "junit", "guava", "guava")) + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() @@ -912,6 +919,8 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { .setDevDependency(true) .setProxyName("myext1") .addImport("alpha", "alpha") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -920,6 +929,8 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { .setDevDependency(false) .setProxyName("myext2") .addImport("beta", "beta") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -928,6 +939,8 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { .setDevDependency(true) .setProxyName("myext3") .addImport("gamma", "gamma") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -936,6 +949,8 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { .setDevDependency(false) .setProxyName("myext4") .addImport("delta", "delta") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() @@ -1037,6 +1052,8 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { .setDevDependency(false) .setProxyName("myext2") .addImport("beta", "beta") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -1046,6 +1063,8 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { .setDevDependency(false) .setProxyName("myext4") .addImport("delta", "delta") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() @@ -1151,6 +1170,8 @@ public void testModuleExtensions_innate() throws Exception { Location.fromFileLineColumn("/workspace/MODULE.bazel", 2, 5)) .setDevDependency(false) .addImport("repo_name", "repo_name") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -1158,6 +1179,8 @@ public void testModuleExtensions_innate() throws Exception { Location.fromFileLineColumn("/workspace/MODULE.bazel", 4, 13)) .setDevDependency(false) .addImport("guava", "guava") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addProxy( ModuleExtensionUsage.Proxy.builder() @@ -1165,6 +1188,8 @@ public void testModuleExtensions_innate() throws Exception { Location.fromFileLineColumn("/workspace/MODULE.bazel", 5, 13)) .setDevDependency(true) .addImport("vuaga", "vuaga") + .setContainingModuleFilePath( + LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag( Tag.builder() 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 c8d9518387dcb2..5d16c09873dd0b 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 @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.modcommand.OutputFormatters.OutputFormatter; import com.google.devtools.build.lib.bazel.bzlmod.modcommand.OutputFormatters.OutputFormatter.Explanation; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.util.MaybeCompleteSet; @@ -611,6 +612,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { .setLocation(Location.fromFileLineColumn("C@1.0/MODULE.bazel", 2, 23)) .setImports(ImmutableBiMap.of("repo1", "repo1", "repo3", "repo3")) .setDevDependency(false) + .setContainingModuleFilePath(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .setUsingModule(createModuleKey("C", "1.0")) .build()) @@ -625,6 +627,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { .setLocation(Location.fromFileLineColumn("D@1.0/MODULE.bazel", 1, 10)) .setImports(ImmutableBiMap.of("repo1", "repo1", "repo2", "repo2")) .setDevDependency(false) + .setContainingModuleFilePath(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .setUsingModule(createModuleKey("D", "1.0")) .build()) @@ -639,6 +642,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { .setLocation(Location.fromFileLineColumn("Y@2.0/MODULE.bazel", 2, 13)) .setImports(ImmutableBiMap.of("repo2", "repo2")) .setDevDependency(false) + .setContainingModuleFilePath(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .setUsingModule(createModuleKey("Y", "2.0")) .build()) @@ -653,6 +657,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception { .setLocation(Location.fromFileLineColumn("Y@2.0/MODULE.bazel", 13, 10)) .setImports(ImmutableBiMap.of("myrepo", "repo5")) .setDevDependency(false) + .setContainingModuleFilePath(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME) .build()) .addTag(buildTag("dep").addAttr("coord", "junit").build()) .addTag(buildTag("dep").addAttr("coord", "guava").build()) diff --git a/src/test/py/bazel/bzlmod/mod_command_test.py b/src/test/py/bazel/bzlmod/mod_command_test.py index eac9852c4a378d..1437a93c63232b 100644 --- a/src/test/py/bazel/bzlmod/mod_command_test.py +++ b/src/test/py/bazel/bzlmod/mod_command_test.py @@ -986,6 +986,135 @@ def testModTidyFixesInvalidImport(self): module_file.read().split('\n'), ) + def testModTidyWithIncludes(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'include("//:firstProd.MODULE.bazel")', + 'include("//:second.MODULE.bazel")', + ], + ) + self.ScratchFile( + 'firstProd.MODULE.bazel', + [ + 'ext = use_extension("//:extension.bzl", "ext")', + 'use_repo(ext, "dep2", "bad_dep")', + 'include("//:firstDev.MODULE.bazel")', + ], + ) + self.ScratchFile( + 'firstDev.MODULE.bazel', + [ + ( + 'ext_dev = use_extension("//:extension.bzl", "ext",' + ' dev_dependency = True)' + ), + 'use_repo(ext_dev, "dev2", "bad_dev")', + ], + ) + self.ScratchFile( + 'second.MODULE.bazel', + [ + 'ext = use_extension("//:extension.bzl", "ext")', + 'use_repo(ext, "blad_dep")', + ( + 'ext_dev = use_extension("//:extension.bzl", "ext",' + ' dev_dependency = True)' + ), + 'use_repo(ext_dev, "blad_dev")', + 'ext2 = use_extension("//:extension.bzl", "ext2")', + 'use_repo(ext2, "blaad_dep")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("WORKSPACE")', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' repo_rule(name="dep")', + ' repo_rule(name="dep2")', + ' repo_rule(name="dev")', + ' repo_rule(name="dev2")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=["dep", "dep2"],', + ' root_module_direct_dev_deps=["dev", "dev2"],', + ' )', + '', + 'ext = module_extension(implementation=_ext_impl)', + '', + 'def _ext2_impl(ctx):', + ' repo_rule(name="ext2_dep")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=["ext2_dep"],', + ' root_module_direct_dev_deps=[],', + ' )', + '', + 'ext2 = module_extension(implementation=_ext2_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['mod', 'tidy']) + stderr = '\n'.join(stderr) + self.assertIn( + 'INFO: Updated use_repo calls for @//:extension.bzl%ext', stderr + ) + + with open('MODULE.bazel', 'r') as module_file: + self.assertEqual( + [ + 'include("//:firstProd.MODULE.bazel")', + '', # formatted despite no extension usages! + 'include("//:second.MODULE.bazel")', + '', + ], + module_file.read().split('\n'), + ) + with open('firstProd.MODULE.bazel', 'r') as module_file: + self.assertEqual( + [ + 'ext = use_extension("//:extension.bzl", "ext")', + 'use_repo(ext, "dep", "dep2")', + '', + 'include("//:firstDev.MODULE.bazel")', + '', + ], + module_file.read().split('\n'), + ) + with open('firstDev.MODULE.bazel', 'r') as module_file: + self.assertEqual( + [ + ( + 'ext_dev = use_extension("//:extension.bzl", "ext",' + ' dev_dependency = True)' + ), + 'use_repo(ext_dev, "dev", "dev2")', + '', + ], + module_file.read().split('\n'), + ) + with open('second.MODULE.bazel', 'r') as module_file: + self.assertEqual( + [ + 'ext = use_extension("//:extension.bzl", "ext")', + '', + ( + 'ext_dev = use_extension("//:extension.bzl", "ext",' + ' dev_dependency = True)' + ), + '', + 'ext2 = use_extension("//:extension.bzl", "ext2")', + 'use_repo(ext2, "ext2_dep")', + '', + ], + module_file.read().split('\n'), + ) + if __name__ == '__main__': absltest.main()