Rework how we determine coordinate formats

private/coursier_utilities.bzl

@@ -15,25 +15,11 @@
 # For example, some jars have the type "eclipse-plugin", and Coursier would not
 # download them if it's not asked to to resolve "eclipse-plugin".
 
+load("@bazel_skylib//lib:structs.bzl", "structs")
 load("//:specs.bzl", "parse")
+load("//private/lib:coordinates.bzl", _SUPPORTED_PACKAGING_TYPES = "SUPPORTED_PACKAGING_TYPES", "unpack_coordinates")
 
-SUPPORTED_PACKAGING_TYPES = [
-    "jar",
-    "json",
-    "aar",
-    "bundle",
-    "eclipse-plugin",
-    "exe",
-    "orbit",
-    "test-jar",
-    "hk2-jar",
-    "maven-plugin",
-    "scala-jar",
-    "dylib",
-    "so",
-    "dll",
-    "pom",
-]
+SUPPORTED_PACKAGING_TYPES = _SUPPORTED_PACKAGING_TYPES
 
 # See https://github.com/bazelbuild/rules_jvm_external/issues/686
 # A single package uses classifier to distinguish the jar files (one per platform),
@@ -47,22 +33,33 @@ PLATFORM_CLASSIFIER = [
 ]
 
 def strip_packaging_and_classifier(coord):
-    # Strip some packaging and classifier values based on the following maven coordinate formats
+    # Strip some packaging and classifier values.
+
+    # We want to modify some of the values
+    unpacked_struct = unpack_coordinates(coord)
+    unpacked = {} | structs.to_dict(unpacked_struct)
+
+    if unpacked.get("classifier", None) in ["sources", "native"]:
+        unpacked["classifier"] = None
+
+    # We add "pom" into SUPPORTED_PACKAGING_TYPES here because "pom" is not a
+    # packaging type that Coursier CLI accepts.
+    if unpacked.get("packaging", None)  in SUPPORTED_PACKAGING_TYPES + ["pom"]:
+        unpacked["packaging"] = None
+
+    # We are expected to return one of:
+    #
     # groupId:artifactId:version
     # groupId:artifactId:packaging:version
     # groupId:artifactId:packaging:classifier:version
-    coordinates = coord.split(":")
-    if len(coordinates) > 4:
-        if coordinates[3] in ["sources", "natives"]:
-            coordinates.pop(3)
-
-    if len(coordinates) > 3:
-        # We add "pom" into SUPPORTED_PACKAGING_TYPES here because "pom" is not a
-        # packaging type that Coursier CLI accepts.
-        if coordinates[2] in SUPPORTED_PACKAGING_TYPES + ["pom"]:
-            coordinates.pop(2)
-
-    return ":".join(coordinates)
+    #
+    # TODO: check call sites and see what people do with the returned string
+    # Can we use use coordinates.bzl%to_external_form?
+    to_return = unpacked["group"]
+    for item in ["artifact", "packaging", "classifier", "version"]:
+        if unpacked.get(item, None):
+            to_return += ":" + unpacked[item]
+    return to_return
 
 def strip_packaging_and_classifier_and_version(coord):
     coordinates = coord.split(":")

private/lib/coordinates.bzl

@@ -1,3 +1,21 @@
+SUPPORTED_PACKAGING_TYPES = [
+    "aar",
+    "bundle",
+    "dll",
+    "eclipse-plugin",
+    "exe",
+    "dylib",
+    "hk2-jar",
+    "jar",
+    "json",
+    "maven-plugin",
+    "orbit",
+    "pom",
+    "scala-jar",
+    "so",
+    "test-jar",
+]
+
 def unpack_coordinates(coords):
     """Takes a maven coordinate and unpacks it into a struct with fields
     `group`, `artifact`, `version`, `packaging`, `classifier`
@@ -17,46 +35,61 @@ def unpack_coordinates(coords):
     if len(pieces) == 2:
         return struct(group = group, artifact = artifact, version = "", packaging = None, classifier = None)
 
+    # Strictly speaking this could be one of `g:a:p` or `g:a:v` but realistically, it's going to be
+    # a regular `g:a:v` triplet. If that's not what someone wants, they'll need to qualify the
+    # type in some other way
+    if len(pieces) == 3:
+        version = pieces[2]
+        packaging = None
+        if "@" in pieces[2]:
+            (version, packaging) = pieces[2].split("@", 2)
+
+        return struct(group = group, artifact = artifact, version = version, packaging = packaging, classifier = None)
+
     # Unambiguously the original format
     if len(pieces) == 5:
         packaging = pieces[2]
         classifier = pieces[3]
         version = pieces[4]
         return struct(group = group, artifact = artifact, packaging = packaging, classifier = classifier, version = version)
 
-    # If we're using BOMs, the version is optional. That means at this point
-    # we could be dealing with g:a:p or g:a:v
-    is_gradle = pieces[2][0].isdigit()
+    if len(pieces) == 4:
+        # Either g:a:p:v or g:a:v:c or g:a:v:c@p.
 
-    if len(pieces) == 3:
-        if is_gradle:
-            if "@" in pieces[2]:
-                (version, packaging) = pieces[2].split("@", 2)
-                return struct(group = group, artifact = artifact, packaging = packaging, version = version, classifier = None)
+        # Handle the easy case first
+        if "@" in pieces[3]:
+            (classifier, packaging) = pieces[3].split("@", 2)
             version = pieces[2]
-            return struct(group = group, artifact = artifact, version = version, packaging = None, classifier = None)
-        else:
+            return struct(group = group, artifact = artifact, packaging = packaging, classifier = classifier, version = version)
+
+        # Experience has show us that versions can be almost anything, but there's a relatively small
+        # pool of packaging types that people use. This isn't a perfect heuristic, but it's Good
+        # Enough for us right now. Previously, we attempted to figure out if the `piece[2]` was a
+        # version by checking to see whether it's a number. Foolish us.
+
+        if pieces[2] in SUPPORTED_PACKAGING_TYPES:
             packaging = pieces[2]
-            return struct(group = group, artifact = artifact, packaging = packaging, version = "", classifier = None)
+            version = pieces[3]
+            rewritten = "%s:%s:%s@%s" % (group, artifact, version, packaging)
+            print("Assuming %s should be interpreted as %s" % (coords, rewritten))
+            return struct(group = group, artifact = artifact, packaging = packaging, version = version, classifier = None)
 
-    if len(pieces) == 4:
-        if is_gradle:
+        # We could still be in one of `g:a:p:v` or `g:a:v:c`, but it's likely the latter. I do not
+        # think there are any packaging formats commonly in use in the maven ecosystem that contain
+        # numbers, so we'll check to see if `pieces[2]` contains one or more numbers and use that to
+        # decide. This allows us to cope with packaging formats we've not seen before, such as the
+        # `packaging` we use in our own tests.
+        digit_count = len([c for c in pieces[2].elems() if c.isdigit()])
+
+        if digit_count:
             version = pieces[2]
-            if "@" in pieces[3]:
-                (classifier, packaging) = pieces[3].split("@", 2)
-                return struct(group = group, artifact = artifact, packaging = packaging, classifier = classifier, version = version)
             classifier = pieces[3]
             return struct(group = group, artifact = artifact, classifier = classifier, version = version, packaging = None)
         else:
-            packaging = pieces[2]
-            version = pieces[3]
-            return struct(group = group, artifact = artifact, packaging = packaging, version = version, classifier = None)
+            return struct(group = group, artifact = artifact, classifier = None, version = pieces[3], packaging = pieces[2])
 
     fail("Could not parse maven coordinate: %s" % coords)
 
-def _is_version_number(part):
-    return part[0].isdigit()
-
 def to_external_form(coords):
     """Formats `coords` as a string suitable for use by tools such as Gradle.
 

tests/unit/coordinates_test.bzl

@@ -42,11 +42,11 @@ complete_original_format_test = unittest.make(_complete_original_format_impl)
 def _original_format_omitting_scope_impl(ctx):
     env = unittest.begin(ctx)
 
-    unpacked = unpack_coordinates("group:artifact:type:1.2.3")
+    unpacked = unpack_coordinates("group:artifact:test-jar:1.2.3")
     asserts.equals(env, "group", unpacked.group)
     asserts.equals(env, "artifact", unpacked.artifact)
     asserts.equals(env, "1.2.3", unpacked.version)
-    asserts.equals(env, "type", unpacked.packaging)
+    asserts.equals(env, "test-jar", unpacked.packaging)
     asserts.equals(env, None, unpacked.classifier)
 
     return unittest.end(env)
@@ -100,10 +100,11 @@ def _multiple_formats_impl(ctx):
 
     coords_to_structs = {
         "groupId:artifactId:1.2.3": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = None, packaging = None),
-        "groupId:artifactId:type:1.2.3": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = None, packaging = "type"),
+        "groupId:artifactId:test-jar:1.2.3": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = None, packaging = "test-jar"),
         "groupId:artifactId:type:classifier:1.2.3": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = "classifier", packaging = "type"),
         "groupId:artifactId:1.2.3@type": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = None, packaging = "type"),
         "groupId:artifactId:1.2.3:classifier@type": struct(group = "groupId", artifact = "artifactId", version = "1.2.3", classifier = "classifier", packaging = "type"),
+        "io.netty:netty-transport-native-unix-common:jar:linux-aarch_64:4.1.100.Final": struct(group = "io.netty", artifact = "netty-transport-native-unix-common", version = "4.1.100.Final", classifier = "linux-aarch_64", packaging = "jar"),
     }
 
     for (coords, expected) in coords_to_structs.items():
@@ -114,6 +115,17 @@ def _multiple_formats_impl(ctx):
 
 multiple_formats_test = unittest.make(_multiple_formats_impl)
 
+def _unusual_version_format_impl(ctx):
+    env = unittest.begin(ctx)
+
+    unpacked = unpack_coordinates("group:artifact:FY21R16")
+
+    asserts.equals(env, "FY21R16", unpacked.version)
+
+    return unittest.end(env)
+
+unusual_version_format_test = unittest.make(_unusual_version_format_impl)
+
 def coordinates_test_suite():
     unittest.suite(
         "coordinates_tests",
@@ -125,4 +137,5 @@ def coordinates_test_suite():
         gradle_format_with_type_and_classifier_test,
         gradle_format_with_type_but_no_classifier_test,
         multiple_formats_test,
+        unusual_version_format_test,
     )

tests/unit/exports/BUILD

@@ -1,4 +1,3 @@
-load("@rules_java//java:defs.bzl", "java_library")
 load("//tests/unit/exports:exports_test.bzl", "exports_tests")
 
 exports_tests(name = "exports_tests")