Add Java 11 compatibility #2812

leonardehrenfried · 2019-08-24T20:52:27Z

To be completed by pull request submitter:

issue: Fixes Target Java 11 #2717
roadmap: Not on the roadmap, it appears
tests: n/a
formatting: n/a
documentation: If you are adding a new configuration option, have you added an explanation to the configuration documentation tables and sections?
changelog: add a bullet point to the changelog file with description and link to the linked issue

To be completed by @opentripplanner/plc:

reviews and approvals by 2 members, ideally from different organizations
after merging: update the relevant card on the roadmap

This PR adds is a first try of making OTP work with Java 11. I got very close but I'm stuck with a single problem right now.

I did the following:

Updated the relevant Maven plugins
Added a module-info.java that lists all the required modules (has to be maintained manually - gross but necessary)

This made the project compile and the tests run.

The last remaining problem is the ObjectDiffer usage in GraphInitializationTest, which relies heavily on reflection. Its extensive usage of reflection sd problematic under Java 11.

I tried to work around it by adding --add-opens command line arguments to the forked test JVM and made some progress. However, I also had to add 2 classes to ObjectDiffer's ignore list:

Method as it was part of a proxy object which could not be reflected on in Java 11
JarFile: the caused a stack overflow in the test but I'm not actually what exactly has changed since Java 8 to make this use a huge stack.

In the end, I made the test pass but I'm not sure if excluding quite a few of these classes defeats the purpose of the test.

Looking forward to hearing your opinion about this.

Open questions:

Is support for Java 8 still needed considering that it's EOL?

leonardehrenfried · 2019-08-25T21:31:19Z

Hmm, the tests don't pass on Travis which is weird because the pass on my local machine, also running Java 11 and Linux.

Perhaps more classes need to be excluded?

leonardehrenfried · 2019-08-25T21:43:55Z

Ok ignoring SoftReference does the trick but now I see lots of Javadoc errors.

It appears that Java 11 is a lot more strict with regards to the actual Javadoc syntax.

Are you ok with me cleaning these up?

abyrd · 2019-08-28T11:02:58Z

Great to have a PR for this. I hope to try it out in the coming days and hopefully help get everything working. @leonardehrenfried do feel free to clean up Javadoc to meet the new exacting standards.

leonardehrenfried · 2019-09-05T15:53:03Z

I have cleaned up the Javadoc so that it doesn't throw any errors on Java 11 anymore.

Looking forward to your review.

I also successfully built a graph and ran it (including debug layers) on Java 11! Very pleased with this!

abyrd · 2019-09-06T07:18:10Z

Hi @leonardehrenfried. Thanks for raising the issue of Java 11+ compatibility and putting in the effort to solve these Jigsaw module problems. I had tried to migrate a smaller but related project at one point and ran into the same final roadblock: the ObjectDiffer used in the tests inherently works by reflection. I think it will be much easier for OTP to explicitly permit the module containing ObjectDiffer (kryo-tools) to perform reflection if kryo-tools is itself migrated to the Jigsaw module system. Fortunately this is our own project, so I can make the changes. I'm working on migrating that module, which is good training for then reviewing this PR and suggesting a fix to allow kryo-tools to perform reflection in tests.

abyrd · 2019-09-06T07:27:03Z

Oh nice, I see you've added arguments to exceptionally allow reflection just during the tests. Maybe that's even better than module directives that would open things to reflection during non-test execution.

leonardehrenfried · 2019-09-06T08:07:29Z

Exactly. This allows us to run kryo-tools as is without adding module information to it.

I'm not sure if that would help anyway, since you would have to add the permission to the java.base module, don't you? Or am I seeing this wrong?

leonardehrenfried · 2019-09-06T09:07:13Z

One thing we ought to think about is Java 8 compatibility. Is it desired considering that it's EOL?

I'm not sure if there have been Java bytecode changes from Java 8 to 11 but if there are, then Java 8 will not be able to run code compiled for 11.

abyrd · 2019-09-11T07:06:25Z

I'm not too concerned about maintaining compatibility. Java 8 has started disappearing from various build and deployment environments anyway and it's very easy to get a copy of 11 or higher.

I think your approach, making exceptions only during tests, is probably a good one. I'm going to continue with that kryo-tools Java 11 migration just to make sure I understand well enough how the whole system works.

abyrd

I have built this locally and it works, thanks so much for working though this process! Having studied the question a but I do think adding "opens" switches to the test runner is the right approach for targeted white-box testing, as it still prevents reflection during normal non-test execution.

Also, it's good to update all these Maven plugins to more recent versions.

Once this is merged we'll port it over to the OTP2 branch.

abyrd · 2019-09-11T07:26:28Z

pom.xml

@@ -78,7 +78,7 @@
    </scm>

    <properties>
-        <geotools.version>20.1</geotools.version>
+        <geotools.version>21.2</geotools.version>


I see Geotools 21 was the first version with Java 11 compatibility. I assume we aren't jumping to an even newer Geotools because of the transitive dependency on JTS, which has changed its package name and will conflict with other OTP dependencies?

I tought that 21.2 is the newest stable version: https://github.com/geotools/geotools/tags

Or would you like to use 22-RC?

Oh I suppose it is. I saw 23-SNAPSHOT and must have assumed there was already a 22 release. But now I see 22 is also still in SNAPSHOT. In that case then there's no question, 21.2 is fine.

abyrd · 2019-09-11T07:34:12Z

pom.xml

@@ -259,10 +258,25 @@
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-surefire-plugin</artifactId>
-                <version>2.21.0</version>
+                <version>2.19.1</version>


I see that in c42ca0c you downgraded this due to a corrupted stream error. I'd like to eventually find and fix the root problem. Can you provide a link to the ticket you referenced in your commit message?

Of course, I will try again and clarify the commit message.

abyrd · 2019-09-11T07:43:33Z

src/main/java/org/opentripplanner/analyst/batch/CSVPopulation.java

@@ -64,7 +64,7 @@ else if (CRS.getAxisOrder(destCrs) == CRS.AxisOrder.EAST_NORTH)
                if (!destCrs.equals(sourceCrs)) {
                    transform = true;

-                    // find the transformation, being strict about datums &c.


Personally I like ampersand-c. as a way to write "et cetera", as the ampersand is a ligature of Latin "et". It's fine to remove it though if it might confuse some people.

Oh I see, Javadoc is interpreting this as in HTML. Now I see why it's removed.

Correct, Java 11 is way more strict with these HTML entities.

A big problem where also the < and > characters. I got a little creative and converted to unicode characters where possible. If not I just spelled them out with words.

In my commits on the 2.x port, I used > to replace > in some places, but that makes it kind of unreadable in a text editor. Although I managed to eliminate all the error messages, the most complete "solution" was to just turn off the doclint in the config in the POM.

Tip: In Intellij Ctrl+J (at leas with mac shortcuts) displays the formated text - I find it easier to read. So ,I tend to favor good readable formated JavaDoc over it to be easy to read in plain text.

abyrd · 2019-09-11T07:48:47Z

@t2gran could you also review this?

Ports PR #2812 over to 2.x as a single commit.

abyrd · 2019-09-11T08:59:58Z

@leonardehrenfried I ported this over to dev-2.x in PR #2829. You might want to cherry-pick my additional commit b121b85 from over there.

leonardehrenfried · 2019-09-11T11:57:16Z

I've clarified why I downgraded the surefire plugin in the message of the rebased commit: ca708cc

leonardehrenfried · 2019-09-13T09:22:47Z

@abyrd If you're concerned about the surefire downgrade, I'm happy to do more research about upgrading it together with an upgrade to Junit 5. I'd suggest to do that in a separate PR in order to limit the scope of this one.

abyrd · 2019-09-13T10:03:46Z

Thanks @leonardehrenfried. I'm not really concerned about it as long as it runs the tests within the build as expected. I was thinking of putting a brief comment in the POM XML itself (rather than in the commit message) because this is where a contributor would be most likely to see it if/when they decide to upgrade the surefire plugin. Otherwise they might just see an "outdated" version and not understand it was an intentional choice. But having the comment on the commit message is good enough.

The combination of Java 11, the junit version, the surefire plugin version and the plugin config `forkCount` and `reuseForks` caused a variety of failures when running the tests, most frequently a ClassNotFoundException like this: ClassNotFoundException: org.apache.maven.plugin.surefire.StartupReportConfiguration It is not entirely clear what the correct way to fix this but it occurs even when you use version 3.0.0-M3 of the surefire plugin, which is the newest released version as of writing this. Perhaps also upgrading to junit 5 might resolve this, but that seemed a little beyond the scope of the PR. The least invasive choice was to downgrade the surefire plugin version. junit-team/junit5#1367 https://jira.apache.org/jira/browse/SUREFIRE-1520 https://issues.apache.org/jira/browse/SUREFIRE-1534 https://stackoverflow.com/questions/53437819/maven-surefire-and-jdk-11

This eliminates Maven warnings. Arguably we might want Javadoc linting on, but it was already switched off using a different parameter for an older Javadoc plugin version.

leonardehrenfried · 2019-09-16T10:44:29Z

I played around with it a bit and i can make the build pass by excluding the package that is causing the Javadoc error:
<excludePackageNames>org.opentripplanner.graph_builder.module.ned:*</excludePackageNames>

Is this an acceptable solution?

abyrd · 2019-09-17T04:06:57Z

@leonardehrenfried absolutely, we can exclude some classes from the Javadoc to avoid the problem. I already wasted to much time trying to make this class visible to the Javadoc tools. Just be sure to include a comment alongside the setting explaining why we're excluding this one package.

After everything I've been reading about the module system, I'm also wondering whether this system will ever really be adopted or will just create a worldwide mess. It seems quite flawed and appears to have mostly negative impacts on the average project, and this view is shared by several knowledgeable sources.

t2gran · 2019-09-17T08:45:02Z

In our case we probably want org.opentripplanner.otp. 👍

t2gran · 2019-09-17T08:53:42Z

I think we only need JavaDoc on the interface classes - documenting the API. Developers read the JAvaDoc in an IDE or the source.The API classes is needed because that is the place for the most up to date doc on the API. (In OTP2 we could drop the JavaDoc entierly, the GraphQL enpoints will have schema witch document them)

@leonardehrenfried I approved the review, because all my comments were not that important or on stuff that existed from before - feel free to fix these things, but my point was to give some hints for next time to improve.

leonardehrenfried · 2019-09-17T09:50:16Z

@abyrd I've added a comment about the exclusion and also about the surefire downgrade. Regarding surefire, would you welcome a separate PR where I upgraded Junit and Surefire to their newest versions? And whilst I am cleaning up the build, can I also get rid of the dependency on osmpbf 1.3.4-SNAPSHOT (as mentioned in #2638)?

@t2gran While I was there I also incorporated your Javadoc review feedback.

leonardehrenfried · 2019-09-23T20:15:16Z

I would absolutely love to get this merged. Is there any other improvement you'd like me to do?

abyrd · 2019-10-12T10:32:19Z

@leonardehrenfried we are now working on finalizing this transition to Java 11+, hoping to do it within a few days. To answer your last question, yes if you want to do the work to research how to move Junit and Surefire to their newest versions, it would be nice to have a pull request to that effect. But it's not necessarily high priority.

I think we already have a path to update the PBF dependency to a newer version, though it doesn't really solve #2638 in the sense that it's still a forked library. But it's a forked library with one tiny patch so it doesn't really bother me. We released crosby.binary:osmpbf version 1.3.3-entur with a dependency on com.google.protobuf:protobuf-java version 3.8.0, to the Conveyal Maven repo, which is already in the OTP POM:

<repository>
  <id>conveyal</id>
   <name>conveyal</name>
  <url>https://maven.conveyal.com</url>
</repository>

Before we move to merge this PR #2812, I am going to revise #2829 (the port of this PR to OTP2) to see how well it works with no module-info at all, only declaring the automatic module name. We may also disable Javadoc publication there. It's good to have the module-info file around for future reference, but I think we should do the simplest thing that works to allow building on JDK11+.

Migrate to Java 11 (porting PR #2812, plus a few small changes)

abyrd · 2019-10-14T16:09:10Z

@leonardehrenfried we have revised #2829 and merged it into the OTP2 dev branch. It has no module-info at all, and only declares an automatic module name. This seems to be working well. We will not be disabling Javadoc publication because deploying to Maven Central requires a Javadoc JAR.

Therefore, in this branch I suggest removing module-info from this PR and making sure <doclint>none</doclint> is set on the Javadoc generation in both surefire-plugin sections of the POM. Then we can merge it.

We can then introduce a module-info on the theoretical future day when all our dependencies declare their own module names.

leonardehrenfried · 2019-10-15T10:00:12Z

Thanks for heads up @abyrd, but this pull request already has no module-info.java and uses <doclint>none</doclint>.

I changed that when you suggested it in #2812 (comment)

leonardehrenfried · 2020-03-10T10:13:31Z

Hi @abyrd,

I know that this has been merged into 2.x but is it also going to land in OTP 1.5?

leonardehrenfried · 2020-03-30T19:12:35Z

Java 14 has been released and Java 8 has been EOL for quite some time.

Would there be any chance to merge this into dev-1.x?

leonardehrenfried requested a review from a team as a code owner August 24, 2019 20:52

leonardehrenfried force-pushed the java-11 branch 2 times, most recently from 574a39d to d4cc827 Compare August 25, 2019 21:19

leonardehrenfried mentioned this pull request Aug 27, 2019

Target Java 11 #2717

Closed

leonardehrenfried force-pushed the java-11 branch 4 times, most recently from 38b3581 to e39bfd0 Compare September 5, 2019 15:39

leonardehrenfried force-pushed the java-11 branch from e39bfd0 to ff05951 Compare September 6, 2019 08:56

abyrd previously approved these changes Sep 11, 2019

View reviewed changes

abyrd added a commit that referenced this pull request Sep 11, 2019

Allow building on Java 11 (Jigsaw modules)

6788a0d

Ports PR #2812 over to 2.x as a single commit.

abyrd mentioned this pull request Sep 11, 2019

Migrate to Java 11 (porting PR #2812, plus a few small changes) #2829

Merged

leonardehrenfried dismissed abyrd’s stale review via e86e55d September 11, 2019 11:01

leonardehrenfried force-pushed the java-11 branch 2 times, most recently from 62ef895 to cc9d265 Compare September 11, 2019 11:56

t2gran added the OTP1 Fix or backport to the 1.x version of OTP label Sep 13, 2019

leonardehrenfried and others added 7 commits September 16, 2019 12:14

Pass --add-opens to forked test JVM

aca91ab

Exclude java.lang.reflect.Method from graph test

a19a273

Make tests pass by ignoring jar-file-related classes

bbe4c32

Also ignore SoftReference

cbaa7bc

Fix Javadoc to adhere to Java 11's stricter syntax rules

e41888a

Specify plugin version and switch off Javadoc linting.

ec99e1b

This eliminates Maven warnings. Arguably we might want Javadoc linting on, but it was already switched off using a different parameter for an older Javadoc plugin version.

leonardehrenfried force-pushed the java-11 branch from 8b4e214 to 72ce6fe Compare September 16, 2019 10:14

leonardehrenfried added 2 commits September 17, 2019 11:25

Remove module-info.java, use auto-modules

36d875b

Incorporate @t2gran's review feedback

d996430

leonardehrenfried force-pushed the java-11 branch from 72ce6fe to d996430 Compare September 17, 2019 09:41

Remove unnecessary repository

00ed2b5

t2gran previously approved these changes Sep 17, 2019

View reviewed changes

abyrd added a commit that referenced this pull request Oct 12, 2019

Merge pull request #2829 from opentripplanner/java-11-2.x

d46ca40

Migrate to Java 11 (porting PR #2812, plus a few small changes)

Merge branch 'dev-1.x' into java-11

408cbec

leonardehrenfried dismissed t2gran’s stale review via 408cbec October 15, 2019 09:57

Merge branch 'dev-1.x' into java-11

f45ac81

leonardehrenfried mentioned this pull request Apr 17, 2020

Geotools is missing from used repositories #3034

Closed

leonardehrenfried closed this Apr 13, 2021

Add Java 11 compatibility #2812

Add Java 11 compatibility #2812

Conversation

leonardehrenfried commented Aug 24, 2019 • edited

leonardehrenfried commented Aug 25, 2019

leonardehrenfried commented Aug 25, 2019 • edited

abyrd commented Aug 28, 2019

leonardehrenfried commented Sep 5, 2019 • edited

abyrd commented Sep 6, 2019

abyrd commented Sep 6, 2019

leonardehrenfried commented Sep 6, 2019

leonardehrenfried commented Sep 6, 2019 • edited

abyrd commented Sep 11, 2019 • edited

abyrd left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried Sep 11, 2019 • edited

Choose a reason for hiding this comment

abyrd Sep 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abyrd commented Sep 11, 2019

abyrd commented Sep 11, 2019

leonardehrenfried commented Sep 11, 2019 • edited

leonardehrenfried commented Sep 13, 2019

abyrd commented Sep 13, 2019

leonardehrenfried commented Sep 16, 2019

abyrd commented Sep 17, 2019

t2gran commented Sep 17, 2019 • edited

t2gran commented Sep 17, 2019

leonardehrenfried commented Sep 17, 2019

leonardehrenfried commented Sep 23, 2019

abyrd commented Oct 12, 2019

abyrd commented Oct 14, 2019

leonardehrenfried commented Oct 15, 2019

leonardehrenfried commented Mar 10, 2020

leonardehrenfried commented Mar 30, 2020

leonardehrenfried commented Aug 24, 2019 •

edited

leonardehrenfried commented Aug 25, 2019 •

edited

leonardehrenfried commented Sep 5, 2019 •

edited

leonardehrenfried commented Sep 6, 2019 •

edited

abyrd commented Sep 11, 2019 •

edited

abyrd left a comment •

edited

leonardehrenfried Sep 11, 2019 •

edited

abyrd Sep 11, 2019 •

edited

leonardehrenfried commented Sep 11, 2019 •

edited

t2gran commented Sep 17, 2019 •

edited