Use '~' instead of '.' as the canonical repo name separator

So far we've been using "module_name.version[.extension_name.internal_repo_name]" as the canonical repo name format. This is problematic since versions can already contain dots, which means that "A.1.0-pre.foo.bar.baz" can be interpreted as "baz generated by extension bar in [email protected]" or "bar.baz generated by extension foo in [email protected]". If we use '~' as the separator, this problem goes away.

PiperOrigin-RevId: 460471660
Change-Id: I4dd4d302c86c0e4dbe0731c75d07cb53bf35deb6

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

@@ -163,7 +163,7 @@ static BazelModuleResolutionValue createValue(
     BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
     for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
       String bestName =
-          id.getBzlFileLabel().getRepository().getName() + "." + id.getExtensionName();
+          id.getBzlFileLabel().getRepository().getName() + "~" + id.getExtensionName();
       if (!bestName.startsWith("@")) {
         // We have to special-case extensions hosted in well-known modules, because *all* repos
         // generated by Bzlmod have to have an '@'-prefixed name, except well-known modules.

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

@@ -107,7 +107,7 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) {
       ModuleExtensionUsage usage = e.getValue();
       for (Map.Entry<String, String> entry : usage.getImports().entrySet()) {
         String canonicalRepoName =
-            getExtensionUniqueNames().get(extensionId) + "." + entry.getValue();
+            getExtensionUniqueNames().get(extensionId) + "~" + entry.getValue();
         mapping.put(entry.getKey(), RepositoryName.createUnvalidated(canonicalRepoName));
       }
     }

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

@@ -77,7 +77,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       ImmutableMap<String, Package> generatedRepos =
           ((SingleExtensionEvalValue) value).getGeneratedRepos();
       String repoPrefix =
-          bazelModuleResolutionValue.getExtensionUniqueNames().get(extensionId) + '.';
+          bazelModuleResolutionValue.getExtensionUniqueNames().get(extensionId) + '~';
       for (Map.Entry<String, Package> entry : generatedRepos.entrySet()) {
         RepositoryName canonicalRepoName =
             RepositoryName.createUnvalidated(repoPrefix + entry.getKey());

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

@@ -76,6 +76,6 @@ public RepositoryName getCanonicalRepoName() {
       return RepositoryName.MAIN;
     }
     return RepositoryName.createUnvalidated(
-        String.format("@%s.%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
+        String.format("@%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
   }
 }

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

@@ -31,7 +31,6 @@
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.SkyFunction;
-import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -158,7 +157,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     ModuleExtension extension = ((ModuleExtension.InStarlark) exported).get();
     ModuleExtensionEvalStarlarkThreadContext threadContext =
         new ModuleExtensionEvalStarlarkThreadContext(
-            usagesValue.getExtensionUniqueName() + ".",
+            usagesValue.getExtensionUniqueName() + "~",
             extensionId.getBzlFileLabel().getPackageIdentifier(),
             BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(),
             directories,
@@ -170,8 +169,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       ModuleExtensionContext moduleContext =
           createContext(env, usagesValue, starlarkSemantics, extension);
       threadContext.storeInThread(thread);
-      // TODO(wyv): support native.register_toolchains
-      // TODO(wyv): make sure the Label constructor works
       try {
         Starlark.fastcall(
             thread, extension.getImplementation(), new Object[] {moduleContext}, new Object[0]);

src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java

@@ -37,7 +37,7 @@ public final class RepositoryName {
 
   @SerializationConstant public static final RepositoryName MAIN = new RepositoryName("");
 
-  private static final Pattern VALID_REPO_NAME = Pattern.compile("@?[\\w\\-.#]*");
+  private static final Pattern VALID_REPO_NAME = Pattern.compile("@?[\\w\\-.~]*");
 
   private static final LoadingCache<String, RepositoryName> repositoryNameCache =
       Caffeine.newBuilder()
@@ -145,7 +145,7 @@ static void validate(String name) throws LabelSyntaxException {
     if (!VALID_REPO_NAME.matcher(name).matches()) {
       throw LabelParser.syntaxErrorf(
           "invalid repository name '@%s': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.'"
-              + " and '#'",
+              + " and '~'",
           StringUtilities.sanitizeControlChars(name));
     }
   }

src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java

@@ -182,7 +182,7 @@ private Optional<RepositoryMapping> computeForModuleExtensionRepo(
                     internalName -> internalName,
                     internalName ->
                         RepositoryName.createUnvalidated(
-                            extensionUniqueName + "." + internalName)));
+                            extensionUniqueName + "~" + internalName)));
     // Find the key of the module containing this extension. This will be used to compute additional
     // mappings -- any repo generated by an extension contained in the module "foo" can additionally
     // see all repos that "foo" can see.
@@ -213,7 +213,7 @@ private SkyValue computeFromWorkspace(
           RepositoryMapping.createAllowingFallback(
               externalPackage.getRepositoryMapping(repositoryName)));
     }
-    // If bzlmod is in play, we need to transform mappings to "foo" into mappings for "foo.1.3" (if
+    // If bzlmod is in play, we need to transform mappings to "foo" into mappings for "foo~1.3" (if
     // there is a module called "foo" in the dep graph and its version is 1.3, that is).
     ImmutableMap<String, ModuleKey> moduleNameLookup =
         bazelModuleResolutionValue.getModuleNameLookup();
@@ -228,7 +228,7 @@ private SkyValue computeFromWorkspace(
               ModuleKey moduleKey = moduleNameLookup.get(toRepo.getName());
               return moduleKey == null ? toRepo : moduleKey.getCanonicalRepoName();
             }));
-    // If there's no existing mapping to "foo", we should add a mapping from "foo" to "foo.1.3"
+    // If there's no existing mapping to "foo", we should add a mapping from "foo" to "foo~1.3"
     // anyways.
     for (Map.Entry<String, ModuleKey> entry : moduleNameLookup.entrySet()) {
       mapping.putIfAbsent(entry.getKey(), entry.getValue().getCanonicalRepoName());

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

@@ -93,13 +93,13 @@ public void createValue_basic() throws Exception {
         .containsExactly(
             RepositoryName.MAIN,
             ModuleKey.ROOT,
-            RepositoryName.create("@dep.1.0"),
+            RepositoryName.create("@dep~1.0"),
             createModuleKey("dep", "1.0"),
-            RepositoryName.create("@dep.2.0"),
+            RepositoryName.create("@dep~2.0"),
             createModuleKey("dep", "2.0"),
-            RepositoryName.create("@rules_cc.1.0"),
+            RepositoryName.create("@rules_cc~1.0"),
             createModuleKey("rules_cc", "1.0"),
-            RepositoryName.create("@rules_java.override"),
+            RepositoryName.create("@rules_java~override"),
             createModuleKey("rules_java", ""));
     assertThat(value.getModuleNameLookup())
         .containsExactly(
@@ -156,14 +156,14 @@ public void createValue_moduleExtensions() throws Exception {
 
     ModuleExtensionId maven =
         ModuleExtensionId.create(
-            Label.parseCanonical("@@rules_jvm_external.1.0//:defs.bzl"), "maven");
+            Label.parseCanonical("@@rules_jvm_external~1.0//:defs.bzl"), "maven");
     ModuleExtensionId pip =
-        ModuleExtensionId.create(Label.parseCanonical("@@rules_python.2.0//:defs.bzl"), "pip");
+        ModuleExtensionId.create(Label.parseCanonical("@@rules_python~2.0//:defs.bzl"), "pip");
     ModuleExtensionId myext =
-        ModuleExtensionId.create(Label.parseCanonical("@@dep.2.0//:defs.bzl"), "myext");
+        ModuleExtensionId.create(Label.parseCanonical("@@dep~2.0//:defs.bzl"), "myext");
     ModuleExtensionId myext2 =
         ModuleExtensionId.create(
-            Label.parseCanonical("@@dep.2.0//incredible:conflict.bzl"), "myext");
+            Label.parseCanonical("@@dep~2.0//incredible:conflict.bzl"), "myext");
 
     BazelModuleResolutionValue value =
         BazelModuleResolutionFunction.createValue(depGraph, depGraph, ImmutableMap.of());
@@ -181,10 +181,10 @@ public void createValue_moduleExtensions() throws Exception {
 
     assertThat(value.getExtensionUniqueNames())
         .containsExactly(
-            maven, "@rules_jvm_external.1.0.maven",
-            pip, "@rules_python.2.0.pip",
-            myext, "@dep.2.0.myext",
-            myext2, "@dep.2.0.myext2");
+            maven, "@rules_jvm_external~1.0~maven",
+            pip, "@rules_python~2.0~pip",
+            myext, "@dep~2.0~myext",
+            myext2, "@dep~2.0~myext2");
 
     assertThat(value.getFullRepoMapping(ModuleKey.ROOT))
         .isEqualTo(
@@ -195,26 +195,26 @@ public void createValue_moduleExtensions() throws Exception {
                 "root",
                 "",
                 "rje",
-                "@rules_jvm_external.1.0",
+                "@rules_jvm_external~1.0",
                 "rpy",
-                "@rules_python.2.0",
+                "@rules_python~2.0",
                 "av",
-                "@rules_jvm_external.1.0.maven.autovalue",
+                "@rules_jvm_external~1.0~maven~autovalue",
                 "numpy",
-                "@rules_python.2.0.pip.numpy"));
+                "@rules_python~2.0~pip~numpy"));
     assertThat(value.getFullRepoMapping(depKey))
         .isEqualTo(
             createRepositoryMapping(
                 depKey,
                 "dep",
-                "@dep.2.0",
+                "@dep~2.0",
                 "rules_python",
-                "@rules_python.2.0",
+                "@rules_python~2.0",
                 "np",
-                "@rules_python.2.0.pip.numpy",
+                "@rules_python~2.0~pip~numpy",
                 "oneext",
-                "@dep.2.0.myext.myext",
+                "@dep~2.0~myext~myext",
                 "twoext",
-                "@dep.2.0.myext2.myext"));
+                "@dep~2.0~myext2~myext"));
   }
 }

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

@@ -147,18 +147,18 @@ public void getRepoSpec_bazelModule() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C.2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C~2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C~2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
-                    ImmutableMap.of("name", "@C.2.0", "path", "/usr/local/modules/@C.2.0"))
+                    ImmutableMap.of("name", "@C~2.0", "path", "/usr/local/modules/@C~2.0"))
                 .build());
   }
 
@@ -180,19 +180,19 @@ public void getRepoSpec_nonRegistryOverride() throws Exception {
 
     EvaluationResult<GetRepoSpecByNameValue> result =
         evaluator.evaluate(
-            ImmutableList.of(getRepoSpecByNameKey("@C.override")), evaluationContext);
+            ImmutableList.of(getRepoSpecByNameKey("@C~override")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.override")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C~override")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
                     ImmutableMap.of(
-                        "name", "@C.override",
+                        "name", "@C~override",
                         "path", "/foo/bar/C"))
                 .build());
   }
@@ -216,12 +216,12 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C.3.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C~3.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.3.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C~3.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
@@ -232,9 +232,9 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
                 .setAttributes(
                     ImmutableMap.of(
                         "name",
-                        "@C.3.0",
+                        "@C~3.0",
                         "path",
-                        "/usr/local/modules/@C.3.0",
+                        "/usr/local/modules/@C~3.0",
                         "patches",
                         ImmutableList.of("//:foo.patch"),
                         "patch_args",
@@ -264,18 +264,18 @@ public void getRepoSpec_multipleVersionOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@D.2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@D~2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@D.2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@D~2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
-                    ImmutableMap.of("name", "@D.2.0", "path", "/usr/local/modules/@D.2.0"))
+                    ImmutableMap.of("name", "@D~2.0", "path", "/usr/local/modules/@D~2.0"))
                 .build());
   }