From d6b4c97849452f6b3ef293e64b003f0538ab885a Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 25 Jul 2026 09:11:25 +0200 Subject: [PATCH 1/2] bzlmod: evict losing root artifact in layered dedup Addresses #1549. deduplicate_artifacts_with_root_priority() resolves a duplicate group:artifact between the root module and a contributing module by popping the non-root artifact and, when the root does not force its version, selecting the highest version. The selection was one-sided: when the non-root version won, it was appended to the result, but the losing root artifact was never removed, so both versions survived into the merged artifact list. Installs with duplicate_version_warning = "error" then fail early in _check_artifacts_are_unique(), before any conflict policy can act: Found duplicate artifact versions args4j:args4j has multiple versions 2.33, 2.37 The other two branches (root forces, or the root version is highest) already drop the popped non-root artifact, so only this branch could ship duplicates. Rebuild the returned list without root artifacts that lost the version comparison, so exactly one version per coordinate reaches the resolver regardless of which side won. The function loses its leading underscore so the new regression tests can load it. --- private/extensions/maven.bzl | 18 ++++-- tests/unit/BUILD | 3 + tests/unit/layered_dedup_test.bzl | 103 ++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 tests/unit/layered_dedup_test.bzl diff --git a/private/extensions/maven.bzl b/private/extensions/maven.bzl index d64709a7a..f3b3e89ec 100644 --- a/private/extensions/maven.bzl +++ b/private/extensions/maven.bzl @@ -242,11 +242,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 +263,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 +287,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"]: @@ -739,7 +749,7 @@ 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, @@ -747,7 +757,7 @@ def maven_impl(mctx): 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, diff --git a/tests/unit/BUILD b/tests/unit/BUILD index f50a066d6..2503108e8 100644 --- a/tests/unit/BUILD +++ b/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() diff --git a/tests/unit/layered_dedup_test.bzl b/tests/unit/layered_dedup_test.bzl new file mode 100644 index 000000000..2c592257a --- /dev/null +++ b/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"), + ) From 7a32745e285148245544f160eafae70c9812d916 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 25 Jul 2026 09:12:01 +0200 Subject: [PATCH 2/2] bzlmod: apply pinned root precedence to the coursier resolver Addresses #1549. apply_root_version_conflict_policy() implements the documented semantics of version_conflict_policy = "pinned" -- user-specified versions are chosen unconditionally over transitive ones -- by treating root-declared artifacts as force_version = True during layered dependency merging. It was gated to the gradle and maven resolvers, leaving coursier, the default resolver, on the old behaviour: a contributing module's higher version outranks (or, with duplicate_version_warning = "error", collides with) the root module's deliberately pinned version. The gate is also internally inconsistent: under "pinned" the coursier code path already passes --force-version for every declared artifact at resolution time. Only the bzlmod merge layer ignored the policy, so the root-vs-contributor conflict was decided (or failed) before coursier's own pinned handling could run. Lift the gate: pinned now forces root-declared versions for every resolver. A root module application depending on libraries that contribute their own Maven dependencies via bzlmod layering (for example Gerrit with JGit contributing args4j) no longer needs to enumerate every overlapping coordinate with maven.amend_artifact(force_version = ...). Pinned coursier installs pick up force_version on their root artifacts, which changes lockfile input hashes; a one-time REPIN refreshes them. The two affected in-repo fixtures (policy_pinned_testing, coursier_resolved_with_boms) are repinned here, migrating them to the current lock format on the way; their resolved dependency versions are unchanged. --- docs/bzlmod-api.md | 2 +- private/extensions/maven.bzl | 19 ++++++-- .../coursier_resolved_install.json | 12 +---- .../policy_pinned_testing_install.json | 47 +++++++++++++++++-- tests/unit/version_conflict_policy_test.bzl | 4 +- 5 files changed, 63 insertions(+), 21 deletions(-) diff --git a/docs/bzlmod-api.md b/docs/bzlmod-api.md index 38e747474..dd8340c54 100644 --- a/docs/bzlmod-api.md +++ b/docs/bzlmod-api.md @@ -114,7 +114,7 @@ Combines artifact and bom declarations with setting the location of lock files t | strict_visibility_value | - | List of labels | optional | `["@rules_jvm_external//visibility:private"]` | | use_credentials_from_home_netrc_file | Whether to pass machine login credentials from the ~/.netrc file to coursier. | Boolean | optional | `False` | | use_starlark_android_rules | Whether to use the native or Starlark version of the Android rules. | Boolean | optional | `False` | -| version_conflict_policy | 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 "default", follow the selected resolver's default policy. | String | optional | `"default"` | +| version_conflict_policy | Policy for user-defined vs. transitive dependency version conflicts

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"` | diff --git a/private/extensions/maven.bzl b/private/extensions/maven.bzl index f3b3e89ec..59e28608d 100644 --- a/private/extensions/maven.bzl +++ b/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", @@ -641,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": diff --git a/tests/custom_maven_install/coursier_resolved_install.json b/tests/custom_maven_install/coursier_resolved_install.json index 5ce89d44b..7954d0843 100644 --- a/tests/custom_maven_install/coursier_resolved_install.json +++ b/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", diff --git a/tests/custom_maven_install/policy_pinned_testing_install.json b/tests/custom_maven_install/policy_pinned_testing_install.json index 85cf17fb7..f3a1b139b 100644 --- a/tests/custom_maven_install/policy_pinned_testing_install.json +++ b/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" } diff --git a/tests/unit/version_conflict_policy_test.bzl b/tests/unit/version_conflict_policy_test.bzl index b4210a90d..ec2386e61 100644 --- a/tests/unit/version_conflict_policy_test.bzl +++ b/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"))