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.

Sign up for GitHub

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 as per platform wiki #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add module info as per platform wiki #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions api/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@
<legal.doc.source>../</legal.doc.source>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
<maven-compiler-plugin.version>3.8.1</maven-compiler-plugin.version>
<spec-version-maven-plugin.version>2.1</spec-version-maven-plugin.version>
<maven-bundle-plugin.version>4.2.1</maven-bundle-plugin.version>
Expand All @@ -50,7 +48,8 @@
<maven-enforcer-plugin.version>3.0.0-M3</maven-enforcer-plugin.version>
<build-helper-maven-plugin.version>3.1.0</build-helper-maven-plugin.version>
<maven-project-info-reports-plugin.version>3.0.0</maven-project-info-reports-plugin.version>
<jakarta.transaction-api.version>2.0.0</jakarta.transaction-api.version>
<jakarta.transaction-api.version>2.0.1-RC1</jakarta.transaction-api.version>
<jakarta.cdi-api.version>3.0.1</jakarta.cdi-api.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no changes to the EJB API for Jakarta EE 10, so why is a dependency on CDI being added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would assume this would need to be for EJB 4.0.1-RC1 until transactions releases 2.0.1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JTA has a transitive dependency on CDI. Building without the dependency produces:

[INFO] Compiling 82 source files to /Users/starksm/Dev/JBoss/Jakarta/rh-ejb/api/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] module not found: jakarta.cdi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JTA has a transitive dependency on CDI. Building without the dependency produces:

[INFO] Compiling 82 source files to /Users/starksm/Dev/JBoss/Jakarta/rh-ejb/api/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] module not found: jakarta.cdi

Already approved

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to comment on the dependency on cdi as well. I think it will automatically pull in the transitive dependencies.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not provided to the compiler because the pom level dependency of transactions on CDI is provided scope, and maven does not include this in the dependency graph:

[INFO] --- maven-dependency-plugin:2.8:tree (default-cli) @ jakarta.ejb-api ---
[INFO] jakarta.ejb:jakarta.ejb-api:jar:4.0.1-SNAPSHOT
[INFO] \- jakarta.transaction:jakarta.transaction-api:jar:2.0.1-RC1:compile
[INFO] ------------------------------------------------------------------------

</properties>

<name>Jakarta Enterprise Beans API</name>
Expand Down Expand Up @@ -190,10 +189,30 @@
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<!-- This is the common/default-compile execution configuration -->
<configuration>
<source>${maven.compiler.source}</source>
<target>${maven.compiler.target}</target>
<release>11</release>
<compilerArgs>
<arg>-Xlint:all</arg>
</compilerArgs>
<showDeprecation>true</showDeprecation>
<showWarnings>true</showWarnings>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR #141 has a review comment from Emily about it needing to be an MR JAR. I don't really see why that is needed. If that is required then I would expect something like this to be configured for the release 11 execution:

<outputDirectory>${project.build.outputDirectory}/META-INF/versions/9</outputDirectory>

If no MR JAR is required then we might as well use PR #141

Copy link
Member

@tkburroughs tkburroughs Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree; I don't see why a MR JAR would be needed here. The guidelines referenced in the description indicate the following:

If your spec is not updating the API and producing a service release that needs to support Java SE 8,
run two passes of the compiler-plugin to produce the SE 8 based content, followed by a pass to
produce the SE 9 based module-info.class.

PR #141 appears to follow the guidelines exactly, producing Java 8 content, except for a Java 9 module-info.class.

The guidelines then provide an example of another project which seems to produce a MR JAR, but I don't understand what advantage that would have for enterprise beans, and would just double the size of the API JAR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#141 is also an MR jar, but it does not depend on the correctly modularizied transactions artifact which is seen when compiling that module-info:

[WARNING] /Users/starksm/Dev/JBoss/Jakarta/rh-ejb/api/src/main/java/module-info.java:[22,32] requires transitive directive for an automatic module

That is also why the jakarta.transactions transitive dependency on CDI is not seen and thus required to compile.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented on the change not following the same style as other specs: see here for the other spec doing

</configuration>
<executions>
<!-- This runs after the default-compile execution to rebuild with 8 -->
<execution>
<id>base-compile</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<release>8</release>
<excludes>
<exclude>module-info.java</exclude>
</excludes>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.glassfish.build</groupId>
Expand Down Expand Up @@ -413,5 +432,11 @@
<artifactId>jakarta.transaction-api</artifactId>
<version>${jakarta.transaction-api.version}</version>
</dependency>
<dependency>
<groupId>jakarta.enterprise</groupId>
<artifactId>jakarta.enterprise.cdi-api</artifactId>
<version>${jakarta.cdi-api.version}</version>
<scope>provided</scope>
</dependency>
</dependencies>
</project>
27 changes: 27 additions & 0 deletions api/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (c) 2022 Contributors to the Eclipse Foundation
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
* http://www.eclipse.org/legal/epl-2.0.
*
* This Source Code may also be made available under the following Secondary
* Licenses when the conditions for such availability set forth in the
* Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
* version 2 with the GNU Classpath Exception, which is available at
* https://www.gnu.org/software/classpath/license.html.
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
*/
/**
* Defines the modules for jakarta.ejb
*/
module jakarta.ejb {
requires java.rmi;
requires java.naming;
requires transitive jakarta.transaction;

exports jakarta.ejb;
exports jakarta.ejb.embeddable;
exports jakarta.ejb.spi;
}