search

jakartaee / enterprise-beans

Jakarta Enterprise Beans
https://eclipse.org/ee4j/ejb
Other
20 stars 29 forks source link

Add module-info.java #141

Closed hussainnm closed 2 years ago

hussainnm commented 3 years ago

Fixes #140

tkburroughs commented 3 years ago

@hussainnm

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.

hussainnm commented 3 years ago

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?

dblevins commented 3 years ago

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.

hussainnm commented 3 years ago

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.

dblevins commented 3 years ago

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.

kwsutter commented 3 years ago

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.

hussainnm commented 3 years ago

Changes made as per following email https://www.eclipse.org/lists/jakartaee-spec-project-leads/msg00780.html

https://github.com/eclipse-ee4j/jakartaee-platform/wiki/Modularized-Jars Implementing option 1: module-info.java must be placed in the root of the JAR

chengfang commented 2 years ago

If we do agree on jdk9, there are a few other places that reference it, and we should update:

chengfang commented 2 years ago

If we do agree on jdk9, there are a few other places that reference it, and we should update:

* .travis.yml

* spec/README

* spec/pom (keep it consistent with api sub-module)

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 ?

tkburroughs commented 2 years ago

I think the proposed change that requires jdk9, but targets the compile of everything except module-info.java for 1.8 is a good approach. However, since jdk9 is out of support, should we really require jdk11, and then set a target of 9 for module-info.java. Leaving at jdk9 would be acceptable though.

The 3 additional files identified by @chengfang would also be nice to update, but not required.

chengfang commented 2 years ago

any updates on this PR and issue?

tkburroughs commented 2 years ago

@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:

[INFO] Jakarta Enterprise Beans API 4.0.1-SNAPSHOT ........ FAILURE [ 33.048 s]
[INFO] Jakarta Enterprise Beans Specification 4.0-SNAPSHOT  SKIPPED
[INFO] Jakarta Enterprise Beans Parent 4.0.0-SNAPSHOT ..... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  34.763 s
[INFO] Finished at: 2022-04-22T09:29:32-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:jar (attach-javadocs) on project jakarta.ejb-api: MavenReportException: Error while generating Javadoc:
[ERROR] Exit code: 1 - javadoc: error - The code being documented uses packages in the unnamed module, but the packages defined in https://docs.oracle.com/en/java/javase/11/docs/api/ are in named modules.
[ERROR]
[ERROR] Command line was: cmd.exe /X /C "C:\eclipse\java\11.0.7.10\bin\javadoc.exe @options @packages"
[ERROR]
[ERROR] Refer to the generated Javadoc files in 'C:\jakartaee\enterprise-beans\api\target\apidocs' dir.
[

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?

tkburroughs commented 2 years ago

Found that adding the detectJavaApiLink as follows resolves the JavaDoc issue when building with Java 11:

                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-javadoc-plugin</artifactId>
                    <version>${maven-javadoc-plugin.version}</version>
                    <configuration>
                        <source>${maven.compiler.source}</source>
                        <detectJavaApiLink>false</detectJavaApiLink>
tkburroughs commented 2 years ago

Found that adding the following resolves the JavaDoc issue when building with Java 11:

                        <detectJavaApiLink>false</detectJavaApiLink>
hussainnm commented 2 years ago

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:

Apache Maven 3.8.5 (3599d3414f046de2324203b78ddcf9b5e4388aa0)
Maven home: C:\opt\apache-maven-3.8.5
Java version: 11.0.14.1, vendor: International Business Machines Corporation, runtime: C:\opt\java\jdk-11.0.14.1+1
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

@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

tkburroughs commented 2 years ago

@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.

Emily-Jiang commented 2 years ago
  • Reduce minimum JDK required to 9

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+.

hussainnm commented 2 years ago
  • Reduce minimum JDK required to 9

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.

tkburroughs commented 2 years ago

@hussainnm I believe all of the comments on the PR have been resolved. Do you have any other concerns with merging this?

hussainnm commented 2 years ago

I have no further concerns. This is ready for merge.

tkburroughs commented 2 years ago

Merged; ready for the 4.0.1 release

dblevins commented 2 years ago

Thanks, @tkburroughs!