Merge pull request #18 from vinnybod/classifier-pom-followup

DP-14758 - pom file follow up

README.md

@@ -15,7 +15,7 @@ Table of Contents
     - [Projects using rules_jvm_external](#projects-using-rules_jvm_external)
 - [Prerequisites](#prerequisites)
 - [Usage](#usage)
-    - [With bzlmod (Bazel 6 and above)](#with-bzlmod-bazel-6-and-above)
+    - [With bzlmod (Bazel 7 and above)](#with-bzlmod-bazel-7-and-above)
     - [With WORKSPACE file (legacy)](#with-workspace-file-legacy)
 - [API Reference](#api-reference)
 - [Pinning artifacts and integration with Bazel's downloader](#pinning-artifacts-and-integration-with-bazels-downloader)

docs/bzlmod.md

@@ -30,6 +30,16 @@ maven.install(
         "org.seleniumhq.selenium:selenium-java:4.4.0",
     ],
 )
+
+# You can split off individual artifacts to define artifact-specific options (this example sets `neverlink`).
+# The `maven.install` and `maven.artifact` tags will be merged automatically.
+maven.artifact(
+    artifact = "javapoet",
+    group = "com.squareup",
+    neverlink = True,
+    version = "1.11.1",
+)
+
 use_repo(maven, "maven")
 ```
 
@@ -60,14 +70,8 @@ maven.install(
 )
 ```
 
-Finally, update the `use_repo` call to also expose the `unpinned_maven` repository used to update the dependencies:
-
-```starlark
-use_repo(maven, "maven", "unpinned_maven")
-```
-
-Now you'll be able to use the same `@unpinned_maven//:pin` operation described in the
-[workspace instructions](/README.md#updating-maven_installjson).
+Now you'll be able to use the same `REPIN=1 bazel run @maven//:pin` operation described in the
+[workspace instructions](/README.md#updating-maven_installjson) to update the dependencies.
 
 ## Artifact exclusion
 
@@ -93,4 +97,3 @@ maven.install(
 ## Known issues
 
 - Some error messages print instructions that don't apply under bzlmod, e.g. https://github.com/bazelbuild/rules_jvm_external/issues/827
-- Java Gazelle extension [isn't available](https://github.com/bazel-contrib/rules_jvm/issues/123)

private/extensions/maven.bzl

@@ -412,6 +412,7 @@ def _maven_impl(mctx):
                 # created from the maven_install.json file in the coursier_fetch
                 # invocation after this.
                 name = "unpinned_" + name if repo.get("lock_file") else name,
+                pinned_repo_name = name if repo.get("lock_file") else None,
                 user_provided_name = name,
                 repositories = repo.get("repositories"),
                 artifacts = artifacts_json,

private/rules/coursier.bzl

@@ -137,6 +137,9 @@ def _is_file(repository_ctx, path):
 def _is_directory(repository_ctx, path):
     return repository_ctx.which("test") and repository_ctx.execute(["test", "-d", path]).return_code == 0
 
+def _is_unpinned(repository_ctx):
+    return repository_ctx.attr.pinned_repo_name != ""
+
 def _shell_quote(s):
     # Lifted from
     #   https://github.com/bazelbuild/bazel-skylib/blob/6a17363a3c27dde70ab5002ad9f2e29aff1e1f4b/lib/shell.bzl#L49
@@ -875,7 +878,7 @@ def make_coursier_dep_tree(
         cmd.append("--default=true")
 
     environment = {}
-    if not repository_ctx.attr.name.startswith("unpinned_"):
+    if not _is_unpinned(repository_ctx):
         coursier_cache_location = get_coursier_cache_or_default(
             repository_ctx,
             False,
@@ -1009,7 +1012,7 @@ def _coursier_fetch_impl(repository_ctx):
     # TODO(jin): allow custom cache locations
     coursier_cache_path = get_coursier_cache_or_default(
         repository_ctx,
-        repository_ctx.attr.name.startswith("unpinned_"),
+        _is_unpinned(repository_ctx),
     ).replace("//", "/")
 
     for artifact in dep_tree["dependencies"]:
@@ -1032,13 +1035,13 @@ def _coursier_fetch_impl(repository_ctx):
             #    to file within the repository rule workspace
             print("Assuming maven local for artifact: %s" % artifact["coord"])
             artifact.update({"url": None})
-            if not repository_ctx.attr.name.startswith("unpinned_"):
+            if not _is_unpinned(repository_ctx):
                 artifact.update({"file": _relativize_and_symlink_file_in_maven_local(repository_ctx, artifact["file"])})
 
             files_to_inspect.append(repository_ctx.path(artifact["file"]))
             continue
 
-        if repository_ctx.attr.name.startswith("unpinned_"):
+        if _is_unpinned(repository_ctx):
             artifact.update({"file": _relativize_and_symlink_file_in_coursier_cache(repository_ctx, artifact["file"], coursier_cache_path)})
 
         # Coursier saves the artifacts into a subdirectory structure
@@ -1231,13 +1234,13 @@ def _coursier_fetch_impl(repository_ctx):
         },
         override_targets = repository_ctx.attr.override_targets,
         # Skip maven local dependencies if generating the unpinned repository
-        skip_maven_local_dependencies = repository_ctx.attr.name.startswith("unpinned_"),
+        skip_maven_local_dependencies = _is_unpinned(repository_ctx),
     )
 
     # This repository rule can be either in the pinned or unpinned state, depending on when
     # the user invokes artifact pinning. Normalize the repository name here.
-    if repository_ctx.name.startswith("unpinned_"):
-        repository_name = repository_ctx.name[len("unpinned_"):]
+    if _is_unpinned(repository_ctx):
+        repository_name = repository_ctx.attr.pinned_repo_name
         outdated_build_file_content = ""
     else:
         repository_name = repository_ctx.name
@@ -1439,6 +1442,10 @@ coursier_fetch = repository_rule(
         ),
         "ignore_empty_files": attr.bool(default = False, doc = "Treat jars that are empty as if they were not found."),
         "additional_coursier_options": attr.string_list(doc = "Additional options that will be passed to coursier."),
+        "pinned_repo_name": attr.string(
+            doc = "Name of the corresponding pinned repo for this repo. Presence implies that this is an unpinned repo.",
+            mandatory = False,
+        ),
     },
     environ = [
         "JAVA_HOME",

private/rules/maven_bom.bzl

@@ -41,7 +41,7 @@ def _maven_bom_impl(ctx):
     bom = generate_pom(
         ctx,
         coordinates = coordinates,
-        versioned_dep_coordinates = [f[MavenBomFragmentInfo].coordinates for f in ctx.attr.fragments],
+        versioned_dep_coordinates = [unpack_coordinates(f[MavenBomFragmentInfo].coordinates) for f in ctx.attr.fragments],
         pom_template = ctx.file.pom_template,
         out_name = "%s.xml" % ctx.label.name,
     )
@@ -77,13 +77,22 @@ def _maven_dependencies_bom_impl(ctx):
     # included in the main BOM
     first_order_deps = [f[MavenBomFragmentInfo].coordinates for f in ctx.attr.fragments]
     all_deps = depset(transitive = [f.maven_info.maven_deps for f in fragments]).to_list()
-    combined_deps = [a for a in all_deps if a not in first_order_deps]
+    combined_deps = [unpack_coordinates(a) for a in all_deps if a not in first_order_deps]
 
     unpacked = unpack_coordinates(ctx.attr.bom_coordinates)
+    unpacked = struct(
+        groupId = unpacked.groupId,
+        artifactId = unpacked.artifactId,
+        type = "pom",
+        scope = "import",
+        classifier = unpacked.classifier,
+        version = unpacked.version,
+    )
+
     dependencies_bom = generate_pom(
         ctx,
         coordinates = ctx.attr.maven_coordinates,
-        versioned_dep_coordinates = combined_deps + ["%s:%s:pom:import:%s" % (unpacked.groupId, unpacked.artifactId, unpacked.version)],
+        versioned_dep_coordinates = combined_deps + [unpacked],
         pom_template = ctx.file.pom_template,
         out_name = "%s.xml" % ctx.label.name,
         indent = 12,

private/rules/maven_install.bzl

@@ -123,6 +123,7 @@ def maven_install(
             # created from the maven_install.json file in the coursier_fetch
             # invocation after this.
             name = name if maven_install_json == None else "unpinned_" + name,
+            pinned_repo_name = None if maven_install_json == None else name,
             repositories = repositories_json_strings,
             artifacts = artifacts_json_strings,
             fail_on_missing_checksum = fail_on_missing_checksum,

private/rules/maven_utils.bzl

@@ -4,7 +4,7 @@ def unpack_coordinates(coords):
     where type and scope are optional.
 
     Assumes following maven coordinate syntax:
-    groupId:artifactId[:type[:classifer]]:version
+    groupId:artifactId[:type[:classifier]]:version
     """
     if not coords:
         return None
@@ -147,17 +147,16 @@ def generate_pom(
         substitutions.update({"{parent}": "".join(parts)})
 
     deps = []
-    for dep in sorted(versioned_dep_coordinates) + sorted(unversioned_dep_coordinates):
+    for dep in _sort_unpacked(versioned_dep_coordinates) + _sort_unpacked(unversioned_dep_coordinates):
         include_version = dep in versioned_dep_coordinates
-        unpacked = unpack_coordinates(dep)
-        new_scope = "runtime" if dep in runtime_deps else unpacked.scope
+        new_scope = "runtime" if dep in runtime_deps else dep.scope
         unpacked = struct(
-            groupId = unpacked.groupId,
-            artifactId = unpacked.artifactId,
-            type = unpacked.type,
+            groupId = dep.groupId,
+            artifactId = dep.artifactId,
+            type = dep.type,
             scope = new_scope,
-            classifier = unpacked.classifier,
-            version = unpacked.version,
+            classifier = dep.classifier,
+            version = dep.version,
         )
         deps.append(format_dep(unpacked, indent = indent, exclusions = exclusions.get(dep, {}), include_version = include_version))
 
@@ -192,3 +191,11 @@ def determine_additional_dependencies(jar_files, additional_dependencies):
                     to_return.append(dep)
 
     return to_return
+
+def _sort_unpacked(unpacked_dep):
+    """Sorts a list of unpacked dependencies by groupId, artifactId, and version."""
+
+    def _sort_key(dep):
+        return (dep.groupId, dep.artifactId, dep.version)
+
+    return sorted(unpacked_dep, key = _sort_key)

private/rules/pom_file.bzl

@@ -1,6 +1,6 @@
 load("@rules_java//java:defs.bzl", "JavaInfo")
 load(":has_maven_deps.bzl", "MavenInfo", "calculate_artifact_jars", "has_maven_deps")
-load(":maven_utils.bzl", "determine_additional_dependencies", "generate_pom")
+load(":maven_utils.bzl", "determine_additional_dependencies", "generate_pom", "unpack_coordinates")
 
 def _pom_file_impl(ctx):
     # Ensure the target has coordinates
@@ -16,7 +16,8 @@ def _pom_file_impl(ctx):
         if not info.label_to_javainfo.get(target.label):
             fail("exclusions key %s not found in dependencies %s" % (target, info.label_to_javainfo.keys()))
         else:
-            return ctx.expand_make_variables("exclusions", target[MavenInfo].coordinates, ctx.var)
+            coords = ctx.expand_make_variables("exclusions", target[MavenInfo].coordinates, ctx.var)
+            return unpack_coordinates(coords)
 
     exclusions = {
         get_exclusion_coordinates(target): json.decode(targetExclusions)
@@ -31,11 +32,11 @@ def _pom_file_impl(ctx):
             all_maven_deps.append(coords)
 
     expanded_maven_deps = [
-        ctx.expand_make_variables("additional_deps", coords, ctx.var)
+        unpack_coordinates(ctx.expand_make_variables("additional_deps", coords, ctx.var))
         for coords in all_maven_deps
     ]
     expanded_runtime_deps = [
-        ctx.expand_make_variables("maven_runtime_deps", coords, ctx.var)
+        unpack_coordinates(ctx.expand_make_variables("maven_runtime_deps", coords, ctx.var))
         for coords in runtime_maven_deps
     ]
 
@@ -45,7 +46,7 @@ def _pom_file_impl(ctx):
     out = generate_pom(
         ctx,
         coordinates = coordinates,
-        versioned_dep_coordinates = sorted(expanded_maven_deps),
+        versioned_dep_coordinates = expanded_maven_deps,
         runtime_deps = expanded_runtime_deps,
         pom_template = ctx.file.pom_template,
         out_name = "%s.xml" % ctx.label.name,

tests/integration/maven_bom/BUILD

@@ -37,6 +37,10 @@ java_export(
     name = "one-dep",
     srcs = ["OneDep.java"],
     maven_coordinates = "com.example:one-dep:1.0.0",
+    visibility = [
+        ":__pkg__",
+        "//tests/integration/pom_file:__pkg__",
+    ],
     deps = [
         artifact("com.google.guava:guava"),
     ],

tests/integration/pom_file/BUILD

@@ -0,0 +1,67 @@
+load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
+load("//:defs.bzl", "artifact", "java_export", "maven_bom")
+
+java_export(
+    name = "pom-example",
+    srcs = ["PomExample.java"],
+    maven_coordinates = "com.example:app:1.0.0",
+    deps = [
+        "//tests/integration/maven_bom:one-dep",
+    ],
+)
+
+diff_test(
+    name = "validate-pom",
+    file1 = ":pom-example-pom",
+    file2 = "pom.golden.xml",
+)
+
+java_export(
+    name = "pom-example-with-runtime-dep",
+    srcs = ["PomExample.java"],
+    maven_coordinates = "com.example:app:1.0.0",
+    runtime_deps = [
+        "//tests/integration/maven_bom:one-dep",
+    ],
+    deps = [
+        "//tests/integration/maven_bom:one-dep",
+        artifact("com.google.guava:guava"),
+    ],
+)
+
+diff_test(
+    name = "validate-pom-with-runtime-dep",
+    file1 = ":pom-example-with-runtime-dep-pom",
+    file2 = "pom-with-runtime-dep.golden.xml",
+)
+
+java_export(
+    name = "pom-example-with-classifier-dep",
+    srcs = ["PomExample.java"],
+    maven_coordinates = "com.example:app:1.0.0",
+    runtime_deps = [
+        ":classifier-artifact-one",
+    ],
+    deps = [
+        ":classifier-artifact-one",
+        ":classifier-artifact-two",
+    ],
+)
+
+java_export(
+    name = "classifier-artifact-one",
+    srcs = ["ClassifierArtifactOne.java"],
+    maven_coordinates = "com.example:lib:jar:linux-x86:1.0.0",
+)
+
+java_export(
+    name = "classifier-artifact-two",
+    srcs = ["ClassifierArtifactTwo.java"],
+    maven_coordinates = "com.example:lib:jar:linux-arm64:1.0.0",
+)
+
+diff_test(
+    name = "validate-pom-example-with-classifier-dep",
+    file1 = ":pom-example-with-classifier-dep-pom",
+    file2 = "pom-with-classifier-dep.golden.xml",
+)

tests/integration/pom_file/ClassifierArtifactOne.java

@@ -0,0 +1,5 @@
+package tests.integration.pom_file;
+
+public class ClassifierArtifactOne {
+  // This space left blank intentionally
+}

tests/integration/pom_file/ClassifierArtifactTwo.java

@@ -0,0 +1,5 @@
+package tests.integration.pom_file;
+
+public class ClassifierArtifactTwo {
+  // This space left blank intentionally
+}

tests/integration/pom_file/PomExample.java

@@ -0,0 +1,9 @@
+package tests.integration.pom_file;
+
+public class PomExample {
+
+  public static void main(String[] args) {
+    System.out.println("Hello World!");
+  }
+
+}

tests/integration/pom_file/pom-with-classifier-dep.golden.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>com.example</groupId>
+    <artifactId>app</artifactId>
+    <version>1.0.0</version>
+
+    <dependencies>
+        <dependency>
+            <groupId>com.example</groupId>
+            <artifactId>lib</artifactId>
+            <version>1.0.0</version>
+            <scope>runtime</scope>
+            <classifier>linux-x86</classifier>
+        </dependency>
+        <dependency>
+            <groupId>com.example</groupId>
+            <artifactId>lib</artifactId>
+            <version>1.0.0</version>
+            <classifier>linux-arm64</classifier>
+        </dependency>
+    </dependencies>
+</project>