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

Extra maven artifacts support for java_export rule #805

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krisnaru
Copy link

@krisnaru krisnaru commented Dec 10, 2022

Issue:
Java_export needs to upload multiple artifacts as part of single rule. Artifacts could be war/tar files and it needs to support classifier.

Example:
BUILD:
java_export(
name = "example-export",
extra_artifacts = {
"release": "//src/main/java/com/github/bazelbuild/rulesjvmexternal/example/export:tar",
},
maven_coordinates = "com.example:bazel-example:0.0.1",
runtime_deps = [
"//src/main/java/com/github/bazelbuild/rulesjvmexternal/example/export",
],
)

bazel run -define "maven_repo=file://$HOME/.m2/repository" :example-export.publish

@krisnaru
Copy link
Author

@cheister I have made one more commit towards templating maven coordinates. Could you please check?

@krisnaru krisnaru changed the title Expand the maven coordinates template from variables Extra maven artifacts support for java_export rule Dec 14, 2022
@krisnaru krisnaru force-pushed the master branch 2 times, most recently from 6620973 to 006f6ff Compare December 14, 2022 22:03
@krisnaru krisnaru force-pushed the master branch 2 times, most recently from f2c9503 to f7bb009 Compare December 17, 2022 19:59
@krisnaru
Copy link
Author

@cheister could you please review and approve if you are fine with changes

@krisnaru
Copy link
Author

krisnaru commented Jan 4, 2023

@cheister could you please review and approve if you are fine with changes

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.

My apologies it has taken so long to review this. A couple of comments, but I think this is looking good.

private/rules/maven_publish.bzl Outdated Show resolved Hide resolved
private/rules/maven_publish.bzl Outdated Show resolved Hide resolved
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.

This is almost there. I've spotted one issue, but once that's fixed, let's merge this.

@@ -106,6 +111,17 @@ public static void main(String[] args)
futures.add(upload(repo, credentials, coords, "-javadoc.jar", docJar, gpgSign));
}

if(args.length > 9) {
String ext = com.google.common.io.Files.getFileExtension(args[9]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this value need to be calculated per tuple, rather than just on the final one? eg war=foo.jar,tar=foo.tar would result in ext being set to tar for both the war and tar classifiers.

for(String artifactTuple: extraArtifactTuples) {
String[] splits = artifactTuple.split("=");
String classifier = splits[0];
Path artifact = getPathIfSet(splits[1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Path should exist. If it doesn't, we've got real problems.... Paths.get is fine here.

@shs96c
Copy link
Collaborator

shs96c commented Mar 7, 2023

@krisnaru, can I help you in any way to get this one over the line? It'd be lovely to have this in the tree....

@thirtyseven
Copy link
Contributor

I have continued the work in this PR in #926 .

@cjohnstoniv
Copy link

Is there any intention/support for extra classifier artifacts being in the BOM too? Would be useful for things like test-jars that sometime get published and depended on.

@thirtyseven
Copy link
Contributor

Is there any intention/support for extra classifier artifacts being in the BOM too? Would be useful for things like test-jars that sometime get published and depended on.

@cjohnstoniv We didn't need that for our use team's use case, but I'm sure it would make sense to extend this to the BOM if you want to contribute a PR.

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.

4 participants