fix: apply remaining bzlmod layering review feedback from #1618 (#1629)
Filter testonly non-root artifacts before the conflicting-force check so
that declarations layering drops cannot fail the build. Report equal-version
force displacement with an RJE_VERBOSE-gated INFO diagnostic; the non-root
declaration previously replaced the root's silently. Rename the
addtional_artifact_message variable.
diff --git a/docs/bzlmod.md b/docs/bzlmod.md
index edce4bc..400c00c 100644
--- a/docs/bzlmod.md
+++ b/docs/bzlmod.md
@@ -196,7 +196,7 @@
the existing repository-level duplicate check, which warns or fails according to
`duplicate_version_warning`. Forcing is the exception: if any module, the root included, sets
`force_version` on the same coordinate at two different versions, layering will fail with an
-error message.
+error message. Non-root declarations marked `testonly` are dropped before this check.
The surviving declaration is chosen by these rules:
@@ -314,6 +314,17 @@
your root does not) do not produce this warning either; they are covered by the contribution
warning above instead.
+One equal-version case still reports itself. When a non-root module forces the same version that
+your root module declares, its declaration displaces the root's, and an `INFO` message is printed
+when `RJE_VERBOSE` is set:
+
+```
+INFO: For dependency 'com.google.protobuf:protobuf-java' the bazel_worker_java bazel dep forces version 3.25.5; its declaration replaces the root module's declaration of the same version.
+```
+
+**Remedy:** set `force_version = True` on the root declaration to keep the root module's
+declaration, or drop the root declaration if the non-root module's is what you want.
+
#### Which versions are reaching the repository?
When more than one version of the same dependency makes it into the repository, whether declared
diff --git a/private/lib/layering.bzl b/private/lib/layering.bzl
index f1c51fb..71079a8 100644
--- a/private/lib/layering.bzl
+++ b/private/lib/layering.bzl
@@ -78,13 +78,16 @@
coordinate_to_forced_artifact = {}
for bazel_dep_name in bazel_dep_to_non_root_artifacts:
module_coordinate_to_artifact = {}
- module_artifacts = bazel_dep_to_non_root_artifacts.get(bazel_dep_name, [])
+ module_artifacts = [
+ artifact
+ for artifact in bazel_dep_to_non_root_artifacts.get(bazel_dep_name, [])
+ if not getattr(artifact, "testonly", False)
+ ]
_fail_if_conflicting_forces(bazel_dep_name, module_artifacts)
for artifact in module_artifacts:
- if not getattr(artifact, "testonly", False):
- artifact_key = to_key(artifact)
- if _candidate_takes_precedence(module_coordinate_to_artifact.get(artifact_key), artifact):
- module_coordinate_to_artifact[artifact_key] = artifact
+ artifact_key = to_key(artifact)
+ if _candidate_takes_precedence(module_coordinate_to_artifact.get(artifact_key), artifact):
+ module_coordinate_to_artifact[artifact_key] = artifact
for artifact_key, artifact in module_coordinate_to_artifact.items():
if getattr(artifact, "force_version", False) and artifact_key not in root_forced_artifact_keys:
@@ -137,6 +140,7 @@
)
duplicate_artifact_warning = ""
+ forced_version_info = ""
filtered_root_artifacts = []
filtered_non_root_artifacts = []
for root_artifact in root_artifacts:
@@ -161,13 +165,18 @@
fail(message)
elif duplicate_version_warning == "warn":
duplicate_artifact_warning = duplicate_artifact_warning + "\nWARNING: " + message
+ elif non_root_forced:
+ forced_version_info = forced_version_info + (
+ "\nINFO: For dependency '%s:%s' the %s bazel dep forces version %s; " % (root_artifact.group, root_artifact.artifact, bazel_dep_name, non_root_artifact.version) +
+ "its declaration replaces the root module's declaration of the same version."
+ )
if keep_root_artifact:
filtered_root_artifacts.append(root_artifact)
# Add any remaining non root artifacts that weren't found in the root artifact list
- addtional_artifact_message = ""
+ additional_artifact_message = ""
for bazel_dep_name, non_root_artifact in non_root_coordinate_to_artifact.values():
- addtional_artifact_message = addtional_artifact_message + (
+ additional_artifact_message = additional_artifact_message + (
"\nINFO: The @%s repo is getting the additional artifact %s:%s:%s from the %s bazel dep." % (name, non_root_artifact.group, non_root_artifact.artifact, non_root_artifact.version, bazel_dep_name)
)
filtered_non_root_artifacts.append(non_root_artifact)
@@ -175,8 +184,10 @@
diagnostics = []
if duplicate_artifact_warning != "":
diagnostics.append(_diagnostic(duplicate_artifact_warning, "always"))
- if addtional_artifact_message != "":
- diagnostics.append(_diagnostic(addtional_artifact_message, "repin_verbose"))
+ if forced_version_info != "":
+ diagnostics.append(_diagnostic(forced_version_info, "verbose"))
+ if additional_artifact_message != "":
+ diagnostics.append(_diagnostic(additional_artifact_message, "repin_verbose"))
return struct(
artifacts = filtered_root_artifacts + filtered_non_root_artifacts,
diff --git a/tests/unit/layering_test.bzl b/tests/unit/layering_test.bzl
index 1abb461..597054f 100644
--- a/tests/unit/layering_test.bzl
+++ b/tests/unit/layering_test.bzl
@@ -195,7 +195,14 @@
asserts.equals(env, [non_root], result.artifacts)
asserts.true(env, result.artifacts[0].force_version)
- asserts.equals(env, [], result.diagnostics)
+ asserts.equals(
+ env,
+ [struct(
+ text = "\nINFO: For dependency 'com.example:library' the dep bazel dep forces version 1.0; its declaration replaces the root module's declaration of the same version.",
+ gate = "verbose",
+ )],
+ result.diagnostics,
+ )
return unittest.end(env)
@@ -259,6 +266,44 @@
testonly_nonroot_is_filtered_test = unittest.make(_testonly_nonroot_is_filtered_impl)
+def _testonly_force_conflicts_are_ignored_impl(ctx):
+ env = unittest.begin(ctx)
+ kept = _artifact("1.0", force_version = True)
+
+ # A testonly declaration is dropped, so its forced version cannot conflict
+ # with the surviving declaration's force.
+ asserts.equals(
+ env,
+ [kept],
+ deduplicate_non_root_artifacts(
+ {"dep": [kept, _artifact("2.0", force_version = True, testonly = True)]},
+ return_only_artifacts = True,
+ ),
+ )
+
+ return unittest.end(env)
+
+testonly_force_conflicts_are_ignored_test = unittest.make(_testonly_force_conflicts_are_ignored_impl)
+
+def _conflicting_testonly_forces_are_dropped_impl(ctx):
+ env = unittest.begin(ctx)
+
+ asserts.equals(
+ env,
+ [],
+ deduplicate_non_root_artifacts(
+ {"dep": [
+ _artifact("1.0", force_version = True, testonly = True),
+ _artifact("2.0", force_version = True, testonly = True),
+ ]},
+ return_only_artifacts = True,
+ ),
+ )
+
+ return unittest.end(env)
+
+conflicting_testonly_forces_are_dropped_test = unittest.make(_conflicting_testonly_forces_are_dropped_impl)
+
def _multiple_nonroot_highest_wins_impl(ctx):
env = unittest.begin(ctx)
higher = _artifact("2.0")
@@ -833,6 +878,8 @@
partial.make(root_force_beats_conflicting_nonroot_forces_test, size = "small"),
partial.make(single_nonroot_survives_test, size = "small"),
partial.make(testonly_nonroot_is_filtered_test, size = "small"),
+ partial.make(testonly_force_conflicts_are_ignored_test, size = "small"),
+ partial.make(conflicting_testonly_forces_are_dropped_test, size = "small"),
partial.make(multiple_nonroot_highest_wins_test, size = "small"),
partial.make(equal_version_tie_keeps_first_module_metadata_test, size = "small"),
partial.make(root_force_beats_higher_unforced_nonroot_test, size = "small"),
