-
-
Notifications
You must be signed in to change notification settings - Fork 256
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 google cloud storage support to java_export #463
Conversation
d32efeb
to
3bccb78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's in good shape. Thank you for writing it!
@jin can weigh in on the request to avoid asking people to add extra steps to their workspaces. Is there any way we can import them into the repo itself?
It'd be nice to have some tests, but I guess it's hard to add those for publishing.
If credentials are needed, how are they detected and specified?
README.md
Outdated
@@ -952,6 +960,10 @@ bazel run --stamp \ | |||
//user_project:exported_lib.publish` | |||
``` | |||
|
|||
It's also possible to publish to a Google Cloud Storage bucket (make sure to not add a trailing slash): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could we just remove a trailing slash if we see one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about it, but I didn't want to mess with the users args, in case s/he really want the extra slash from some reason.
Now that I think about maybe it doesn't really make sense, will add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
defs.bzl
Outdated
""" | ||
Adds the maven dependencies that are needed to run java_export rule. | ||
""" | ||
maven_install( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use a jvm_import
to pull in a jar we add to the repo? Other than the vast number of dependencies?
The current way of using rules_jvm_external
doesn't need anyone to go through the cumbersome "import the ruleset, load the rule set deps, run the deps setup" dance. That's why the current deployer doesn't use anything outside the Java 8 JRE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm wrong, but AFAIK bazel doesn't support transitive deps, so the only real way to avoid forcing the user from adding those deps is to save the jars in our repo, which isn't a solution IMHO.
Am I wrong about it? Even with jvm_import it will include target from different repo, which the user will need to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's just a single (or few) jar dependency, we could vendor it.
How about doing it this way instead of requiring a load statement? https://github.com/bazelbuild/rules_jvm_external#exporting-and-consuming-artifacts-from-external-repositories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GCS lib has 23 deps (so 24 total jars), we can vendor them but I'm not sure it's the right approach.
LMK what you think about it, if you prefer it to be vendored I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shs96c and I had a discussion offline about this, and I think that it's would be a cleaner approach to go with your implementation, with an internal maven_install
repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Right now I'm using the application default, since it's GCS best practice. |
3bccb78
to
5eec5f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please also get approval from @shs96c.
defs.bzl
Outdated
@@ -154,6 +154,22 @@ def _parse_artifact_str(artifact_str): | |||
else: | |||
return parse.parse_maven_coordinate(artifact_str) | |||
|
|||
JAVA_EXPORT_REPO_NAME = "internal_java_exports_maven" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix with _INTERNAL
to make it clear that this is not meant for consumption by end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to INTERNAL_JAVA_EXPORT_REPO_NAME
since vars starts with underscore are internal and can't be imported in the build file.
7bc2bf5
to
a601d56
Compare
Sorry for the slow feedback on this. After discussing with @jin, I think the best thing to do to avoid vendoring in all the jars we need is to move |
After a far too long, I've created #481 to help make it easier for us to pull in the bits that we need to rules_jvm_external |
I've landed #481 so it would be helpful if you could rebase on top of that, and use those features to pull in any dependencies, please. |
a601d56
to
26a6681
Compare
@shs96c Done, LMK what you think, it's a much smaller change now, most of it is the maven_pin file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you patience while we got the infrastructure in to support this, and thank you again for the patch. LGTM!
Closes #458 (this issue is for publishing only, I would probably open another one to add GCS support to maven_install, hopefully I'll get to it).
@shs96c LMK what you think.