From a1d4e4f4267c1797b686719aa385e707b732c541 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 17 Jan 2025 19:48:33 +0100 Subject: [PATCH] Fix collection of Maven runtime deps (#1308) A transitive dependency of a `maven_project_jar` is a runtime dependency if and only if each path to it from the root goes through at least one `runtime_deps` edge. Before this commit, every transitive dependency that was reachable from the root via at least one path with a `runtime_deps` edge was considered to be a runtime dependency, even if other paths require it as a compile dependency. Also show a helpful error when users attempt to move a dep from `runtime_deps` to `deps` on a `java_export` target with no `srcs`. --- .bazelci/presubmit.yml | 3 +- private/lib/coordinates.bzl | 2 + private/rules/has_maven_deps.bzl | 38 +++-- private/rules/java_export.bzl | 6 + private/rules/maven_bom.bzl | 6 +- private/rules/maven_utils.bzl | 8 +- private/rules/pom_file.bzl | 16 +- tests/unit/runtime_scope/BUILD | 3 + tests/unit/runtime_scope/Foo.java | 0 .../unit/runtime_scope/runtime_scope_test.bzl | 146 ++++++++++++++++++ 10 files changed, 201 insertions(+), 27 deletions(-) create mode 100644 tests/unit/runtime_scope/BUILD create mode 100644 tests/unit/runtime_scope/Foo.java create mode 100644 tests/unit/runtime_scope/runtime_scope_test.bzl diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index 17e8e3aef..885487c48 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -52,7 +52,8 @@ tasks: - "//..." ubuntu2204_latest: platform: ubuntu2204 - bazel: latest + # TODO: Revert to latest after fixing incompatibilities with Bazel 8.0.0 + bazel: 7.x environment: # This tests custom cache locations. # https://github.com/bazelbuild/rules_jvm_external/pull/316 diff --git a/private/lib/coordinates.bzl b/private/lib/coordinates.bzl index cb86755fc..f76566722 100644 --- a/private/lib/coordinates.bzl +++ b/private/lib/coordinates.bzl @@ -11,6 +11,8 @@ def unpack_coordinates(coords): return None pieces = coords.split(":") + if len(pieces) < 2: + fail("Could not parse maven coordinate: %s" % coords) group = pieces[0] artifact = pieces[1] diff --git a/private/rules/has_maven_deps.bzl b/private/rules/has_maven_deps.bzl index 5d72e3d64..ba0196f67 100644 --- a/private/rules/has_maven_deps.bzl +++ b/private/rules/has_maven_deps.bzl @@ -4,9 +4,8 @@ MavenInfo = provider( fields = { # Fields to do with maven coordinates "coordinates": "Maven coordinates for the project, which may be None", - "maven_deps": "Depset of first-order maven dependencies", - "maven_runtime_deps": "Depset of first-order maven runtime dependencies", - "as_maven_dep": "Depset of this project if used as a maven dependency", + "maven_deps": "Depset of coordinates of all transitive maven dependencies", + "maven_compile_deps": "Depset of coordinates of all transitive maven dependencies with compile scope", # Fields used for generating artifacts "artifact_infos": "Depset of JavaInfo instances of targets to include in the maven artifact", @@ -29,7 +28,7 @@ to offer `tags` to be used to infer maven information. _EMPTY_INFO = MavenInfo( coordinates = None, maven_deps = depset(), - as_maven_dep = depset(), + maven_compile_deps = depset(), artifact_infos = depset(), dep_infos = depset(), label_to_javainfo = {}, @@ -39,7 +38,7 @@ _EMPTY_INFO = MavenInfo( _STOPPED_INFO = MavenInfo( coordinates = "STOPPED", maven_deps = depset(), - as_maven_dep = depset(), + maven_compile_deps = depset(), artifact_infos = depset(), dep_infos = depset(), label_to_javainfo = {}, @@ -109,7 +108,8 @@ def calculate_artifact_source_jars(maven_info): _gathered = provider( fields = [ "all_infos", - "runtime_infos", + "all_deps", + "compile_deps", "label_to_javainfo", "artifact_infos", "transitive_exports", @@ -122,20 +122,26 @@ def _extract_from(gathered, maven_info, dep, include_transitive_exports, is_runt gathered.all_infos.append(maven_info) - if is_runtime_dep: - gathered.runtime_infos.append(maven_info) - gathered.label_to_javainfo.update(maven_info.label_to_javainfo) if java_info: - if maven_info.coordinates == "STOPPED": + if maven_info.coordinates == _STOPPED_INFO.coordinates: pass - if maven_info.coordinates: + elif maven_info.coordinates: gathered.dep_infos.append(dep[JavaInfo]) + + own_coordinates = depset([maven_info.coordinates]) + gathered.all_deps.append(own_coordinates) + if not is_runtime_dep: + gathered.compile_deps.append(own_coordinates) else: gathered.artifact_infos.append(dep[JavaInfo]) if include_transitive_exports: gathered.transitive_exports.append(maven_info.transitive_exports) + gathered.all_deps.append(maven_info.maven_deps) + if not is_runtime_dep: + gathered.compile_deps.append(maven_info.maven_compile_deps) + def _has_maven_deps_impl(target, ctx): if not JavaInfo in target: return [_EMPTY_INFO] @@ -157,7 +163,8 @@ def _has_maven_deps_impl(target, ctx): gathered = _gathered( all_infos = [], - runtime_infos = [], + all_deps = [], + compile_deps = [], artifact_infos = [target[JavaInfo]], transitive_exports = [], dep_infos = [], @@ -181,8 +188,8 @@ def _has_maven_deps_impl(target, ctx): transitive_exports_from_deps = gathered.transitive_exports dep_infos = gathered.dep_infos label_to_javainfo = gathered.label_to_javainfo - maven_deps = depset(transitive = [i.as_maven_dep for i in all_infos]) - maven_runtime_deps = depset(transitive = [i.as_maven_dep for i in gathered.runtime_infos]) + maven_deps = depset(transitive = gathered.all_deps) + maven_compile_deps = depset(transitive = gathered.compile_deps) transitive_exports_from_exports = depset() if hasattr(ctx.rule.attr, "exports"): @@ -195,8 +202,7 @@ def _has_maven_deps_impl(target, ctx): info = MavenInfo( coordinates = coordinates, maven_deps = maven_deps, - maven_runtime_deps = maven_runtime_deps, - as_maven_dep = depset([coordinates]) if coordinates else maven_deps, + maven_compile_deps = maven_compile_deps, artifact_infos = depset(direct = artifact_infos), dep_infos = depset(direct = dep_infos, transitive = [i.dep_infos for i in all_infos]), label_to_javainfo = label_to_javainfo, diff --git a/private/rules/java_export.bzl b/private/rules/java_export.bzl index 94a433a1d..0f04a22ff 100644 --- a/private/rules/java_export.bzl +++ b/private/rules/java_export.bzl @@ -90,6 +90,12 @@ def java_export( doc_resources = kwargs.pop("doc_resources", []) toolchains = kwargs.pop("toolchains", []) + # java_library doesn't allow srcs without deps, but users may try to specify deps rather than + # runtime_deps on java_export to indicate that the generated POM should list the deps as compile + # deps. + if kwargs.get("deps") and not kwargs.get("srcs"): + fail("deps not allowed without srcs; move to runtime_deps (for 'runtime' scope in the generated POM) or exports (for 'compile' scope)") + # Construct the java_library we'll export from here. java_library( name = lib_name, diff --git a/private/rules/maven_bom.bzl b/private/rules/maven_bom.bzl index dc710833b..582256d0c 100644 --- a/private/rules/maven_bom.bzl +++ b/private/rules/maven_bom.bzl @@ -26,6 +26,7 @@ def _label(label_or_string): def _maven_bom_impl(ctx): fragments = [f[MavenBomFragmentInfo] for f in ctx.attr.fragments] + dep_coordinates = [f.coordinates for f in fragments] # Expand maven coordinates for any variables to be replaced. coordinates = ctx.expand_make_variables("coordinates", ctx.attr.maven_coordinates, {}) @@ -33,7 +34,8 @@ 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 = dep_coordinates, + versioned_compile_dep_coordinates = dep_coordinates, pom_template = ctx.file.pom_template, out_name = "%s.xml" % ctx.label.name, ) @@ -70,12 +72,14 @@ def _maven_dependencies_bom_impl(ctx): 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] + compile_deps = depset(transitive = [f.maven_info.maven_compile_deps for f in fragments]).to_list() unpacked = unpack_coordinates(ctx.attr.bom_coordinates) dependencies_bom = generate_pom( ctx, coordinates = ctx.attr.maven_coordinates, versioned_dep_coordinates = combined_deps + ["%s:%s:pom:%s" % (unpacked.group, unpacked.artifact, unpacked.version)], + versioned_compile_dep_coordinates = compile_deps, pom_template = ctx.file.pom_template, out_name = "%s.xml" % ctx.label.name, indent = 12, diff --git a/private/rules/maven_utils.bzl b/private/rules/maven_utils.bzl index 97f3df0b8..5c900b6a0 100644 --- a/private/rules/maven_utils.bzl +++ b/private/rules/maven_utils.bzl @@ -62,8 +62,12 @@ def generate_pom( parent = None, versioned_dep_coordinates = [], unversioned_dep_coordinates = [], - runtime_deps = [], + versioned_compile_dep_coordinates = [], indent = 8): + versioned_compile_dep_coordinates_set = { + k: None + for k in versioned_compile_dep_coordinates + } unpacked_coordinates = _unpack_coordinates(coordinates) substitutions = { "{groupId}": unpacked_coordinates.group, @@ -93,7 +97,7 @@ def generate_pom( include_version = dep in versioned_dep_coordinates unpacked = _unpack_coordinates(dep) - new_scope = "runtime" if dep in runtime_deps else "compile" + new_scope = "compile" if dep in versioned_compile_dep_coordinates_set else "runtime" if unpacked.packaging == "pom": new_scope = "import" diff --git a/private/rules/pom_file.bzl b/private/rules/pom_file.bzl index 9a1177d24..ff6d0139b 100644 --- a/private/rules/pom_file.bzl +++ b/private/rules/pom_file.bzl @@ -13,19 +13,21 @@ def _pom_file_impl(ctx): additional_deps = determine_additional_dependencies(artifact_jars, ctx.attr.additional_dependencies) all_maven_deps = info.maven_deps.to_list() - runtime_maven_deps = info.maven_runtime_deps.to_list() + compile_maven_deps = info.maven_compile_deps.to_list() for dep in additional_deps: - for coords in dep[MavenInfo].as_maven_dep.to_list(): - all_maven_deps.append(coords) + dep_info = dep[MavenInfo] + dep_coordinates = [dep_info.coordinates] if dep_info.coordinates else dep_info.as_maven_dep.to_list() + all_maven_deps.extend(dep_coordinates) + compile_maven_deps.extend(dep_coordinates) expanded_maven_deps = [ 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) - for coords in runtime_maven_deps + expanded_compile_deps = [ + ctx.expand_make_variables("maven_compile_deps", coords, ctx.var) + for coords in compile_maven_deps ] # Expand maven coordinates for any variables to be replaced. @@ -35,7 +37,7 @@ def _pom_file_impl(ctx): ctx, coordinates = coordinates, versioned_dep_coordinates = sorted(expanded_maven_deps), - runtime_deps = expanded_runtime_deps, + versioned_compile_dep_coordinates = expanded_compile_deps, pom_template = ctx.file.pom_template, out_name = "%s.xml" % ctx.label.name, ) diff --git a/tests/unit/runtime_scope/BUILD b/tests/unit/runtime_scope/BUILD new file mode 100644 index 000000000..9fe74b1d5 --- /dev/null +++ b/tests/unit/runtime_scope/BUILD @@ -0,0 +1,3 @@ +load("//tests/unit/runtime_scope:runtime_scope_test.bzl", "runtime_scope_tests") + +runtime_scope_tests(name = "runtime_scope_tests") diff --git a/tests/unit/runtime_scope/Foo.java b/tests/unit/runtime_scope/Foo.java new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/runtime_scope/runtime_scope_test.bzl b/tests/unit/runtime_scope/runtime_scope_test.bzl new file mode 100644 index 000000000..6494b62b2 --- /dev/null +++ b/tests/unit/runtime_scope/runtime_scope_test.bzl @@ -0,0 +1,146 @@ +"""Unit tests for java_export runtime_deps behavior.""" + +load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") +load("@rules_java//java:defs.bzl", "JavaInfo", "java_library") +load("//private/rules:has_maven_deps.bzl", "MavenInfo", "has_maven_deps") +load("//private/rules:maven_project_jar.bzl", "maven_project_jar") + +def _runtime_scope_test_impl(ctx): + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + + asserts.equals(env, [ + # keep sorted + "example:dep_leaf:1.0.0", + "example:exported_leaf:1.0.0", + "example:leaf:1.0.0", + "example:runtime_dep_and_dep_leaf:1.0.0", + "example:runtime_dep_leaf:1.0.0", + ], sorted(tut[MavenInfo].maven_deps.to_list())) + asserts.equals(env, [ + # keep sorted + "example:exported_leaf:1.0.0", + "example:leaf:1.0.0", + "example:runtime_dep_and_dep_leaf:1.0.0", + ], sorted(tut[MavenInfo].maven_compile_deps.to_list())) + + return analysistest.end(env) + +_runtime_scope_test = analysistest.make( + _runtime_scope_test_impl, + extra_target_under_test_aspects = [has_maven_deps], +) + +def runtime_scope_tests(name): + java_library( + name = "library_to_test", + deps = [ + ":is_dep_has_exports", + ":is_dep_has_runtime_deps", + ], + exports = [":is_export_has_deps"], + runtime_deps = [":is_runtime_dep_has_deps"], + srcs = ["Foo.java"], + ) + + java_library( + name = "is_dep_has_exports", + exports = [ + ":leaf", + ":runtime_dep_and_dep_leaf", + ":stop_propagation", + ], + srcs = ["Foo.java"], + ) + + java_library( + name = "is_dep_has_runtime_deps", + runtime_deps = [ + ":runtime_dep_leaf", + ":runtime_dep_and_dep_leaf", + ], + srcs = ["Foo.java"], + ) + + java_library( + name = "is_export_has_deps", + deps = [":exported_leaf"], + srcs = ["Foo.java"], + ) + + java_library( + name = "is_runtime_dep_has_deps", + deps = [":dep_leaf"], + srcs = ["Foo.java"], + ) + + java_library( + name = "leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:leaf:1.0.0", + ], + ) + + java_library( + name = "exported_leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:exported_leaf:1.0.0", + ], + ) + + java_library( + name = "dep_leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:dep_leaf:1.0.0", + ], + ) + + java_library( + name = "runtime_dep_leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:runtime_dep_leaf:1.0.0", + ], + ) + + java_library( + name = "runtime_dep_and_dep_leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:runtime_dep_and_dep_leaf:1.0.0", + ], + deps = [":transitive_maven_dep"], + ) + + java_library( + name = "stop_propagation", + srcs = ["Foo.java"], + tags = [ + "no-maven", + ], + deps = [":not_included_leaf"], + ) + + java_library( + name = "not_included_leaf", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:not_included_leaf:1.0.0", + ], + ) + + java_library( + name = "transitive_maven_dep", + srcs = ["Foo.java"], + tags = [ + "maven_coordinates=example:transitive_maven_dep:1.0.0", + ], + ) + + _runtime_scope_test( + name = name, + target_under_test = ":library_to_test", + )