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

Api version 2.1.2 issues #681

Closed
jansupol opened this issue Oct 16, 2018 · 26 comments
Closed

Api version 2.1.2 issues #681

jansupol opened this issue Oct 16, 2018 · 26 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jansupol
Copy link
Contributor

We hit a few issues with latest 2.1.2 API for which the API should be re-released. We still examine consequences of the issue, so the list is not full, yet

OSGI manifest:

o OSGi specifically requires Java SE 9: Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=9.0))". This is the main reason for which the API needs to be re-released. The Felix maven plugin automatically fills this, but it is possible to let him know Java SE 8 is enough.
o Bundle-Version: 2.1.99.b01. This should likely be 2.1.1.x
o Export-Package: version="2.1.99"
o Specification-Version: 2.1.99.01 - Should be 2.1
o Automatic-Module-Name: java.ws.rs - not needed when there is module.info already

o OSGI is tough one. For bing sure the OSGI is ok, I'd recommend at least trying to load it in GF. Ideally, the JAX-RS TCK should be run with the new API before it is released. Santiago or me can do that.

Dependency Scope:

o JAX-B - while for JDK 11 the dependency for maven is in provided scope, module-info has it transitive. In JDK 11 there is no JAX-B, which means the API would not work in standalone out of the box unless the user will specifies the dependency. While it can be by design, it differs from "transitive" in module-info.

Versions - The goal is to depend on jakarta prefixed API jars. While these were not released, yet, it would make sense to wait with the release after the dependencies are released:
o JAX-B - javax.xml.bind:jaxb-api
o javax.activation:javax.activation-api

Module name:

o As the discussion goes on it looks like "jakarta" should be in module name eventually

@mkarg
Copy link
Contributor

mkarg commented Oct 16, 2018

Feel free to provide a PR.

Regarding our own module name, this cannot be changed in 2.x as it would break backwards compatibility. Such a change can only be done in 3.x.

@mkarg mkarg added the bug Something isn't working label Oct 16, 2018
@mkarg mkarg added this to the 2.1.3 milestone Oct 16, 2018
@mkarg
Copy link
Contributor

mkarg commented Oct 17, 2018

Regarding the module name, we can stay with the current one, as the PMC today announced that this is just a recommendation. It would break backwards compatibility, so we can only change it in 3.x.

@chkal
Copy link
Contributor

chkal commented Oct 18, 2018

I think many of the OSGi problems are caused by the spec-version-maven-plugin. Is anyone familiar with the plugin? There seems to be no documentation and my understanding is that it basically automatically sets a few maven properties (like 2.1.99, 2.1.99.01, etc). I spent quite some time trying to configure it for JSR 371 but I'm still not sure how it actually works.

@mkarg
Copy link
Contributor

mkarg commented Oct 18, 2018

That is why I proposed to get rid of it. But the problem then is, that some people want to keep OSGi compatibility, so we have to provide that by hand or annoy them. So we either need an expert for that plugin (as Oracle did not publish, possibly not even write, a documentation), or we need an OSGi expert to create the needed meta information manually.

@chkal
Copy link
Contributor

chkal commented Oct 18, 2018

Agreed! It looks like the plugin basically enforces these rules. But I didn't check them in detail yet.

@spericas
Copy link
Contributor

@jansupol Do you of anyone that can help us configure the plugin correctly? Is there any correlation between the JDK version used to build the artifacts and the one shown in the manifest?

@mkarg
Copy link
Contributor

mkarg commented Oct 19, 2018

@chkal Those rules look like being Glassifish-specific (in the meaning of: specific to one particular product). As we strive to be vendor-independent, I think we should simply ignore them. So it might be best to simply remove that plugin and just add a static minimum OSGi manifest possible by hand. I hate all those strange special magic produced by fancy Maven plugins.

@mkarg
Copy link
Contributor

mkarg commented Oct 24, 2018

Guys, if nobody chimes up to fix that strange plugin, I will simply remove OSGi in master and EE4J_8 as the JAX-RS charter does not imply it and apparently the wrong config is a showstopper (if it is not, I wonder what this issue is all about). So if anybody needs OSGi, speak up now and provide a different solution.

@spericas
Copy link
Contributor

@mkarg -1 This is likely important for EE4J, even if not for JAX-RS directly. Let's wait a bit more.

@arjantijms
Copy link
Contributor

-1 from me as well. This is almost certainly important for the EE4J project, and this implementation of the JAX-RS API is still a part of Eclipse-EE4J of course.

@jansupol
Copy link
Contributor Author

For JAX-RS API it is crucial to be an OSGi bundle, otherwise, JAX-RS can never be used in glassfish. Glassfish is using OSGi to load modules/jars. The argument that JAX-RS is a vendor-independent project and does not need OSGi is not strong enough, Glassfish is too important customer to scratch OSGi, just because some issues were experienced with Felix plugin.

JAX-RS is not alone in this. Every other API needs to be an OSGi bundle. That means, every other API repository is an example how to use the Felix plugin. I can try to copy/paste the Felix plugin settings information and create PR. I understand that everyone would like to release the API asap, but again, it make sense to wait until the dependencies used by JAX-RS API are released from EF. I understand there was a weak deadline, we may cross, but my understanding is that the deadline is for the release pipeline to be functional and ready, not for the actual release.

@mkarg
Copy link
Contributor

mkarg commented Oct 24, 2018

No need to argue. What I asked for was not an argument why I should not remove it, but a solution created by those people that want to keep it. Even if I do not remove it, it still is not working. So who volunteers on this?

@chkal
Copy link
Contributor

chkal commented Oct 26, 2018

I'm also +1 for keeping the OSGi manifest. However, I'm still not very happy about the fact that the OSGi manifest entries are magically created from some plugin without any documentation. I spent many hours trying to set up this plugin for JSR 371 and still don't fully understand how this plugin is supposed to work. So what about simply moving the relevant configuration for the OSGi manifest to Maven properties? IMO this would dramatically simplify the OSGi part of the build.

@mkarg
Copy link
Contributor

mkarg commented Oct 26, 2018

@chkal Manually these properties means that everybody must have an understand of the sense of these properties, i. e. the sense of OSGi. I have no clue of OSGi and have no use of that. So someone of the OSGi lovers has to maintain that.

@chkal
Copy link
Contributor

chkal commented Nov 2, 2018

@jansupol

JAX-B - while for JDK 11 the dependency for maven is in provided scope, module-info has it transitive. In JDK 11 there is no JAX-B, which means the API would not work in standalone out of the box unless the user will specifies the dependency. While it can be by design, it differs from "transitive" in module-info.

I'm trying to understand if this is really a problem. Disclaimer: I'm not an expert for JPMS, so my thoughts may be totally wrong. 😆

JAX-RS uses JAXB annotations on the javax.ws.rs.core.Link class. The requires transitive entry in the module-info.java basically means that modules depending on JAX-RS will automatically gain access to the JAXB API, correct? As JAX-RS exports the Link class, using requires transitive makes sense to me.

Regarding Maven: provided seems to be the correct dependency scope for JAXB with JDK11+. We need it for compilation and it has to be available at runtime. I think there is no other Maven scope we can use in this case.

I see two ways to handle this situation:

  • We keep it as it is. The downside is that modules depending on JAX-RS automatically get access to the JAXB classes, although we cannot ensure that these classes are actually available at runtime.
  • We remove the transitive keyword from module-info.java. In this case modules depending on JAX-RS need to add a requires clause them self. Although I'm not sure if this solution would really work, because JAXB is used on classes exported by the JAX-RS module.

It would be great to get some more feedback from JPMS experts regarding this.

@spericas
Copy link
Contributor

spericas commented Nov 2, 2018

@chkal Not an expert in JPMS either, but I just wanted to add a note that we need to drop that dependency in 3.0. It was a mistake to ever introduce it (and I am to blame for proposing that).

@jansupol
Copy link
Contributor Author

jansupol commented Nov 9, 2018

@chkal Thanks for the elaboration. I am no expert on JPMS either, I wrote it after a discussion with people who convinced me it is an issue. Later on, I had a few discussions about it with other people, who say the opposite. I am inclined to think it's best to leave it as it is, just as you say.

@chkal
Copy link
Contributor

chkal commented Nov 10, 2018

@jansupol Thanks. I agree. Maybe we should leave it as it is.

It looks like #686 and #687 fixed everything mentioned in this issue. So I'll close this issue now. Feel free to reopen if I missed anything.

@chkal chkal closed this as completed Nov 10, 2018
@chkal chkal self-assigned this Nov 10, 2018
@chkal
Copy link
Contributor

chkal commented Nov 18, 2018

Version 2.1.3 has been release to the Staging repository now. The manifest of this release looks like this:

Manifest-Version: 1.0
Automatic-Module-Name: java.ws.rs
Bnd-LastModified: 1542482237634
Build-Id: 11/17/2018 02:17 PM
Build-Jdk: 9.0.1
Built-By: genie.jaxrs
Bundle-Description: Java API for RESTful Web Services (JAX-RS)
Bundle-DocURL: https://www.eclipse.org/org/foundation/
Bundle-License: http://www.eclipse.org/legal/epl-2.0, https://www.gnu.
 org/software/classpath/license.html
Bundle-ManifestVersion: 2
Bundle-Name: javax.ws.rs-api
Bundle-SymbolicName: javax.ws.rs-api
Bundle-Vendor: Eclipse Foundation
Bundle-Version: 2.1.3
Created-By: Apache Maven Bundle Plugin
DynamicImport-Package: *
Export-Package: javax.ws.rs.client;version="2.1.3";uses:="javax.net.ss
 l,javax.ws.rs,javax.ws.rs.core",javax.ws.rs.core;version="2.1.3";uses
 :="javax.ws.rs,javax.xml.bind.annotation,javax.xml.bind.annotation.ad
 apters,javax.xml.namespace",javax.ws.rs;version="2.1.3";uses:="javax.
 ws.rs.core",javax.ws.rs.ext;version="2.1.3";uses:="javax.ws.rs,javax.
 ws.rs.core",javax.ws.rs.container;version="2.1.3";uses:="javax.ws.rs.
 core",javax.ws.rs.sse;version="2.1.3";uses:="javax.ws.rs.client,javax
 .ws.rs.core"
Extension-Name: javax.ws.rs
Implementation-Version: 2.1.3
Import-Package: javax.net.ssl,javax.ws.rs;version="[2.1,3)",javax.ws.r
 s.client;version="[2.1,3)",javax.ws.rs.core;version="[2.1,3)",javax.w
 s.rs.ext;version="[2.1,3)",javax.xml.bind.annotation,javax.xml.bind.a
 nnotation.adapters,javax.xml.namespace
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
Specification-Vendor: Oracle Corporation
Specification-Version: 2.1
Tool: Bnd-3.5.0.201709291849

@jansupol Can you confirm that this manifest is ok?

@chkal
Copy link
Contributor

chkal commented Nov 18, 2018

The Bundle-Name, Bundle-SymbolicName and Extension-Name are still using the javax prefix, but it looks like most of the latest releases are doing it the same way (Servlet, Enterprise Security, Websocket). A few others already migrated to a jakarta prefix (JPA for example).

@arjantijms
Copy link
Contributor

I'm pretty sure these should be using javax, as it indicated the package name and that one hasn't changed.

Only the maven coordinates have changed. I agree, it's somewhat confusing.

I assume JPA is wrong here then. cc @m0mus

@chkal
Copy link
Contributor

chkal commented Nov 18, 2018

@arjantijms AFAIK there is no official recommendation for the bundle names yet. It was only mentioned in eclipse-ee4j/ee4j#34 but without any decision.

@chkal
Copy link
Contributor

chkal commented Nov 22, 2018

It looks like the OSGi manifest for 2.1.3 is fine. @jansupol just confirmed this in eclipse-ee4j/jersey#3994 (comment). So from my point of view we could push 2.1.3 into Maven Central now.

@mkarg
Copy link
Contributor

mkarg commented Nov 22, 2018

@spericas Pusing ASAP ok for you, or do we need a voting for this?

@mkarg
Copy link
Contributor

mkarg commented Nov 23, 2018

@eclipse-ee4j/eclipse-jaxrs Last Call! I will forward JAX-RS 2.1.3 from OSSRH Staging to Maven Central if nobody posts -1 within the next eight hours. As JAX-RS 2.1.3 passed TCK I do not see a reason (and the project lead did not tell me one either) to either wait any longer nor to set up an additional voting, because we already agreed to publish 2.1.3 and never decided that that vote is "for staging only".

@mkarg
Copy link
Contributor

mkarg commented Nov 24, 2018

Pushed JAX-RS 2.1.3 to Maven Central. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants