-
Notifications
You must be signed in to change notification settings - Fork 29
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 module-info.java #141
Add module-info.java #141
Conversation
Fixes jakartaee#140 - Add module-info.java - Change compiler source and target to JDK 11 - Update maven-bundle-plugin to 5.1.2 Signed-off-by: hussainnm <hussain.nm@cognizant.com>
- update parent project to 1.0.7 - fix github url in scm Signed-off-by: hussainnm <hussain.nm@cognizant.com>
I do not believe we can change the compiler version to 11, since Jakarta Enterprise Beans 4.0.x must remain compatible with Java 8 as required by Jakarta EE 9. If Jakarta EE 10 will no longer require support for Java 8, I believe EJB would have to provide a new specification level of at least 4.1 to do that as well. The 4.0 version of the spec requires Java 8, so the 4.0.x versions of the published JARs must maintain that level of support. |
Got it, then we will need a release review created for 4.1 to include module-info for Jakarta EE 10. Shall I go ahead and update the project version to 4.1.0-SNAPSHOT? |
Right, it'd need to be a Plan Review followed by a Release Review. I say go ahead and update the PR to 4.1, but before we can merge it or create a Plan Review we should get some clarification at the platform level as there was an equivalent request made to all the spec projects. That request indicated a service release, but I don't believe this level of change can go into a service release. |
Signed-off-by: hussainnm <hussain.nm@cognizant.com>
I have added configuration to build a multi-release jar to maintain compatibility with JDK8. Hence this can still be done via a service release. |
The main issue is that this API jar is only a reference API jar. Vendors/implementations can supply their own. So this jar can't have any features that aren't part of the standard. If we add a module-info, then everyone who produces an EJB API jar must also have the exact same module-info. There would also need to be signature tests to verify the presence and contents of the module-info in an EJB API jar. Those tests would need to be passed to claim certification. Current rules do not allow for adding API requirements and TCK tests in a service release, so the main issue is we need to agree on the intent at the platform level and then clarify the module-info expectations for all the spec projects. |
@dblevins, this was the gist of the conversation on yesterday's Spec Committee call (I know you were not able to attend, but @jeanouii was on). We're trying to figure out the easiest method of providing these module-info classes without requiring a full minor release for those projects not interested in doing another release for Jakarta EE 10. EJB is one of them, DI, BV, and a couple others fall into this camp. Although adding a new test to the TCK does fall outside of the realm of a service release, we talked about allowing an exception for this case. Again, to make the process easy for this handful of Specs with no plans for a new release. I think the best place to have this discussion is in the Platform issue so that all projects can be on the same page. There is also a thread on the Platform mailing list discussing the testing requirements. |
Signed-off-by: hussainnm <hussain.nm@cognizant.com>
- Reduce minimum JDK required to 9 - Exclude module-info.java in first pass to compile with source and target as 1.8 - Include module-info.java in second pass to compile with release as 9 Changes made as per following email[1] https://www.eclipse.org/lists/jakartaee-spec-project-leads/msg00780.html Signed-off-by: hussainnm <hussain.nm@cognizant.com>
Changes made as per following email https://github.com/eclipse-ee4j/jakartaee-platform/wiki/Modularized-Jars
|
If we do agree on jdk9, there are a few other places that reference it, and we should update:
|
I'm approving this PR, considering the above points I listed should not be blocking it. @dblevins @tkburroughs @kwsutter @Emily-Jiang can you please give it another look in light of the platform spec guideline quoted above by @hussainnm ? |
I think the proposed change that requires jdk9, but targets the compile of everything except The 3 additional files identified by @chengfang would also be nice to update, but not required. |
any updates on this PR and issue? |
Signed-off-by: hussainnm <hussain.nm@cognizant.com>
@hussainnm Thanks for updating the PR with the project rename changes. While the changes here do produce an API JAR with the new module-info.class that is needed, I have found that when I build locally the build now fails right after producing the API JAR when it attempts to produce the javadoc. I'm seeing this:
I'm guessing this is because I'm using Java 11, but it would be nice if that could be supported since Java 9 has been out of support for awhile. Have you been able to build with Java 11? |
Found that adding the
|
Found that adding the following resolves the JavaDoc issue when building with Java 11:
|
I am able to build without any issues with the latest JDK 11.0.14 as well as 18.0.1. The jenkins instance for ejb-api is setup with JDK9 presently, so that is another reason I wanted to keep the Class Version to lowest which could support module-info. My maven and java setup:
@tkburroughs your issue is related to JDK-8240169 : javadoc fails to link to docs with non-matching modularity which got fixed in 11.0.9 |
@hussainnm Thanks for your work on this PR. I have moved up to a newer version of Java 11 and am building without issue. I agree with keeping the minimum version at Java 9, I just wanted to make sure it would build using later JDKs. I also agree with not moving the transaction dependency up, since the Jakarta EE 10 platform recommends including a module-info compiled with Java 9. Since this PR meets the requirements and recommendations for Jakarta EE 10 Modularized-Jars, I recommend this be merged and we begin the process of publishing the 4.0.1 maintenance release. |
Just one minor comment: I don't think this reduce the mininum JDK level to 9 because this spec still works with Java 8. The jar with module-info will have to be JDK 9+. |
Minimum JDK required is for building the project. |
@hussainnm I believe all of the comments on the PR have been resolved. Do you have any other concerns with merging this? |
I have no further concerns. This is ready for merge. |
Merged; ready for the 4.0.1 release |
Thanks, @tkburroughs! |
Fixes #140
Change compiler source and target to JDK 11Add build configuration to generate a multi-release jar targeted to JDK 11+Enforce minimum JDK required is 11 to build the api project