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

rules_jvm_external 6.6 broken for maven installs #1290

Open
daniel-b2c2 opened this issue Nov 26, 2024 · 15 comments · May be fixed by #1311
Open

rules_jvm_external 6.6 broken for maven installs #1290

daniel-b2c2 opened this issue Nov 26, 2024 · 15 comments · May be fixed by #1311

Comments

@daniel-b2c2
Copy link

daniel-b2c2 commented Nov 26, 2024

Just upgraded to 6.6 of rules_jvm_external.

We have the following legitimate dependency in a maven.install() clause:

"com.google.apis:google-api-services-calendar:v3-rev20241101-2.0.0",
"com.google.apis:google-api-services-sheets:v4-rev20241008-2.0.0",

which now produces the following errors on jvm_rules_external 6.6 when running the REPIN=1 bazel run @maven//:pin command

ERROR: /buildout/external/rules_jvm_external~/private/rules/coursier.bzl:928:13: An error occurred during the fetch of repository 'rules_jvm_external~~maven~unpinned_maven':
   Traceback (most recent call last):
	File "/buildout/external/rules_jvm_external~/private/rules/coursier.bzl", line 1039, column 38, in _coursier_fetch_impl
		dep_tree = make_coursier_dep_tree(
	File "/buildout/external/rules_jvm_external~/private/rules/coursier.bzl", line 928, column 13, in make_coursier_dep_tree
		fail("Error while fetching artifact with coursier: " + exec_result.stderr)
Error in fail: Error while fetching artifact with coursier: OpenJDK 64-Bit Server VM warning: Options -Xverify:none and -noverify were deprecated in JDK 13 and will likely be removed in a future release.
Resolution error: Error downloading com.google.apis:google-api-services-sheets:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-sheets//google-api-services-sheets-.pom
Error downloading com.google.apis:google-api-services-calendar:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-calendar//google-api-services-calendar-.pom
ERROR: no such package '@@rules_jvm_external~~maven~unpinned_maven//': Error while fetching artifact with coursier: OpenJDK 64-Bit Server VM warning: Options -Xverify:none and -noverify were deprecated in JDK 13 and will likely be removed in a future release.
Resolution error: Error downloading com.google.apis:google-api-services-sheets:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-sheets//google-api-services-sheets-.pom
Error downloading com.google.apis:google-api-services-calendar:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-calendar//google-api-services-calendar-.pom
ERROR: /buildout/external/rules_jvm_external~~maven~maven/BUILD:15644:6: @@rules_jvm_external~~maven~maven//:pin depends on @@rules_jvm_external~~maven~unpinned_maven//:pin in repository @@rules_jvm_external~~maven~unpinned_maven which failed to fetch. no such package '@@rules_jvm_external~~maven~unpinned_maven//': Error while fetching artifact with coursier: OpenJDK 64-Bit Server VM warning: Options -Xverify:none and -noverify were deprecated in JDK 13 and will likely be removed in a future release.
Resolution error: Error downloading com.google.apis:google-api-services-sheets:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-sheets//google-api-services-sheets-.pom
Error downloading com.google.apis:google-api-services-calendar:
  not found: https://company.jfrog.io/artifactory/maven-virtual/com/google/apis/google-api-services-calendar//google-api-services-calendar-.pom
WARNING: errors encountered while analyzing target '@@rules_jvm_external~~maven~maven//:pin', it will not be built.
Analysis failed

Given the version of the dependencies has been truncated in the debug output, I wonder if there is a bug parsing the unusual format that google provides for its calendar and sheets library?

@shs96c
Copy link
Collaborator

shs96c commented Nov 28, 2024

In the function, if there are three parts to the coordinates, we treat the third part as a version number unless there's an @ sign in the coordinates. You should only be running into this if there's a coordinate which specifies a classifier or extension.

Presumably to recreate the issue it's enough to have one of these dependencies in the artifacts of a maven_install or install tag?

@shs96c
Copy link
Collaborator

shs96c commented Nov 28, 2024

The function mentions that version numbers are optional. This is the case where someone has specified a BOM, and then wants to specify the extension to use for an entry in the artifacts list of a maven_install. At that point, it becomes essentially impossible to determine which of the two coordinate variants we want to use, and we need to guess.

@shs96c
Copy link
Collaborator

shs96c commented Nov 28, 2024

Without a SemVer 1.0.0 compliant version number, Maven can't do version ordering, which suggests that people should be using that, but I don't think it's the place for rules_jvm_external to be telling people that.

@dmivankov
Copy link
Contributor

Don't see mentions of version being optional around Maven BOM docs https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#bill-of-materials-bom-poms
Logically BOM should be very specific so rules_jvm_external should probably prefer g:a:v over g:a:p (in case g:a:p is even actually possible to hit somewhere in the wild)

@shs96c
Copy link
Collaborator

shs96c commented Nov 28, 2024

Version numbers are optional in artifacts listed in BOMs, even with Maven. You'd end up with something like:

<dependencies>
  <dependency>
    <groupId>com.example</groupId>
    <artifactId>foo</artifactId</artifactId>
  </dependency>
</dependencies>

@daniel-b2c2
Copy link
Author

daniel-b2c2 commented Nov 28, 2024

Do I understand correctly:

We are trying to support BOM's and given that a BOM specifies the library versions for those that inherit it, it should not be necessary to specify the version in the maven.install() coordinates, correct? i.e. group:artifact should be enough?

Do I understand that the issue we're running into is:

User has provided group:artifact:package in the context of a BOM and it's impossible to tell this apart from group:artifact:version when version is non-semver?

I would argue the rareness of 2-arity + package means that if you see '3' co-ordinates it should be assumed to be g:a:v, and if somebody wants to specify a package while omitting the version, then they just cannot expect to reassign meaning to the v token

Instead from a design point of view you would expect to either:

  • use g:a:v:p (which defeats the purpose of the bom)
  • or g:a::p if you must not specify the version (empty version)
  • or for readability, if there were a reserved character which cannot be a version by itself e.g. g:a:?:p or g:a:_:p or g:a:*:p
  • or one that actually works today, use the full expanded format below
maven.artifact(
            group = "com.google.guava",
            artifact = "guava",
            packaging = "jar",
        )

The key with these examples is that it ensure that each index of each element always retaining the same meaning regardless of context or value.

my two cents...

@shs96c
Copy link
Collaborator

shs96c commented Dec 2, 2024

We are trying to support BOM's and given that a BOM specifies the library versions for those that inherit it, it should not be necessary to specify the version in the maven.install() coordinates, correct? i.e. group:artifact should be enough?

Correct

User has provided group:artifact:package in the context of a BOM and it's impossible to tell this apart from group:artifact:version when version is non-semver?

Essentially, this is what I'm saying. Using g:a::p is probably the best solution. I'll put a PR together to support this.

@shs96c
Copy link
Collaborator

shs96c commented Dec 2, 2024

Apologies. The format for the existing style is group:artifact[:packaging[:classifier]]:version, so g:a:p: is the correct form, and that is already supported.

@dmivankov
Copy link
Contributor

There seems to be a mix of string formats

Maven doesn't seem to prescribe anything except for g:a:v for string representation whereas in pom.xml any combination is possible: group, artifact, version (optional if defined in dependencyManagement, or in scope of exclusions or like), type (same as packaging?), classifier, scope (defines how artifact is used but not which (sub)artifact)

It would be nice to map out where string versions are expected to be used and then go from there, ideally only keeping strings on the surface. Currently is seems some of the extra attributes have inconsistent levels support (different naming, being or not being included in generated pom, support for specifying at all, etc)

@peakschris
Copy link

I'm also seeing a regression here in 6.6 vs 6.5:

maven.install(
artifacts = [
...
"me.friwi:jcef-api:jcef-7f53d6d+cef-100.0.14+g4e5ba66+chromium-100.0.4896.75",
"com.company:thing1:VX.2.5.0.0",
"com.company:thing2:VX.2.5.0.0",
....
],
)

In all the cases, the version numbers have alphanumeric characters in them.

For now, we are sticking with 6.5

@daniel-b2c2
Copy link
Author

daniel-b2c2 commented Jan 1, 2025

@shs96c

g:a:p:

Just wondering is the plan to roll forward with the g:a:p: format?

@dmivankov

g:a:v[:c][@t] https://github.com/bazel-contrib/rules_jvm_external/blob/6.5/private/lib/coordinates.bzl#L84

Isn't this an 'export' format to match 'gradle' or some such? So it's not part of the possible inputs, only an output? Correct me if I misunderstood this case.

@dmivankov
Copy link
Contributor

@dmivankov

g:a:v[:c][@t] https://github.com/bazel-contrib/rules_jvm_external/blob/6.5/private/lib/coordinates.bzl#L84

Isn't this an 'export' format to match 'gradle' or some such? So it's not part of the possible inputs, only an output? Correct me if I misunderstood this case.

that one might well be only used for export, I was trying to find all possible formats mentioned in the repo

@honnix
Copy link
Contributor

honnix commented Jan 16, 2025

Any progress on this issue? What would be a suggestion on user side to work around this? Switching to maven.artifact instead?

@shs96c
Copy link
Collaborator

shs96c commented Jan 16, 2025

Sorry. I've been sick or busy with work since the start of the year. I'll try and make some time for this today or tomorrow.

@shs96c shs96c linked a pull request Jan 16, 2025 that will close this issue
@shs96c
Copy link
Collaborator

shs96c commented Jan 16, 2025

I've prepped #1311 to try and address this issue.

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 a pull request may close this issue.

5 participants