Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for pom generation #1208

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

vinnybod
Copy link
Contributor

@vinnybod vinnybod commented Jul 30, 2024

This adds tests for various pom generation scenarios.


Original context:
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_pom so versioned_dep_coordinates and unversioned_dep_coordinates are expected to already be unpacked and _maven_dependencies_bom sets the type/scope itself. I also added tests.

@vinnybod
Copy link
Contributor Author

I am seeing that this is failing the bom generation tests. WIll take a look.

@vinnybod vinnybod marked this pull request as draft July 30, 2024 22:07
@vinnybod vinnybod marked this pull request as ready for review July 30, 2024 23:05
Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to unpack_coordinates is a breaking change for our users. I've tried to suggest something we could try to meet all our needs.

private/rules/maven_utils.bzl Outdated Show resolved Hide resolved
private/lib/coordinates.bzl Outdated Show resolved Hide resolved
@shs96c
Copy link
Collaborator

shs96c commented Sep 17, 2024

It looks like the test failures are genuine.

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're almost there :)

private/rules/maven_utils.bzl Outdated Show resolved Hide resolved
private/rules/maven_utils.bzl Outdated Show resolved Hide resolved
private/rules/pom_file.bzl Outdated Show resolved Hide resolved
@vinnybod vinnybod changed the title Fix pom generation for classifier artifacts Add tests for pom generation Nov 21, 2024
@vinnybod
Copy link
Contributor Author

@shs96c It looks like #1279 fixes my original complaint 😄 :

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.

I merged master into this branch and now all it does is add some new tests.

@vinnybod vinnybod requested a review from shs96c November 21, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java_export generated pom.xml does not include <classifier> property
2 participants