bzlmod: Use a Version object (née ParsedVersion) everywhere instead o…

…f a string (#13316) See motivating TODO in ModuleFileGlobals.java. This avoids double-parsing (once at input verification time and once at selection time). PiperOrigin-RevId: 384243747
bazelbuild · Jul 12, 2021 · 32e91a7 · 32e91a7
1 parent c5c15f9
commit 32e91a7
Show file tree

Hide file tree

Showing 19 changed files with 479 additions and 356 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
@@ -18,11 +18,13 @@ java_library(
         "ArchiveRepoSpecBuilder.java",
         "ModuleKey.java",
         "RepoSpec.java",
+        "Version.java",
     ],
     deps = [
         "//src/main/java/net/starlark/java/eval",
         "//third_party:auto_value",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -57,7 +59,6 @@ java_library(
         "ModuleFileValue.java",
         "ModuleOverride.java",
         "NonRegistryOverride.java",
-        "ParsedVersion.java",
         "RegistryOverride.java",
         "SelectionFunction.java",
         "SelectionValue.java",

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryFunction.java
@@ -43,7 +43,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     if (root == null) {
       return null;
     }
-    ModuleKey rootModuleKey = ModuleKey.create(root.getModule().getName(), "");
+    ModuleKey rootModuleKey = ModuleKey.create(root.getModule().getName(), Version.EMPTY);
     ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();
     Map<ModuleKey, Module> depGraph = new HashMap<>();
     depGraph.put(
@@ -86,13 +86,13 @@ private static Module rewriteDepKeys(
       Module module, ImmutableMap<String, ModuleOverride> overrides, String rootModuleName) {
     return module.withDepKeysTransformed(
         depKey -> {
-          String newVersion = depKey.getVersion();
+          Version newVersion = depKey.getVersion();
 
           @Nullable ModuleOverride override = overrides.get(depKey.getName());
           if (override instanceof NonRegistryOverride || rootModuleName.equals(depKey.getName())) {
-            newVersion = "";
+            newVersion = Version.EMPTY;
           } else if (override instanceof SingleVersionOverride) {
-            String overrideVersion = ((SingleVersionOverride) override).getVersion();
+            Version overrideVersion = ((SingleVersionOverride) override).getVersion();
             if (!overrideVersion.isEmpty()) {
               newVersion = overrideVersion;
             }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java
@@ -85,7 +85,8 @@ private Optional<byte[]> grabFile(String url, ExtendedEventHandler eventHandler)
   public Optional<byte[]> getModuleFile(ModuleKey key, ExtendedEventHandler eventHandler)
       throws IOException, InterruptedException {
     return grabFile(
-        constructUrl(getUrl(), "modules", key.getName(), key.getVersion(), "MODULE.bazel"),
+        constructUrl(
+            getUrl(), "modules", key.getName(), key.getVersion().toString(), "MODULE.bazel"),
         eventHandler);
   }
 
@@ -125,7 +126,8 @@ public RepoSpec getRepoSpec(ModuleKey key, String repoName, ExtendedEventHandler
             constructUrl(getUrl(), "bazel_registry.json"), BazelRegistryJson.class, eventHandler);
     Optional<SourceJson> sourceJson =
         grabJson(
-            constructUrl(getUrl(), "modules", key.getName(), key.getVersion(), "source.json"),
+            constructUrl(
+                getUrl(), "modules", key.getName(), key.getVersion().toString(), "source.json"),
             SourceJson.class,
             eventHandler);
     if (!sourceJson.isPresent()) {
@@ -163,7 +165,12 @@ public RepoSpec getRepoSpec(ModuleKey key, String repoName, ExtendedEventHandler
       for (Map.Entry<String, String> entry : sourceJson.get().patches.entrySet()) {
         remotePatches.put(
             constructUrl(
-                getUrl(), "modules", key.getName(), key.getVersion(), "patches", entry.getKey()),
+                getUrl(),
+                "modules",
+                key.getName(),
+                key.getVersion().toString(),
+                "patches",
+                entry.getKey()),
             entry.getValue());
       }
     }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Module.java
@@ -35,7 +35,7 @@ public abstract class Module {
   public abstract String getName();
 
   /** The version of the module. Must be empty iff the module has a {@link NonRegistryOverride}. */
-  public abstract String getVersion();
+  public abstract Version getVersion();
 
   /**
    * The compatibility level of the module, which essentially signifies the "major version" of the
@@ -81,7 +81,7 @@ public Module withDepKeysTransformed(UnaryOperator<ModuleKey> transform) {
   public abstract static class Builder {
     public abstract Builder setName(String value);
 
-    public abstract Builder setVersion(String value);
+    public abstract Builder setVersion(Version value);
 
     /** Optional; defaults to {@code 0}. */
     public abstract Builder setCompatibilityLevel(int value);

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
@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction.ModuleFileFunctionException;
+import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
 import com.google.devtools.build.lib.starlarkbuildapi.repository.ModuleFileGlobalsApi;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -43,11 +44,16 @@ public void module(String name, String version, StarlarkInt compatibilityLevel)
       throw Starlark.errorf("the module() directive can only be called once");
     }
     moduleCalled = true;
-    // TODO(wyv): add validation logic for name (alphanumerical) and version (use ParsedVersion) &
-    //   others in the future
+    // TODO(wyv): add validation logic for name (alphanumerical) & others in the future
+    Version parsedVersion;
+    try {
+      parsedVersion = Version.parse(version);
+    } catch (ParseException e) {
+      throw new EvalException("Invalid version in module()", e);
+    }
     module
         .setName(name)
-        .setVersion(version)
+        .setVersion(parsedVersion)
         .setCompatibilityLevel(compatibilityLevel.toInt("compatibility_level"));
   }
 
@@ -56,9 +62,14 @@ public void bazelDep(String name, String version, String repoName) throws EvalEx
     if (repoName.isEmpty()) {
       repoName = name;
     }
-    // TODO(wyv): add validation logic for name (alphanumerical), version (use ParsedVersion),
-    //   and repoName (RepositoryName?)
-    if (deps.putIfAbsent(repoName, ModuleKey.create(name, version)) != null) {
+    // TODO(wyv): add validation logic for name (alphanumerical) and repoName (RepositoryName?)
+    Version parsedVersion;
+    try {
+      parsedVersion = Version.parse(version);
+    } catch (ParseException e) {
+      throw new EvalException("Invalid version in bazel_dep()", e);
+    }
+    if (deps.putIfAbsent(repoName, ModuleKey.create(name, parsedVersion)) != null) {
       throw Starlark.errorf("a bazel_dep with the repo name %s already exists", repoName);
     }
   }
@@ -94,10 +105,16 @@ public void singleVersionOverride(
       Iterable<?> patches,
       StarlarkInt patchStrip)
       throws EvalException {
+    Version parsedVersion;
+    try {
+      parsedVersion = Version.parse(version);
+    } catch (ParseException e) {
+      throw new EvalException("Invalid version in single_version_override()", e);
+    }
     addOverride(
         moduleName,
         SingleVersionOverride.create(
-            version,
+            parsedVersion,
             registry,
             checkAllStrings(patches, "patches"),
             patchStrip.toInt("single_version_override.patch_strip")));

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
@@ -31,7 +31,7 @@
 @AutoValue
 public abstract class ModuleFileValue implements SkyValue {
 
-  public static final ModuleKey ROOT_MODULE_KEY = ModuleKey.create("", "");
+  public static final ModuleKey ROOT_MODULE_KEY = ModuleKey.create("", Version.EMPTY);
 
   /**
    * The module resulting from the module file evaluation. Note, in particular, that the version of

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
@@ -21,20 +21,20 @@
 @AutoValue
 public abstract class ModuleKey {
 
-  public static ModuleKey create(String name, String version) {
+  public static ModuleKey create(String name, Version version) {
     return new AutoValue_ModuleKey(name, version);
   }
 
   /** The name of the module. Can be empty for the root module (if it doesn't specify a name). */
   public abstract String getName();
 
   /** The version of the module. Must be empty iff the module has a {@link NonRegistryOverride}. */
-  public abstract String getVersion();
+  public abstract Version getVersion();
 
   @Override
   public final String toString() {
     return (getName().isEmpty() ? "_" : getName())
         + "@"
-        + (getVersion().isEmpty() ? "_" : getVersion());
+        + (getVersion().isEmpty() ? "_" : getVersion().toString());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SelectionFunction.java
@@ -58,18 +58,11 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
     // First figure out the version to select for every selection group.
     ImmutableMap<ModuleKey, Module> depGraph = discovery.getDepGraph();
-    Map<SelectionGroup, ParsedVersion> selectedVersions = new HashMap<>();
+    Map<SelectionGroup, Version> selectedVersions = new HashMap<>();
     for (Map.Entry<ModuleKey, Module> entry : depGraph.entrySet()) {
       ModuleKey key = entry.getKey();
       Module module = entry.getValue();
-
-      ParsedVersion parsedVersion;
-      try {
-        parsedVersion = ParsedVersion.parse(key.getVersion());
-      } catch (ParsedVersion.ParseException e) {
-        throw new SelectionFunctionException(e);
-      }
-      selectedVersions.merge(SelectionGroup.of(module), parsedVersion, ParsedVersion::max);
+      selectedVersions.merge(SelectionGroup.of(module), key.getVersion(), Version::max);
     }
 
     // Now build a new dep graph where deps with unselected versions are removed.
@@ -79,7 +72,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       Module module = entry.getValue();
 
       // Remove any dep whose version isn't selected.
-      String selectedVersion = selectedVersions.get(SelectionGroup.of(module)).getOriginal();
+      Version selectedVersion = selectedVersions.get(SelectionGroup.of(module));
       if (!moduleKey.getVersion().equals(selectedVersion)) {
         continue;
       }
@@ -91,9 +84,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
               depKey ->
                   ModuleKey.create(
                       depKey.getName(),
-                      selectedVersions
-                          .get(SelectionGroup.of(depGraph.get(depKey)))
-                          .getOriginal())));
+                      selectedVersions.get(SelectionGroup.of(depGraph.get(depKey))))));
     }
     ImmutableMap<ModuleKey, Module> newDepGraph = newDepGraphBuilder.build();
 
@@ -104,7 +95,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     // multiple_version_override).
     DepGraphWalker walker = new DepGraphWalker(newDepGraph);
     try {
-      walker.walk(ModuleKey.create(discovery.getRootModuleName(), ""), null);
+      walker.walk(ModuleKey.create(discovery.getRootModuleName(), Version.EMPTY), null);
     } catch (SelectionException e) {
       throw new SelectionFunctionException(e);
     }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java
@@ -31,15 +31,15 @@
 public abstract class SingleVersionOverride implements RegistryOverride {
 
   public static SingleVersionOverride create(
-      String version, String registry, ImmutableList<String> patches, int patchStrip) {
+      Version version, String registry, ImmutableList<String> patches, int patchStrip) {
     return new AutoValue_SingleVersionOverride(version, registry, patches, patchStrip);
   }
 
   /**
    * The version to pin the module to. Can be empty if it shouldn't be pinned (in which case it will
    * still participate in version resolution).
    */
-  public abstract String getVersion();
+  public abstract Version getVersion();
 
   @Override
   public abstract String getRegistry();