From 4c1a50bae68a98a9a279ca26b83a74ad3391d601 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Wed, 24 Jul 2024 17:58:00 -0400 Subject: [PATCH] Add flag --incompatible_use_plus_in_repo_names Defaults to false. When set to true, we use `+` instead of `~` as the separator in canonical repo names. Some more subtle changes include: - We now officially say that the "build" part of version strings (the part that begins with a plus) is ignored and stripped. - When the flag is set to true, we effectively increase the lockfile version by 1 (see code comment in BazelLockFileModule). - When true, we no longer insert a `_main` in front of names of repos generated by module extensions hosted in the main repo. (`~abc` as a name was problematic, but `+abc` is not.) - When true, we no longer insert a `v` in front of numerical versions in canonical repo names. (`my_mod~1.1` could be a Windows short path, but `my_mod+1.1` cannot.) Work towards https://github.com/bazelbuild/bazel/issues/22865. --- .../devtools/build/lib/bazel/bzlmod/BUILD | 2 + .../bazel/bzlmod/BazelDepGraphFunction.java | 41 +++++++++++++------ .../lib/bazel/bzlmod/BazelDepGraphValue.java | 14 +++++-- .../lib/bazel/bzlmod/BazelLockFileModule.java | 11 +++++ .../lib/bazel/bzlmod/ModuleExtensionId.java | 1 + .../lib/bazel/bzlmod/ModuleFileFunction.java | 13 ++++-- .../build/lib/bazel/bzlmod/ModuleKey.java | 28 +++++++++---- .../bzlmod/SingleExtensionEvalFunction.java | 32 +++++++++++++-- .../lib/bazel/bzlmod/StarlarkBazelModule.java | 2 +- .../build/lib/bazel/bzlmod/Version.java | 29 ++++++++----- .../build/lib/cmdline/RepositoryName.java | 5 ++- .../semantics/BuildLanguageOptions.java | 14 +++++++ .../lib/skyframe/BzlmodRepoRuleFunction.java | 7 +++- .../skyframe/RepositoryMappingFunction.java | 6 ++- .../lib/skyframe/RepositoryMappingValue.java | 2 +- .../lib/bazel/bzlmod/InterimModuleTest.java | 2 +- .../packages/semantics/ConsistencyTest.java | 2 + 17 files changed, 162 insertions(+), 49 deletions(-) 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 d1047420ae45a4..8a49eb1dce0927 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 @@ -26,6 +26,7 @@ java_library( ], deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/net/starlark/java/eval", "//third_party:auto_value", "//third_party:gson", @@ -251,6 +252,7 @@ java_library( "//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/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value", 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 5770b09e1d3d74..7faf5a17e796fb 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 @@ -33,7 +33,9 @@ import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.LabelConverter; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code; +import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; @@ -42,6 +44,7 @@ import com.google.devtools.build.skyframe.SkyValue; import java.util.Map.Entry; import javax.annotation.Nullable; +import net.starlark.java.eval.StarlarkSemantics; /** * This function runs Bazel module resolution, extracts the dependency graph from it and creates a @@ -58,13 +61,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws BazelDepGraphFunctionException, InterruptedException { BazelModuleResolutionValue selectionResult = (BazelModuleResolutionValue) env.getValue(BazelModuleResolutionValue.KEY); + StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); if (env.valuesMissing()) { return null; } var depGraph = selectionResult.getResolvedDepGraph(); ImmutableBiMap canonicalRepoNameLookup = - computeCanonicalRepoNameLookup(depGraph); + computeCanonicalRepoNameLookup(depGraph, starlarkSemantics); ImmutableTable extensionUsagesById; try { extensionUsagesById = getExtensionUsagesById(depGraph, canonicalRepoNameLookup.inverse()); @@ -73,14 +77,17 @@ public SkyValue compute(SkyKey skyKey, Environment env) } ImmutableBiMap extensionUniqueNames = - calculateUniqueNameForUsedExtensionId(extensionUsagesById); + calculateUniqueNameForUsedExtensionId(extensionUsagesById, starlarkSemantics); return BazelDepGraphValue.create( depGraph, canonicalRepoNameLookup, depGraph.values().stream().map(AbridgedModule::from).collect(toImmutableList()), extensionUsagesById, - extensionUniqueNames.inverse()); + extensionUniqueNames.inverse(), + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES) + ? '+' + : '~'); } private static ImmutableTable @@ -126,7 +133,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } private static ImmutableBiMap computeCanonicalRepoNameLookup( - ImmutableMap depGraph) { + ImmutableMap depGraph, StarlarkSemantics semantics) { // Find modules with multiple versions in the dep graph. Currently, the only source of such // modules is multiple_version_override. ImmutableSet multipleVersionsModules = @@ -151,13 +158,14 @@ private static ImmutableBiMap computeCanonicalRepoNam toImmutableBiMap( key -> multipleVersionsModules.contains(key.getName()) - ? key.getCanonicalRepoNameWithVersion() - : key.getCanonicalRepoNameWithoutVersion(), + ? key.getCanonicalRepoNameWithVersion(semantics) + : key.getCanonicalRepoNameWithoutVersion(semantics), key -> key)); } private ImmutableBiMap calculateUniqueNameForUsedExtensionId( - ImmutableTable extensionUsagesById) { + ImmutableTable extensionUsagesById, + StarlarkSemantics starlarkSemantics) { // Calculate a unique name for each used extension id with the following property that is // required for BzlmodRepoRuleFunction to unambiguously identify the extension that generates a // given repo: @@ -166,18 +174,23 @@ private ImmutableBiMap calculateUniqueNameForUsedExte BiMap extensionUniqueNames = HashBiMap.create(); for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) { int attempt = 1; - while (extensionUniqueNames.putIfAbsent(makeUniqueNameCandidate(id, attempt), id) != null) { + while (extensionUniqueNames.putIfAbsent( + makeUniqueNameCandidate(id, attempt, starlarkSemantics), id) + != null) { attempt++; } } return ImmutableBiMap.copyOf(extensionUniqueNames); } - private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) { + private static String makeUniqueNameCandidate( + ModuleExtensionId id, int attempt, StarlarkSemantics starlarkSemantics) { + boolean usePlus = + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES); // Ensure that the resulting extension name (and thus the repository names derived from it) do // not start with a tilde. RepositoryName repository = id.getBzlFileLabel().getRepository(); - String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName(); + String nonEmptyRepoPart = repository.isMain() && !usePlus ? "_main" : repository.getName(); // 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 "_". @@ -191,14 +204,18 @@ private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) .map( namespace -> String.format( - "%s~_%s%s~%s~%s~%s", + usePlus ? "%s+_%s%s+%s+%s+%s" : "%s~_%s%s~%s~%s~%s", nonEmptyRepoPart, id.getExtensionName(), extensionNameDisambiguator, namespace.getModule().getName(), namespace.getModule().getVersion(), namespace.getUsageExportedName())) - .orElse(nonEmptyRepoPart + "~" + id.getExtensionName() + extensionNameDisambiguator); + .orElse( + nonEmptyRepoPart + + (usePlus ? "+" : "~") + + id.getExtensionName() + + extensionNameDisambiguator); } static class BazelDepGraphFunctionException extends SkyFunctionException { 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 6fb4c98bf8432c..d1e7b4d326650b 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 @@ -43,13 +43,15 @@ public static BazelDepGraphValue create( ImmutableMap canonicalRepoNameLookup, ImmutableList abridgedModules, ImmutableTable extensionUsagesTable, - ImmutableMap extensionUniqueNames) { + ImmutableMap extensionUniqueNames, + char repoNameSeparator) { return new AutoValue_BazelDepGraphValue( depGraph, ImmutableBiMap.copyOf(canonicalRepoNameLookup), abridgedModules, extensionUsagesTable, - extensionUniqueNames); + extensionUniqueNames, + repoNameSeparator); } public static BazelDepGraphValue createEmptyDepGraph() { @@ -80,7 +82,8 @@ public static BazelDepGraphValue createEmptyDepGraph() { canonicalRepoNameLookup, ImmutableList.of(), ImmutableTable.of(), - ImmutableMap.of()); + ImmutableMap.of(), + '~'); } /** @@ -112,6 +115,9 @@ public static BazelDepGraphValue createEmptyDepGraph() { */ public abstract ImmutableMap getExtensionUniqueNames(); + /** The character to use to separate the different segments of a canonical repo name. */ + public abstract char getRepoNameSeparator(); + /** * Returns the full {@link RepositoryMapping} for the given module, including repos from Bazel * module deps and module extensions. @@ -122,7 +128,7 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) { getExtensionUsagesTable().column(key).entrySet()) { ModuleExtensionId extensionId = extIdAndUsage.getKey(); ModuleExtensionUsage usage = extIdAndUsage.getValue(); - String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + "~"; + String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + getRepoNameSeparator(); for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) { for (Map.Entry entry : proxy.getImports().entrySet()) { String canonicalRepoName = repoNamePrefix + entry.getValue(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java index fb5399e1df3698..595287722fcfad 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java @@ -117,6 +117,17 @@ public void afterCommand() { // lockfile that are still up-to-date and adding the newly resolved extension results. BazelLockFileValue newLockfile = BazelLockFileValue.builder() + // HACK: Flipping the flag `--incompatible_use_plus_in_repo_names` causes the canonical + // repo name format to change, which warrants a lockfile invalidation. We could do this + // properly by introducing another field in the lockfile, but that's very annoying since + // we'll need to 1) keep it around for a while; 2) be careful not to parse the rest of + // the lockfile to avoid triggering parsing errors ('~' will be an invalid character in + // repo names eventually). So we just increment the lockfile version if '+' is being + // used. + .setLockFileVersion( + depGraphValue.getRepoNameSeparator() == '+' + ? BazelLockFileValue.LOCK_FILE_VERSION + 1 + : BazelLockFileValue.LOCK_FILE_VERSION) .setRegistryFileHashes( ImmutableSortedMap.copyOf(moduleResolutionValue.getRegistryFileHashes())) .setSelectedYankedVersions(moduleResolutionValue.getSelectedYankedVersions()) 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 3fb6eeb7ec15ac..e994142a205ad3 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 @@ -60,6 +60,7 @@ public static IsolationKey create(ModuleKey module, String usageExportedName) { @Override public final String toString() { + // NOTE: Can't be bothered to switch this based on the flag. return getModule() + "~" + getUsageExportedName(); } 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 cde0a9ab40b800..0a9af353eaaf41 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 @@ -157,7 +157,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) GetModuleFileResult getModuleFileResult; try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) { - getModuleFileResult = getModuleFile(moduleKey, moduleFileKey.getOverride(), env); + getModuleFileResult = + getModuleFile(moduleKey, moduleFileKey.getOverride(), starlarkSemantics, env); } if (getModuleFileResult == null) { return null; @@ -481,7 +482,8 @@ public static RootModuleFileValue evaluateRootModuleFile( // A module with a non-registry override always has a unique version across the // entire dep graph. name -> - ModuleKey.create(name, Version.EMPTY).getCanonicalRepoNameWithoutVersion(), + ModuleKey.create(name, Version.EMPTY) + .getCanonicalRepoNameWithoutVersion(starlarkSemantics), name -> name)); ImmutableSet moduleFilePaths = Stream.concat( @@ -552,14 +554,17 @@ private record GetModuleFileResult( @Nullable private GetModuleFileResult getModuleFile( - ModuleKey key, @Nullable ModuleOverride override, Environment env) + ModuleKey key, + @Nullable ModuleOverride override, + StarlarkSemantics starlarkSemantics, + Environment env) throws ModuleFileFunctionException, InterruptedException { // If there is a non-registry override for this module, we need to fetch the corresponding repo // first and read the module file from there. if (override instanceof NonRegistryOverride) { // A module with a non-registry override always has a unique version across the entire dep // graph. - RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion(); + RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion(starlarkSemantics); RepositoryDirectoryValue repoDir = (RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(canonicalRepoName)); if (repoDir == null) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java index 6fb5e0a378c8b2..3a70de89beb7e6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java @@ -20,8 +20,10 @@ import com.google.common.base.Splitter; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import java.util.Comparator; import java.util.List; +import net.starlark.java.eval.StarlarkSemantics; /** A module name, version pair that identifies a module in the external dependency graph. */ @AutoValue @@ -79,8 +81,12 @@ public final String toString() { * *

This method must not be called if the module has a {@link NonRegistryOverride}. */ + public RepositoryName getCanonicalRepoNameWithVersion(StarlarkSemantics semantics) { + return getCanonicalRepoName(/* includeVersion= */ true, semantics); + } + public RepositoryName getCanonicalRepoNameWithVersion() { - return getCanonicalRepoName(/* includeVersion= */ true); + return getCanonicalRepoNameWithVersion(StarlarkSemantics.DEFAULT); } /** @@ -88,26 +94,31 @@ public RepositoryName getCanonicalRepoNameWithVersion() { * only guaranteed to be unique when there is a single version of the module in the entire dep * graph. */ + public RepositoryName getCanonicalRepoNameWithoutVersion(StarlarkSemantics semantics) { + return getCanonicalRepoName(/* includeVersion= */ false, semantics); + } + public RepositoryName getCanonicalRepoNameWithoutVersion() { - return getCanonicalRepoName(/* includeVersion= */ false); + return getCanonicalRepoNameWithoutVersion(StarlarkSemantics.DEFAULT); } - private RepositoryName getCanonicalRepoName(boolean includeVersion) { + private RepositoryName getCanonicalRepoName(boolean includeVersion, StarlarkSemantics semantics) { if (WELL_KNOWN_MODULES.containsKey(getName())) { return WELL_KNOWN_MODULES.get(getName()); } if (ROOT.equals(this)) { return RepositoryName.MAIN; } + boolean usePlus = semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES); String suffix; if (includeVersion) { // getVersion().isEmpty() is true only for modules with non-registry overrides, which enforce // that there is a single version of the module in the dep graph. Preconditions.checkState(!getVersion().isEmpty()); - // Prepend "v" to prevent canonical repo names, which form segments of file paths, from - // looking like a Windows short path. Such paths segments would incur additional file IO - // during analysis (see WindowsShortPath). - suffix = "v" + getVersion().toString(); + // When using `~` as the separator, prepend "v" to prevent canonical repo names, which form + // segments of file paths, from looking like a Windows short path. Such paths segments would + // incur additional file IO during analysis (see WindowsShortPath). + suffix = usePlus ? getVersion().toString() : "v" + getVersion().toString(); } else { // This results in canonical repository names such as `rules_foo~` for the module `rules_foo`. // This particular format is chosen since: @@ -125,7 +136,8 @@ private RepositoryName getCanonicalRepoName(boolean includeVersion) { // rarely used. suffix = ""; } - return RepositoryName.createUnvalidated(String.format("%s~%s", getName(), suffix)); + return RepositoryName.createUnvalidated( + String.format("%s%s%s", getName(), usePlus ? "+" : "~", suffix)); } public static ModuleKey fromString(String s) throws Version.ParseException { 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 629aede176fcee..5043b75c45a00a 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 @@ -44,6 +44,7 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; @@ -176,6 +177,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) extension, usagesValue, extension.getEvalFactors(), + starlarkSemantics, lockedExtension); if (singleExtensionValue != null) { return singleExtensionValue; @@ -259,7 +261,13 @@ public SkyValue compute(SkyKey skyKey, Environment env) lockFileInfo = Optional.empty(); } return createSingleExtensionValue( - generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, lockFileInfo, env); + generatedRepoSpecs, + moduleExtensionMetadata, + extensionId, + usagesValue, + lockFileInfo, + starlarkSemantics, + env); } /** @@ -276,6 +284,7 @@ private SingleExtensionValue tryGettingValueFromLockFile( RunnableExtension extension, SingleExtensionUsagesValue usagesValue, ModuleExtensionEvalFactors evalFactors, + StarlarkSemantics starlarkSemantics, LockFileModuleExtension lockedExtension) throws SingleExtensionEvalFunctionException, InterruptedException, @@ -336,6 +345,7 @@ private SingleExtensionValue tryGettingValueFromLockFile( extensionId, usagesValue, Optional.of(new LockFileModuleExtension.WithFactors(evalFactors, lockedExtension)), + starlarkSemantics, env); } if (lockfileMode.equals(LockfileMode.ERROR)) { @@ -450,6 +460,7 @@ private SingleExtensionValue createSingleExtensionValue( ModuleExtensionId extensionId, SingleExtensionUsagesValue usagesValue, Optional lockFileInfo, + StarlarkSemantics starlarkSemantics, Environment env) throws SingleExtensionEvalFunctionException { Optional fixup = Optional.empty(); @@ -477,6 +488,11 @@ private SingleExtensionValue createSingleExtensionValue( } } + char separator = + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES) + ? '+' + : '~'; + return SingleExtensionValue.create( generatedRepoSpecs, generatedRepoSpecs.keySet().stream() @@ -484,7 +500,7 @@ private SingleExtensionValue createSingleExtensionValue( toImmutableBiMap( e -> RepositoryName.createUnvalidated( - usagesValue.getExtensionUniqueName() + "~" + e), + usagesValue.getExtensionUniqueName() + separator + e), Function.identity())), lockFileInfo, fixup); @@ -742,7 +758,11 @@ public RunModuleExtensionResult run( Dict kwargs = repo.tag().getAttributeValues().attributes(); // This cast should be safe since it should have been verified at tag creation time. String name = (String) kwargs.get("name"); - String prefixedName = usagesValue.getExtensionUniqueName() + "~" + name; + char separator = + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES) + ? '+' + : '~'; + String prefixedName = usagesValue.getExtensionUniqueName() + separator + name; Rule ruleInstance; AttributeValues attributesValue; try { @@ -881,9 +901,13 @@ public RunModuleExtensionResult run( ModuleExtensionId extensionId, RepositoryMapping mainRepositoryMapping) throws InterruptedException, SingleExtensionEvalFunctionException { + char separator = + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES) + ? '+' + : '~'; ModuleExtensionEvalStarlarkThreadContext threadContext = new ModuleExtensionEvalStarlarkThreadContext( - usagesValue.getExtensionUniqueName() + "~", + usagesValue.getExtensionUniqueName() + separator, extensionId.getBzlFileLabel().getPackageIdentifier(), BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(), mainRepositoryMapping, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java index 90b28b87eda5b0..f15f7d4e22f8c2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java @@ -139,7 +139,7 @@ public static StarlarkBazelModule create( } return new StarlarkBazelModule( module.getName(), - module.getVersion().getOriginal(), + module.getVersion().getNormalized(), new Tags(Maps.transformValues(typeCheckedTags, StarlarkList::immutableCopyOf)), module.getKey().equals(ModuleKey.ROOT)); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Version.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Version.java index 41072e01064114..42d44025679e40 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Version.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Version.java @@ -36,9 +36,9 @@ * each a sequence of "identifiers" (defined as a non-empty sequence of ASCII alphanumerical * characters and hyphens) separated by dots. The {@code RELEASE} part may not contain hyphens. * - *

Otherwise, this format is identical to SemVer, especially in terms of the comparison algorithm - * (https://semver.org/#spec-item-11). In other words, this format is intentionally looser than - * SemVer; in particular: + *

Otherwise, this format is identical to SemVer, especially in terms of the comparison algorithm. In other words, this format is + * intentionally looser than SemVer; in particular: * *

    *
  • the "release" part isn't limited to exactly 3 segments (major, minor, patch), but can be @@ -51,6 +51,13 @@ * {@code a} and {@code b} compare {@code a < b} iff the same holds when they're compared as Bazel * module versions. * + *

    Versions with a "build" part are generally accepted as input, but they're treated as if the + * "build" part is completely absent. That is, when Bazel outputs version strings, it never outputs + * the "build" part (in fact, it doesn't even store it); similarly, when Bazel accesses registries + * to request versions, the "build" part is never included. This gives us the nice property of + * "consistent with equals" natural ordering (see {@link Comparable}); that is, {@code + * a.compareTo(b) == 0} iff {@code a.equals(b)}. + * *

    The special "empty string" version can also be used, and compares higher than everything else. * It signifies that there is a {@link NonRegistryOverride} for a module. */ @@ -112,15 +119,15 @@ public final int compareTo(Identifier o) { /** Returns the "prerelease" part of the version string as a list of {@link Identifier}s. */ abstract ImmutableList getPrerelease(); - /** Returns the original version string. */ - public abstract String getOriginal(); + /** Returns the normalized version string (that is, with any "build" part stripped). */ + public abstract String getNormalized(); /** * Whether this is just the "empty string" version, which signifies a non-registry override for * the module. */ boolean isEmpty() { - return getOriginal().isEmpty(); + return getNormalized().isEmpty(); } /** @@ -164,7 +171,8 @@ public static Version parse(String version) throws ParseException { } } - return new AutoValue_Version(releaseSplit.build(), prereleaseSplit.build(), version); + String normalized = Strings.isNullOrEmpty(prerelease) ? release : release + '-' + prerelease; + return new AutoValue_Version(releaseSplit.build(), prereleaseSplit.build(), normalized); } private static final Comparator COMPARATOR = @@ -180,17 +188,18 @@ public int compareTo(Version o) { @Override public final String toString() { - return getOriginal(); + return getNormalized(); } @Override public final boolean equals(Object o) { - return this == o || (o instanceof Version && ((Version) o).getOriginal().equals(getOriginal())); + return this == o + || (o instanceof Version && ((Version) o).getNormalized().equals(getNormalized())); } @Override public final int hashCode() { - return Objects.hash("version", getOriginal().hashCode()); + return Objects.hash("version", getNormalized().hashCode()); } /** An exception encountered while trying to {@link Version#parse parse} a version. */ diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java index 588833aa670ad2..622e14179ba1e8 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java @@ -51,7 +51,7 @@ public final class RepositoryName { // Repository names must not start with a tilde as shells treat unescaped paths starting with them // specially. // https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html - private static final Pattern VALID_REPO_NAME = Pattern.compile("|[\\w\\-.][\\w\\-.~]*"); + private static final Pattern VALID_REPO_NAME = Pattern.compile("|[\\w\\-.+][\\w\\-.~+]*"); // Must start with a letter. Can contain ASCII letters and digits, underscore, dash, and dot. private static final Pattern VALID_USER_PROVIDED_NAME = Pattern.compile("[a-zA-Z][-.\\w]*$"); @@ -228,11 +228,12 @@ public String getOwnerModuleDisplayString() { if (ownerRepoIfNotVisible.isMain()) { return "root module"; } else { + boolean hasTilde = ownerRepoIfNotVisible.getName().contains("~"); return String.format( "module '%s'", ownerRepoIfNotVisible .getName() - .substring(0, ownerRepoIfNotVisible.getName().indexOf('~'))); + .substring(0, ownerRepoIfNotVisible.getName().indexOf(hasTilde ? '~' : '+'))); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index b2042a6bd95cd8..aeca3326c75164 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -216,6 +216,17 @@ public final class BuildLanguageOptions extends OptionsBase { + " https://bazel.build/external/overview for more information.") public boolean enableWorkspace; + @Option( + name = "incompatible_use_plus_in_repo_names", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = OptionEffectTag.LOADING_AND_ANALYSIS, + help = + "If true, uses the plus sign (+) as the separator in canonical repo names, instead of the" + + " tilde (~). This is to address severe performance issues on Windows; see" + + " https://github.com/bazelbuild/bazel/issues/22865 for more information.") + public boolean incompatibleUsePlusInRepoNames; + @Option( name = "experimental_isolated_extension_usages", defaultValue = "false", @@ -748,6 +759,7 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(EXPERIMENTAL_ENABLE_SCL_DIALECT, experimentalEnableSclDialect) .setBool(ENABLE_BZLMOD, enableBzlmod) .setBool(ENABLE_WORKSPACE, enableWorkspace) + .setBool(INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES, incompatibleUsePlusInRepoNames) .setBool(EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, experimentalIsolatedExtensionUsages) .setBool( INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW, incompatibleExistingRulesImmutableView) @@ -856,6 +868,8 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String EXPERIMENTAL_ENABLE_SCL_DIALECT = "-experimental_enable_scl_dialect"; public static final String ENABLE_BZLMOD = "+enable_bzlmod"; public static final String ENABLE_WORKSPACE = "+enable_workspace"; + public static final String INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES = + "-incompatible_use_plus_in_repo_names"; public static final String EXPERIMENTAL_ISOLATED_EXTENSION_USAGES = "-experimental_isolated_extension_usages"; public static final String INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index ed90d731f944e6..fb91b59c6ef852 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException; import com.google.devtools.build.lib.packages.RuleFunction; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.skyframe.SkyFunction; @@ -113,9 +114,13 @@ public SkyValue compute(SkyKey skyKey, Environment env) } // Step 3: look for the repo from module extension evaluation results. + char separator = + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES) + ? '+' + : '~'; Optional extensionId = bazelDepGraphValue.getExtensionUniqueNames().entrySet().stream() - .filter(e -> repositoryName.getName().startsWith(e.getValue() + "~")) + .filter(e -> repositoryName.getName().startsWith(e.getValue() + separator)) .map(Entry::getKey) .findFirst(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java index d01fd4d78ea16d..2cfd3319b7ee34 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java @@ -233,7 +233,11 @@ private RepositoryMappingValue computeFromWorkspace( private static Optional maybeGetModuleExtensionForRepo( RepositoryName repositoryName, BazelDepGraphValue bazelDepGraphValue) { return bazelDepGraphValue.getExtensionUniqueNames().entrySet().stream() - .filter(e -> repositoryName.getName().startsWith(e.getValue() + "~")) + .filter( + e -> + repositoryName + .getName() + .startsWith(e.getValue() + bazelDepGraphValue.getRepoNameSeparator())) .map(Entry::getKey) .findFirst(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java index 8e38bba78feef0..39b319ffb8c594 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java @@ -73,7 +73,7 @@ public static RepositoryMappingValue createForBzlmodRepo( return new AutoValue_RepositoryMappingValue( repositoryMapping, Optional.of(associatedModuleName), - Optional.of(associatedModuleVersion.getOriginal())); + Optional.of(associatedModuleVersion.getNormalized())); } /** diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModuleTest.java index dd65628c9bac18..d903da386e3334 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModuleTest.java @@ -39,7 +39,7 @@ public void withDepSpecsTransformed() throws Exception { DepSpec.fromModuleKey( createModuleKey( depSpec.getName() + "_new", - depSpec.getVersion().getOriginal() + ".1")))) + depSpec.getVersion().getNormalized() + ".1")))) .isEqualTo( InterimModuleBuilder.create("", "") .addDep("dep_foo", createModuleKey("foo_new", "1.0.1")) diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index c5f68b2053fac6..5662c7954831d9 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -126,6 +126,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--experimental_single_package_toolchain_binding=" + rand.nextBoolean(), "--enable_bzlmod=" + rand.nextBoolean(), "--enable_workspace=" + rand.nextBoolean(), + "--incompatible_use_plus_in_repo_names=" + rand.nextBoolean(), "--experimental_isolated_extension_usages=" + rand.nextBoolean(), "--experimental_google_legacy_api=" + rand.nextBoolean(), "--experimental_platforms_api=" + rand.nextBoolean(), @@ -172,6 +173,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING, rand.nextBoolean()) .setBool(BuildLanguageOptions.ENABLE_BZLMOD, rand.nextBoolean()) .setBool(BuildLanguageOptions.ENABLE_WORKSPACE, rand.nextBoolean()) + .setBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_GOOGLE_LEGACY_API, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_PLATFORMS_API, rand.nextBoolean())