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 d64709a7a..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", @@ -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,7 +760,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 +768,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/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/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"), + ) 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"))