[7.3.0] Add flag --incompatible_use_plus_in_repo_names (#23141)

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 #22865.

Closes #23103.

PiperOrigin-RevId: 657202616
Change-Id: I015b2a04a823b1d951015a1b2e1b99b154dcc5a2

Commit
2f67e57

Co-authored-by: Xdng Yng <wyverald@gmail.com>

MODULE.bazel

@@ -22,7 +22,7 @@ bazel_dep(name = "stardoc", version = "0.5.6", repo_name = "io_bazel_skydoc")
 bazel_dep(name = "zstd-jni", version = "1.5.2-3.bcr.1")
 bazel_dep(name = "blake3", version = "1.5.1.bcr.1")
 bazel_dep(name = "sqlite3", version = "3.42.0.bcr.1")
-bazel_dep(name = "zlib", version = "1.3")
+bazel_dep(name = "zlib", version = "1.3.1.bcr.3")
 bazel_dep(name = "rules_cc", version = "0.0.9")
 bazel_dep(name = "rules_java", version = "7.6.5")
 bazel_dep(name = "rules_graalvm", version = "0.11.1")

src/MODULE.tools

@@ -13,7 +13,7 @@ bazel_dep(name = "rules_python", version = "0.22.1")
 bazel_dep(name = "buildozer", version = "7.1.2")
 bazel_dep(name = "platforms", version = "0.0.9")
 bazel_dep(name = "protobuf", version = "3.19.6", repo_name = "com_google_protobuf")
-bazel_dep(name = "zlib", version = "1.3")
+bazel_dep(name = "zlib", version = "1.3.1.bcr.3")
 
 cc_configure = use_extension("//tools/cpp:cc_configure.bzl", "cc_configure_extension")
 use_repo(cc_configure, "local_config_cc", "local_config_cc_toolchains")

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",
@@ -250,6 +251,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",

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<RepositoryName, ModuleKey> canonicalRepoNameLookup =
-        computeCanonicalRepoNameLookup(depGraph);
+        computeCanonicalRepoNameLookup(depGraph, starlarkSemantics);
     ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById;
     try {
       extensionUsagesById = getExtensionUsagesById(depGraph, canonicalRepoNameLookup.inverse());
@@ -73,14 +77,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     }
 
     ImmutableBiMap<String, ModuleExtensionId> 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<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
@@ -126,7 +133,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
   }
 
   private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNameLookup(
-      ImmutableMap<ModuleKey, Module> depGraph) {
+      ImmutableMap<ModuleKey, Module> depGraph, StarlarkSemantics semantics) {
     // Find modules with multiple versions in the dep graph. Currently, the only source of such
     // modules is multiple_version_override.
     ImmutableSet<String> multipleVersionsModules =
@@ -151,13 +158,14 @@ private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNam
             toImmutableBiMap(
                 key ->
                     multipleVersionsModules.contains(key.getName())
-                        ? key.getCanonicalRepoNameWithVersion()
-                        : key.getCanonicalRepoNameWithoutVersion(),
+                        ? key.getCanonicalRepoNameWithVersion(semantics)
+                        : key.getCanonicalRepoNameWithoutVersion(semantics),
                 key -> key));
   }
 
   private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExtensionId(
-      ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
+      ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> 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<String, ModuleExtensionId> calculateUniqueNameForUsedExte
     BiMap<String, ModuleExtensionId> 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 {

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

@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.common.collect.ImmutableMap.toImmutableMap;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableBiMap;
@@ -43,13 +42,15 @@ public static BazelDepGraphValue create(
       ImmutableMap<RepositoryName, ModuleKey> canonicalRepoNameLookup,
       ImmutableList<AbridgedModule> abridgedModules,
       ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesTable,
-      ImmutableMap<ModuleExtensionId, String> extensionUniqueNames) {
+      ImmutableMap<ModuleExtensionId, String> extensionUniqueNames,
+      char repoNameSeparator) {
     return new AutoValue_BazelDepGraphValue(
         depGraph,
         ImmutableBiMap.copyOf(canonicalRepoNameLookup),
         abridgedModules,
         extensionUsagesTable,
-        extensionUniqueNames);
+        extensionUniqueNames,
+        repoNameSeparator);
   }
 
   public static BazelDepGraphValue createEmptyDepGraph() {
@@ -65,22 +66,16 @@ public static BazelDepGraphValue createEmptyDepGraph() {
             .build();
 
     ImmutableMap<ModuleKey, Module> emptyDepGraph = ImmutableMap.of(ModuleKey.ROOT, root);
-
     ImmutableMap<RepositoryName, ModuleKey> canonicalRepoNameLookup =
-        emptyDepGraph.keySet().stream()
-            .collect(
-                toImmutableMap(
-                    // All modules in the empty dep graph (just the root module) have an empty
-                    // version, so the choice of including it in the canonical repo name does not
-                    // matter.
-                    ModuleKey::getCanonicalRepoNameWithoutVersion, key -> key));
+        ImmutableMap.of(RepositoryName.MAIN, ModuleKey.ROOT);
 
     return BazelDepGraphValue.create(
         emptyDepGraph,
         canonicalRepoNameLookup,
         ImmutableList.of(),
         ImmutableTable.of(),
-        ImmutableMap.of());
+        ImmutableMap.of(),
+        '+');
   }
 
   /**
@@ -112,6 +107,9 @@ public static BazelDepGraphValue createEmptyDepGraph() {
    */
   public abstract ImmutableMap<ModuleExtensionId, String> 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 +120,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<String, String> entry : proxy.getImports().entrySet()) {
           String canonicalRepoName = repoNamePrefix + entry.getValue();

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

@@ -117,6 +117,18 @@ 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. This does mean that, while this hack exists, normal increments of the lockfile
+            // version need to be done by 2 at a time (i.e. keep LOCK_FILE_VERSION an odd number).
+            .setLockFileVersion(
+                depGraphValue.getRepoNameSeparator() == '+'
+                    ? BazelLockFileValue.LOCK_FILE_VERSION + 1
+                    : BazelLockFileValue.LOCK_FILE_VERSION)
             .setRegistryFileHashes(
                 ImmutableSortedMap.copyOf(moduleResolutionValue.getRegistryFileHashes()))
             .setSelectedYankedVersions(moduleResolutionValue.getSelectedYankedVersions())

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

@@ -35,6 +35,8 @@
 @GenerateTypeAdapter
 public abstract class BazelLockFileValue implements SkyValue, Postable {
 
+  // NOTE: See "HACK" note in BazelLockFileModule. While this hack exists, normal increments of the
+  // lockfile version need to be done by 2 at a time (i.e. keep LOCK_FILE_VERSION an odd number).
   public static final int LOCK_FILE_VERSION = 11;
 
   @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE;

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

@@ -60,6 +60,8 @@ 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. But DO change this to "+" by
+      // Bazel 8!
       return getModule() + "~" + getUsageExportedName();
     }
 

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

@@ -156,7 +156,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;
@@ -475,7 +476,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<PathFragment> moduleFilePaths =
         Stream.concat(
@@ -544,14 +546,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) {