bzlmod: enforce root artifact precedence when version_conflict_policy…

docs/bzlmod-api.md

@@ -114,7 +114,7 @@ Combines artifact and bom declarations with setting the location of lock files t
 | <a id="maven.install-strict_visibility_value"></a>strict_visibility_value |  -   | <a href="https://bazel.build/concepts/labels">List of labels</a> | optional |  `["@rules_jvm_external//visibility:private"]`  |
 | <a id="maven.install-use_credentials_from_home_netrc_file"></a>use_credentials_from_home_netrc_file |  Whether to pass machine login credentials from the ~/.netrc file to coursier.   | Boolean | optional |  `False`  |
 | <a id="maven.install-use_starlark_android_rules"></a>use_starlark_android_rules |  Whether to use the native or Starlark version of the Android rules.   | Boolean | optional |  `False`  |
-| <a id="maven.install-version_conflict_policy"></a>version_conflict_policy |  Policy for user-defined vs. transitive dependency version conflicts<br><br>If "pinned", choose the user-specified version in maven_install unconditionally. With the Gradle and Maven resolvers, this only applies to artifacts contributed by the root module. If "default", follow the selected resolver's default policy.   | String | optional |  `"default"`  |
+| <a id="maven.install-version_conflict_policy"></a>version_conflict_policy |  Policy for user-defined vs. transitive dependency version conflicts<br><br>If "pinned", choose root-declared versioned artifacts unconditionally. Under bzlmod layered installs this applies before merging artifacts from contributing modules; Gradle forces one version per module, while Maven and Coursier force each versioned coordinate independently. If "default", follow the selected resolver's default policy.   | String | optional |  `"default"`  |
 
 <a id="maven.override"></a>
 

private/extensions/maven.bzl

@@ -109,8 +109,10 @@ install = tag_class(
         "version_conflict_policy": attr.string(
             doc = """Policy for user-defined vs. transitive dependency version conflicts
 
-            If "pinned", choose the user-specified version in maven_install unconditionally.
-            With the Gradle and Maven resolvers, this only applies to artifacts contributed by the root module.
+            If "pinned", choose root-declared versioned artifacts unconditionally. Under bzlmod
+            layered installs this applies before merging artifacts from contributing modules;
+            Gradle forces one version per module, while Maven and Coursier force each versioned
+            coordinate independently.
             If "default", follow the selected resolver's default policy.
             """,
             default = "default",
@@ -242,11 +244,12 @@ def _deduplicate_non_root_artifacts(bazel_dep_to_non_root_artifacts, return_only
 #
 # This can be typical for the default @maven namespace, if a bzlmod dependency
 # wishes to contribute to the users' jars.
-def _deduplicate_artifacts_with_root_priority(name, root_artifacts, bazel_dep_to_non_root_artifacts, repin_env_var, rje_verbose_env_var):
+def deduplicate_artifacts_with_root_priority(name, root_artifacts, bazel_dep_to_non_root_artifacts, repin_env_var, rje_verbose_env_var):
     """Deduplicate artifacts, giving priority to root module artifacts with force_version set."""
     non_root_coordinate_to_artifact = _deduplicate_non_root_artifacts(bazel_dep_to_non_root_artifacts)
 
     duplicate_artifact_warning = ""
+    deduped_root_artifacts = []
     filtered_non_root_artifacts = []
     for root_artifact in root_artifacts:
         artifact_key = to_key(root_artifact)
@@ -262,6 +265,15 @@ def _deduplicate_artifacts_with_root_priority(name, root_artifacts, bazel_dep_to
                         "Please update the version in your MODULE.bazel or set `force_version = True`."
                     )
 
+                    # The non-root artifact won the version comparison:
+                    # evict the losing root artifact so exactly one
+                    # version per coordinate reaches the resolver.
+                    # Otherwise both survive into the merged list and
+                    # installs with duplicate_version_warning = "error"
+                    # fail early in _check_artifacts_are_unique().
+                    continue
+        deduped_root_artifacts.append(root_artifact)
+
     # Add any remaining non root artifacts that weren't found in the root artifact list
     addtional_artifact_message = ""
     for bazel_dep_name, non_root_artifact in non_root_coordinate_to_artifact.values():
@@ -277,7 +289,7 @@ def _deduplicate_artifacts_with_root_priority(name, root_artifacts, bazel_dep_to
             if addtional_artifact_message != "":
                 print(addtional_artifact_message)
 
-    return root_artifacts + filtered_non_root_artifacts
+    return deduped_root_artifacts + filtered_non_root_artifacts
 
 def _get_tri_state_bool(amend_val, original_val):
     if amend_val in ["true", "on"]:
@@ -631,8 +643,17 @@ def _forces_gradle_module_version(artifact, forced_versions):
     return version == forced_versions.get("%s:%s" % (artifact.group, artifact.artifact))
 
 def apply_root_version_conflict_policy(artifacts, resolver, version_conflict_policy):
-    """Applies the install-level conflict policy to root module artifacts."""
-    if resolver not in ["gradle", "maven"] or version_conflict_policy != "pinned":
+    """Applies the install-level conflict policy to root module artifacts.
+
+    With version_conflict_policy = "pinned", root-declared versions are
+    chosen unconditionally, so every versioned root artifact is treated
+    as force_version = True during layered (bzlmod) dependency merging,
+    for every resolver. The coursier resolver already forces all
+    declared versions at resolution time under "pinned"; marking the
+    root artifacts here extends the same precedence to the merge with
+    contributing modules' artifacts.
+    """
+    if version_conflict_policy != "pinned":
         return artifacts
 
     if resolver == "gradle":
@@ -739,15 +760,15 @@ def maven_impl(mctx):
                         if k not in bazel_dep_to_non_root_boms.keys():
                             print("\nINFO: The @%s repo is not using boms from %s because it is not in the known_contributing_modules" % (repo_name, k))
 
-            merged_repo["artifacts"] = _deduplicate_artifacts_with_root_priority(
+            merged_repo["artifacts"] = deduplicate_artifacts_with_root_priority(
                 repo_name,
                 root_artifacts,
                 bazel_dep_to_non_root_artifacts,
                 repin_env_var,
                 rje_verbose_env_var,
             )
 
-            merged_repo["boms"] = _deduplicate_artifacts_with_root_priority(
+            merged_repo["boms"] = deduplicate_artifacts_with_root_priority(
                 repo_name,
                 root_boms,
                 bazel_dep_to_non_root_boms,

tests/custom_maven_install/coursier_resolved_install.json

@@ -2,7 +2,7 @@
   "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL",
   "__INPUT_ARTIFACTS_HASH": {
     "com.google.auth:google-auth-library-oauth2-http": 991267597,
-    "com.google.auto:auto-common": -832702775,
+    "com.google.auto:auto-common": 605821486,
     "com.google.cloud:google-cloud-bigquery": 1661989687,
     "com.google.cloud:libraries-bom": 710163787,
     "repositories": -1949687017
@@ -882,20 +882,12 @@
       "autovalue.shaded.com.google.auto.service",
       "autovalue.shaded.com.google.common.annotations",
       "autovalue.shaded.com.google.common.base",
-      "autovalue.shaded.com.google.common.cache",
       "autovalue.shaded.com.google.common.collect",
-      "autovalue.shaded.com.google.common.escape",
-      "autovalue.shaded.com.google.common.eventbus",
-      "autovalue.shaded.com.google.common.graph",
       "autovalue.shaded.com.google.common.hash",
-      "autovalue.shaded.com.google.common.html",
       "autovalue.shaded.com.google.common.io",
       "autovalue.shaded.com.google.common.math",
-      "autovalue.shaded.com.google.common.net",
       "autovalue.shaded.com.google.common.primitives",
       "autovalue.shaded.com.google.common.reflect",
-      "autovalue.shaded.com.google.common.util.concurrent",
-      "autovalue.shaded.com.google.common.xml",
       "autovalue.shaded.com.google.errorprone.annotations",
       "autovalue.shaded.com.google.errorprone.annotations.concurrent",
       "autovalue.shaded.com.google.escapevelocity",
@@ -1162,8 +1154,6 @@
       "io.netty.util.concurrent",
       "io.netty.util.internal",
       "io.netty.util.internal.logging",
-      "io.netty.util.internal.shaded.org.jctools.counters",
-      "io.netty.util.internal.shaded.org.jctools.maps",
       "io.netty.util.internal.shaded.org.jctools.queues",
       "io.netty.util.internal.shaded.org.jctools.queues.atomic",
       "io.netty.util.internal.shaded.org.jctools.queues.atomic.unpadded",

tests/custom_maven_install/policy_pinned_testing_install.json

@@ -1,7 +1,48 @@
 {
   "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL",
-  "__INPUT_ARTIFACTS_HASH": 316608203,
-  "__RESOLVED_ARTIFACTS_HASH": -968606658,
+  "__INPUT_ARTIFACTS_HASH": {
+    "com.google.cloud:google-cloud-storage": -1491579406,
+    "com.google.guava:guava": -1853738824,
+    "repositories": -1949687017
+  },
+  "__RESOLVED_ARTIFACTS_HASH": {
+    "com.fasterxml.jackson.core:jackson-core": -262972278,
+    "com.google.api-client:google-api-client": -793854737,
+    "com.google.api.grpc:proto-google-common-protos": -1885550498,
+    "com.google.api.grpc:proto-google-iam-v1": -453830571,
+    "com.google.api:api-common": -549936361,
+    "com.google.api:gax": 805574868,
+    "com.google.api:gax-httpjson": -1168617842,
+    "com.google.apis:google-api-services-storage": 158985735,
+    "com.google.auth:google-auth-library-credentials": 1709815480,
+    "com.google.auth:google-auth-library-oauth2-http": 735705443,
+    "com.google.cloud:google-cloud-core": -2024787513,
+    "com.google.cloud:google-cloud-core-http": -327232662,
+    "com.google.cloud:google-cloud-storage": -2032350017,
+    "com.google.code.findbugs:jsr305": 870839855,
+    "com.google.code.gson:gson": 1862043778,
+    "com.google.errorprone:error_prone_annotations": 875547987,
+    "com.google.guava:guava": -164902174,
+    "com.google.http-client:google-http-client": -294460505,
+    "com.google.http-client:google-http-client-apache": -921946789,
+    "com.google.http-client:google-http-client-appengine": -849655450,
+    "com.google.http-client:google-http-client-jackson2": -1187124445,
+    "com.google.j2objc:j2objc-annotations": 1702790440,
+    "com.google.oauth-client:google-oauth-client": 1855107119,
+    "com.google.protobuf:protobuf-java": 1939119124,
+    "com.google.protobuf:protobuf-java-util": -944010332,
+    "commons-codec:commons-codec": -1216058892,
+    "commons-logging:commons-logging": 1248790901,
+    "io.grpc:grpc-context": -1228139597,
+    "io.opencensus:opencensus-api": 1930972101,
+    "io.opencensus:opencensus-contrib-http-util": -528120454,
+    "javax.annotation:javax.annotation-api": -1009230154,
+    "org.apache.httpcomponents:httpclient": 483019193,
+    "org.apache.httpcomponents:httpcore": 283803218,
+    "org.checkerframework:checker-compat-qual": -1467964223,
+    "org.codehaus.mojo:animal-sniffer-annotations": -349140135,
+    "org.threeten:threetenbp": 659773227
+  },
   "artifacts": {
     "com.fasterxml.jackson.core:jackson-core": {
       "shasums": {
@@ -672,5 +713,5 @@
       ]
     }
   },
-  "version": "2"
+  "version": "3"
 }

tests/unit/BUILD

@@ -6,6 +6,7 @@ load(":coursier_test.bzl", "coursier_test_suite")
 load(":coursier_utilities_test.bzl", "coursier_utilities_test_suite")
 load(":dependency_tree_parser_test.bzl", "dependency_tree_parser_test_suite")
 load(":java_utilities_test.bzl", "java_utilities_test_suite")
+load(":layered_dedup_test.bzl", "layered_dedup_test_suite")
 load(":maven_version_test.bzl", "maven_version_test_suite")
 load(":proxy_test.bzl", "proxy_test_suite")
 load(":specs_test.bzl", "artifact_specs_test_suite")
@@ -31,6 +32,8 @@ dependency_tree_parser_test_suite()
 
 java_utilities_test_suite()
 
+layered_dedup_test_suite()
+
 maven_version_test_suite()
 
 proxy_test_suite()

tests/unit/layered_dedup_test.bzl

@@ -0,0 +1,103 @@
+"""Tests for layered (bzlmod) artifact deduplication with root priority."""
+
+load("@bazel_skylib//lib:partial.bzl", "partial")
+load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
+load("//private/extensions:maven.bzl", "deduplicate_artifacts_with_root_priority")
+load("//private/lib:coordinates.bzl", "unpack_coordinates")
+
+def _versions_of(artifacts, group, artifact):
+    return [a.version for a in artifacts if a.group == group and a.artifact == artifact]
+
+def _forced(coordinates):
+    a = unpack_coordinates(coordinates)
+    return struct(
+        group = a.group,
+        artifact = a.artifact,
+        version = a.version,
+        packaging = getattr(a, "packaging", None),
+        classifier = getattr(a, "classifier", None),
+        force_version = True,
+    )
+
+def _non_root_higher_version_evicts_root_artifact_impl(ctx):
+    env = unittest.begin(ctx)
+
+    # Regression test for the duplicate that survived dedup: the root
+    # declares 2.33, a contributing module declares 2.37, nothing is
+    # forced. Highest version wins -- and the losing root artifact must
+    # leave the list, or _check_artifacts_are_unique() fails installs
+    # with duplicate_version_warning = "error".
+    merged = deduplicate_artifacts_with_root_priority(
+        "external_deps",
+        [unpack_coordinates("args4j:args4j:2.33")],
+        {"jgit": [unpack_coordinates("args4j:args4j:2.37")]},
+        None,
+        None,
+    )
+
+    asserts.equals(env, ["2.37"], _versions_of(merged, "args4j", "args4j"))
+
+    return unittest.end(env)
+
+non_root_higher_version_evicts_root_artifact_test = unittest.make(_non_root_higher_version_evicts_root_artifact_impl)
+
+def _root_higher_version_drops_non_root_artifact_impl(ctx):
+    env = unittest.begin(ctx)
+
+    merged = deduplicate_artifacts_with_root_priority(
+        "external_deps",
+        [unpack_coordinates("args4j:args4j:2.37")],
+        {"jgit": [unpack_coordinates("args4j:args4j:2.33")]},
+        None,
+        None,
+    )
+
+    asserts.equals(env, ["2.37"], _versions_of(merged, "args4j", "args4j"))
+
+    return unittest.end(env)
+
+root_higher_version_drops_non_root_artifact_test = unittest.make(_root_higher_version_drops_non_root_artifact_impl)
+
+def _forced_root_version_beats_higher_non_root_impl(ctx):
+    env = unittest.begin(ctx)
+
+    merged = deduplicate_artifacts_with_root_priority(
+        "external_deps",
+        [_forced("args4j:args4j:2.33")],
+        {"jgit": [unpack_coordinates("args4j:args4j:2.37")]},
+        None,
+        None,
+    )
+
+    asserts.equals(env, ["2.33"], _versions_of(merged, "args4j", "args4j"))
+
+    return unittest.end(env)
+
+forced_root_version_beats_higher_non_root_test = unittest.make(_forced_root_version_beats_higher_non_root_impl)
+
+def _non_overlapping_artifacts_pass_through_impl(ctx):
+    env = unittest.begin(ctx)
+
+    merged = deduplicate_artifacts_with_root_priority(
+        "external_deps",
+        [unpack_coordinates("args4j:args4j:2.33")],
+        {"jgit": [unpack_coordinates("com.googlecode.javaewah:JavaEWAH:1.2.3")]},
+        None,
+        None,
+    )
+
+    asserts.equals(env, ["2.33"], _versions_of(merged, "args4j", "args4j"))
+    asserts.equals(env, ["1.2.3"], _versions_of(merged, "com.googlecode.javaewah", "JavaEWAH"))
+
+    return unittest.end(env)
+
+non_overlapping_artifacts_pass_through_test = unittest.make(_non_overlapping_artifacts_pass_through_impl)
+
+def layered_dedup_test_suite():
+    unittest.suite(
+        "layered_dedup_tests",
+        partial.make(non_root_higher_version_evicts_root_artifact_test, size = "small"),
+        partial.make(root_higher_version_drops_non_root_artifact_test, size = "small"),
+        partial.make(forced_root_version_beats_higher_non_root_test, size = "small"),
+        partial.make(non_overlapping_artifacts_pass_through_test, size = "small"),
+    )

tests/unit/version_conflict_policy_test.bzl

@@ -8,7 +8,7 @@ load("//private/lib:coordinates.bzl", "unpack_coordinates")
 def _pinned_policy_forces_versioned_root_artifacts_impl(ctx):
     env = unittest.begin(ctx)
 
-    for resolver in ["gradle", "maven"]:
+    for resolver in ["gradle", "maven", "coursier"]:
         versioned = unpack_coordinates("com.example:root:1.0")
         versionless = unpack_coordinates("com.example:managed-by-bom")
 
@@ -31,7 +31,7 @@ def _other_policies_leave_root_artifacts_unchanged_impl(ctx):
     artifact = unpack_coordinates("com.example:root:1.0")
 
     default_artifacts = apply_root_version_conflict_policy([artifact], "gradle", "default")
-    coursier_artifacts = apply_root_version_conflict_policy([artifact], "coursier", "pinned")
+    coursier_artifacts = apply_root_version_conflict_policy([artifact], "coursier", "default")
 
     asserts.false(env, hasattr(default_artifacts[0], "force_version"))
     asserts.false(env, hasattr(coursier_artifacts[0], "force_version"))