Fix pom generation for classifier artifacts #1208
Conversation
vinnybod
requested review from
jin,
Wyverald,
shs96c and
cheister
as code owners
|
I am seeing that this is failing the bom generation tests. WIll take a look. |
private/rules/maven_utils.bzl
Outdated
| split = dep.split(":") | ||
| if len(split) == 5: | ||
| gradle_format = split[0] + ":" + split[1] + ":" + split[4] + ":" + split[3] + "@" + split[2] | ||
| print(st) | ||
| unpacked = _unpack_coordinates(st) |
There was a problem hiding this comment.
This probably needs to be more robust?
|
It looks like the test failures are genuine. |
| @@ -85,13 +92,20 @@ def generate_pom( | |||
| deps = [] | |||
| for dep in sorted(versioned_dep_coordinates) + sorted(unversioned_dep_coordinates): | |||
| include_version = dep in versioned_dep_coordinates | |||
| unpacked = _unpack_coordinates(dep) | |||
| split = dep.split(":") | |||
| if len(split) == 5 and split[3] not in ["import", "compile", "runtime", "test", "provided", "system"]: | |||
There was a problem hiding this comment.
What case is being handled here that we don't already handle in _unpack_coordinates? If it's because you'd like to be able to use gradle-style coordinates in your maven_install artifacts, there's already a heuristic in place to differentiate from the current style and the gradle style
There was a problem hiding this comment.
I had to convert it to the gradle format for it to be parsed correctly. If I just pass it in as-is, then unpack_coordinates is putting the classifier into the <scope> tag.
So maybe this hardcoded check for "import", "compile", "runtime", "test", "provided", "system" should go into the unpack_coordinates?
I feel like I might be missing something obvious if you are saying the heuristic check is already in place.
| unpacked = _unpack_coordinates(dep) | ||
| split = dep.split(":") | ||
| if len(split) == 5 and split[3] not in ["import", "compile", "runtime", "test", "provided", "system"]: | ||
| print(split) |
| @@ -85,13 +92,20 @@ def generate_pom( | |||
| deps = [] | |||
| for dep in sorted(versioned_dep_coordinates) + sorted(unversioned_dep_coordinates): | |||
| include_version = dep in versioned_dep_coordinates | |||
| unpacked = _unpack_coordinates(dep) | |||
| split = dep.split(":") | |||
| if len(split) == 5 and split[3] not in ["import", "compile", "runtime", "test", "provided", "system"]: | |||
There was a problem hiding this comment.
What case is being handled here that we don't already handle in _unpack_coordinates? If it's because you'd like to be able to use gradle-style coordinates in your maven_install artifacts, there's already a heuristic in place to differentiate from the current style and the gradle style
| @@ -1,6 +1,6 @@ | |||
| load("@rules_java//java:defs.bzl", "JavaInfo") | |||
| load(":has_maven_deps.bzl", "MavenInfo", "calculate_artifact_jars", "has_maven_deps") | |||
| load(":maven_utils.bzl", "determine_additional_dependencies", "generate_pom") | |||
| load(":maven_utils.bzl", "determine_additional_dependencies", "generate_pom", "unpack_coordinates") | |||
There was a problem hiding this comment.
This appears to be used. Could you please run bazel run scripts:format?
This fixes #1154
It seems like a bug that scope was being considered in the maven coordinates. I couldn't find any examples of that being the case. But it is common for
group:artifact:type:classifier:version.Demo of the issue: vinnybod/bazel_java_example@classifier-pom
Because bom generation was the only thing depending on sending the scope as part of the GAV, I refactored
maven_utils.generate_pomsoversioned_dep_coordinatesandunversioned_dep_coordinatesare expected to already beunpackedand_maven_dependencies_bomsets the type/scope itself. I also added tests.