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

Bump master to SE 11, introduce JPMS descriptors #1093

Merged
merged 3 commits into from
May 4, 2021

Conversation

lukasj
Copy link
Member

@lukasj lukasj commented Apr 26, 2021

This adds JPMS descriptors to the project and sets Java SE 11 as the minimal required Java SE version to build & run master. Note that running EclipseLink on the module path should still be considered as highly experimental.

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
@lukasj lukasj requested a review from rfelcman April 26, 2021 11:25
@lukasj lukasj linked an issue Apr 26, 2021 that may be closed by this pull request
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

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

What about JPMS usage in the tests https://maven.apache.org/surefire/maven-surefire-plugin/examples/jpms.html ?
But it leads into additional changes in the module-info.java files.
E.g.
See my test in JPA.JPQL Maven module:
modified jpa/org.eclipse.persistence.jpa.jpql/src/main/java/module-info.java

module org.eclipse.persistence.jpa.jpql {

    requires transitive java.sql;

    exports org.eclipse.persistence.jpa.jpql;
    exports org.eclipse.persistence.jpa.jpql.parser;
    exports org.eclipse.persistence.jpa.jpql.tools;
    exports org.eclipse.persistence.jpa.jpql.tools.model;
    exports org.eclipse.persistence.jpa.jpql.tools.model.query;
    exports org.eclipse.persistence.jpa.jpql.tools.resolver;
    exports org.eclipse.persistence.jpa.jpql.tools.spi;
    exports org.eclipse.persistence.jpa.jpql.tools.utility;
    exports org.eclipse.persistence.jpa.jpql.tools.utility.iterable;
    exports org.eclipse.persistence.jpa.jpql.utility;
    exports org.eclipse.persistence.jpa.jpql.utility.iterable;
}

new one in jpa/org.eclipse.persistence.jpa.jpql/src/test/java/module-info.java

module org.eclipse.persistence.jpa.jpql.test {

    requires org.eclipse.persistence.jpa.jpql;
    requires jakarta.persistence;
    requires junit;
    exports org.eclipse.persistence.jpa.tests.jpql to junit;
}

@@ -686,38 +584,4 @@
</plugin>
</plugins>
</build>

<profiles>
<!-- sdo: api dependencies -->
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not related to this line, but in general I'm missing eclipselink/jlib/sdo/commonj.sdo*.jar in the output eclipselink-3.1.0-SNAPSHOT.zip bundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's packaged in the eclipselink.jar and org.eclipse.persistetce.sdo.jar - without that, there is no way to make sdo stuff working on the JPMS properly without figuring out proper set of add-exports/add-reads/patch-module options and that would make it really painful to use it in other projects, like Metro

@@ -357,49 +291,6 @@
<scope>provided</scope>
</dependency>

<dependency>
Copy link
Contributor

@rfelcman rfelcman May 4, 2021

Choose a reason for hiding this comment

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

Just to be sure, we want exclude source jars from eclipselink-*.zip bundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

no. There are multiple ways one can include them. This part of the change does the trick without explicit dependency on source artifacts:

-                           <includeClassifiers>sources</includeClassifiers>
+                           <classifier>sources</classifier>

@@ -358,6 +358,23 @@
<artifactId>jakarta.xml.bind-api</artifactId>
<classifier>sources</classifier>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

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

This javadoc dependencies are added to eclipselink-plugins.zip too. Is it right?

Copy link
Member Author

Choose a reason for hiding this comment

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

only those which which were there except of nosql one

<execution>
<id>nosql-javadoc.jar</id>
<phase>prepare-package</phase>
<goals>
<goal>jar</goal>
</goals>
<configuration>
<skip>true</skip>
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently eclipselink-plugins-nosql-*.zip bundle doesn't contains nosql-javadoc.jar if project is build with -Poss-release profile?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, problem with that is that I haven't found a way to produce javadoc for 2+ modules through maven yet. I'm also not sure we need it.

@lukasj
Copy link
Member Author

lukasj commented May 4, 2021

What about JPMS usage in the tests?

that's for another round. There are issues which needs to be resolved first, ie moxy depends on "old" jmockit, some dependencies are not proper modules (yet) + there are also bugs in el codebase which have to be fixed (ie moxy should be ignoring $VALUES filed in enums) but I don't think it makes sense to do everything at once.

Another thing is that some tests can be written in a way of test module depending on the one being tested while others can't (for multiple reasons - they may be in the same package or simply use private access)

Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasj lukasj merged commit ac66849 into eclipse-ee4j:master May 4, 2021
@lukasj lukasj deleted the jpmsea branch May 4, 2021 12:25
rfelcman added a commit to rfelcman/eclipselink that referenced this pull request May 5, 2021
Default JDK for build and tests is Open JDK 11 instead of previous Oracle JDK 8.0.
Maven is switched to latest version 3.6.3.
This change is related with implementation/activation Java Module System JPMS  in PR eclipse-ee4j#1093.
There is also notification system enabled in the Jenkins pipeline to send a mail on unsuccessful and fixed builds and some minor release scripts changes.

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
rfelcman added a commit that referenced this pull request May 5, 2021
Default JDK for build and tests is Open JDK 11 instead of previous Oracle JDK 8.0.
Maven is switched to latest version 3.6.3.
This change is related with implementation/activation Java Module System JPMS  in PR #1093.
There is also notification system enabled in the Jenkins pipeline to send a mail on unsuccessful and fixed builds and some minor release scripts changes.

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
lukasj added a commit to lukasj/eclipselink that referenced this pull request Aug 31, 2021
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
(cherry picked from commit ac66849)
lukasj added a commit that referenced this pull request Aug 31, 2021
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
(cherry picked from commit ac66849)
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 this pull request may close these issues.

Eclipselink and Java 9+ modularity
2 participants