-
Notifications
You must be signed in to change notification settings - Fork 568
[email protected] #6087
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
[email protected] #6087
Conversation
@bazel-io skip_check unstable_url |
Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (org_objectweb_asm) have been updated in this PR. |
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.
Code Review
This pull request adds the org_objectweb_asm
module at version 9.8. The changes introduce several critical issues in the BUILD files and presubmit configuration that will prevent the module from building correctly. The source globs in the java_library
rules are incorrect for most of the sub-modules, and the target labels in presubmit.yml
are not valid for presubmit jobs. These issues need to be fixed.
looks like the organization is just called "OW2" (https://www.ow2.org/). What do you think about |
OW2 appears to be some sort of incubator and/or code hosting non-profit. The library has a Maven ID How about |
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.
digging a bit more, it appears that OW2 (founded in 2007) is sort of the "spiritual successor" to ObjectWeb (founded in 2002; see https://en.wikipedia.org/wiki/OW2). The Wikipedia article named "ObjectWeb ASM" is long overdue a rename IMO (see this diff -- https://en.wikipedia.org/w/index.php?title=ObjectWeb_ASM&diff=prev&oldid=392803989); I'd guess that they also just never bothered to update the Java package name. All websites now refer to "OW2", though.
And that concludes my study on this topic. Final decision is up to you :) either ow2_asm
or objectweb_asm
seem better than org_objectweb_asm
.
377786c
to
5c0de54
Compare
Require module maintainers' approval for newly pushed changes.
Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (objectweb_asm) have been updated in this PR. |
OK, updated to use the package name |
@@ -0,0 +1,13 @@ | |||
{ | |||
"url": "https://gitlab.ow2.org/asm/asm/-/archive/ASM_9_8/asm-ASM_9_8.tar.gz", |
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.
As far as I remember the archives generated by gitlab's archive endpoint change regularly depending on which CDN node you hit, which makes them unsuitable as BCR sources.
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.
Does that also apply to non-gitlab.com
instances?
If this URL won't work for BCR, and since mirroring it on the Bazel archive mirror also isn't allowed by BCR rules, how.would you recommend I proceed? Other options:
- I copy this archive to my personal GitHub,
- I use Maven-hosted source archives, resulting in six BCR packages for this project (one per build target)
- Ask upstream for non-Maven source archive hosting
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 don't know. We've had reliability issues with ARM's instance, but no checksum issues as far as I remember.
We are working on setting up an automated mirror which should make this much easier. You could also open a manual mirror request issue on the Bazel repo.
We should probably also formulate a clear vision/policy for simple Java modules in the BCR. If we add just a few, we may be risking one version violations if folks also consume Maven artifacts, perhaps unknowingly as transitive deps.
If we plan to add a significant portion of what's available on Maven, it would probably be better to start up a separate Java registry. In fact, such a registry could potentially be smart instead of static, fetching source archives from Maven, generating BUILD files based on the pom.xml on the fly and persistently caching the result so that it's guaranteed to be immutable. This could be a pretty efficient way to get Java source builds to work for real projects.
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.
My understanding is that the Bazel mirror can't be used for BCR source archives, see #2821 and the discussion it links to (cc @meteorcloudy).
If we can assume that gitlab.ow2.org
doesn't have the same CDN-related reproducibility issues as gitlab.com
, then it doesn't seem any worse than other sources of dynamically generated tarballs. Obviously a proper stable source archive would be better, but if that isn't available then this seems like the next best thing.
A project to automatically generate a BCR-compatible package registry from language-specific upstream hosting (e.g. Maven) sounds like it would have a lot of interesting work involved but I think I'd prefer a more conventional approach, even if the per-package toil is slightly higher.
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.
In fact, such a registry could potentially be smart instead of static, fetching source archives from Maven, generating BUILD files based on the pom.xml on the fly and persistently caching the result
Isn't that rules_jvm_external? @jmillikin Why do we need this dependency as a bazel module?
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.
OK, let me add some context here:
Bazel already depends on rules_jvm_external to pull in most of its java dependencies as prebuilt jars, this is the standard integration with Maven in Bazel. It offers two advantages: 1) No need to hand craft BUILD files 2) automatically resolve transitive dependencies (you don't need to worry about fetching asm when using chicory compiler). Of course, the con is that those are not built from sources, like @fmeum said, maybe we could work on rules_jvm_external to make that possible.
But at this point, I don't want to add another way to introduce java dependencies in Bazel. Previously I thought a Bazel module for Chicory was fine because it had no other java dependencies. Now that we do need it, I don't think this is a rabbit hole we should go into (what if there are more in future, we already had to patch chicory). If we do this, 1) there are extra maintenance burden for the community, 2) Such dependencies could be duplicated (asm already introduced via rules_jvm_external) and end up in the Bazel Java server deploy java, and may cause issues if there are version confilcts.
My suggestion is to use rules_jvm_external for all Java dependencies, and hopefully we could add build from source mode in rules_jvm_external and I would be more than happy to switch to 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.
Here is the PR bazelbuild/bazel#27186, you should be able to add "com.dylibso.chicory:compiler:1.5.2"
pretty easily
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'll look into what it takes to add source build capabilities to RJE. Avoiding one version violations is the primary reason for having a single source of truth, which in the case of Java has undisputably become RJE. Since it resolves source jars alongside binary jars, it would even allow you to gradually migrate over.
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 updating the Bazel build to match the new expectations around Java dependencies.
Should I keep this PR open, or close it?
If Bazel projects should use rules_java_external
instead of bazel_dep
for Java dependencies, should I consider existing Java BCR packages (e.g. chicory
, javacc
, javaparser
) to be effectively orphaned? If so, I can send a follow-up PR to clear their maintainers lists.
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 so, I can send a follow-up PR to clear their maintainers lists.
Yeah, I think that makes sense to me.
Context: bazelbuild/bazel-central-registry#6087 (comment) Closes #27186. PiperOrigin-RevId: 817099287 Change-Id: I37002c7899a75368c7dd4b025d27eff127df8ad9
ObjectWeb ASM (https://asm.ow2.io/) is a library for manipulating Java bytecode.