Jgrapht: JDK 9 Modularization

[io7m]

I've opened this ticket to try to keep a running list of the tasks needed to get to full JDK 9 modularization. This continues the work started in https://github.com/jgrapht/jgrapht/pull/458.

• [x] Update plugins to latest versions
• [x] Update JMH to latest version to correct @Generated annotation issues
• [x] Get Antlr4 to publish a module name (https://github.com/antlr/antlr4/pull/2223)
  
  
  
  • [x] Wait for new release
  
  
• [x] Fix failing unit tests
  
  
  
  • [x] The GraphXML exporter was asking for indentation but apparently never actually got it. The tests are written to expect no indentation. On JDK 9, the exporter now actually gets indentation in the resulting XML, so the tests must be updated so that the resulting XML texts match.
  
  
• [x] Write module descriptors (almost complete, just need the last couple of module names for jgraphx and ANTLR)
• [x] Get jgraphx to publish a module name (https://github.com/jgraph/jgraphx/pull/93)
  
  
  
  • [x] Wait for new release
  
  
• [x] Get jgraph to publish a module name (https://github.com/jgraph/legacy-jgraph5) (_cancelled, removing the dependency instead_)

Work is taking place here: https://github.com/io7m/jgrapht/tree/jdk9

[io7m]

Could someone assign this to me? I'm not listed as a contributor, so I can't assign myself.

[io7m]

Thanks!

[d-michail]

I have already fixed a couple of issues in the master branch. You should fetch the changes and continue from there. Thanks.

[io7m]

Will do!

[d-michail]

For future reference, the XML whitespace issues and the jmh generated annotation issues have been fixed in #457.

[d-michail]

There is also a typo in the touchgraph module, where the module is called "demo" instead of "touchgraph".

[io7m]

There is also a typo in the touchgraph module

ARGH! Nice catch... Not sure how I managed that one.

[io7m]

OK, so I've written some tentative module descriptors for the core and io modules, but I had to use the automatically generated antlr4.runtime module name as the Antlr pull request hasn't been accepted yet. This will obviously need to be changed to the real module name if and when they accept it.

The code compiles properly and passes all tests with the module descriptors in place, so I think now it's really just a matter of getting the leftover dependencies to publish Automatic-Module-Name entries. One of those (jgraph) appears to be unmaintained, so it seems unlikely that it'll ever publish one... I'll have to look into what the best practice is in this situation.

[io7m]

Hm, Javadoc-related failure in Travis... Checking what's gone wrong.

[io7m]

The issue is that javadoc aggregation is currently broken in the Javadoc tool if any of the modules in the project don't have module descriptors. There's no workaround yet; this is actually a known bug in the JDK and should be fixed in Java 10.

[jkinable]

Hm, so we cannot include Java 9 modularization, before JDK 10 is released :)?
I doubt we will be able to update jgraph to a newer version any time soon. jgraph is still being developed, but the authors don't publish new versions to maven central so we cannot include any of the newer versions.
In the mean time, it might still be useful to open a PR which fixes as many issues as possible (module descriptors where possible, updated plugin versions, etc)

[io7m]

Hm, so we cannot include Java 9 modularization, before JDK 10 is released :)?

Well, I can correct the issue by writing module descriptors for all of the modules (which I was planning on doing anyway). It's just that the remaining modules have dependencies that haven't been modularized yet.

I doubt we will be able to update jgraph to a newer version any time soon

If the authors aren't receptive to pushing to Central, maybe it'd be worth maintaining a fork in the jgrapht organization that tracks upstream but also publishes module names and pushes to Central. I'd think that'd be pretty easy to set up.

In the mean time, it might still be useful to open a PR

Yep, it's still in progress.

[io7m]

Does anyone have an opinion on how to handle legacy-jgraph5?

I'd like to get this moving but we're currently blocked on "work that other people need to do".

[jsichi]

Is there a reason we still have a dependency on JGraph at all (rather than just JGraphX)? I'm guessing the answer is that the JGraphXAdapter isn't complete, but I'm just guessing that based on the fact that JGraphModelAdapter.java is 1041 LOC, whereas JGraphXAdapter is only 309 LOC.

If that's true, then the correct way to resolve this is to finish the JGraphXAdapter and ditch JGraph for good.

[io7m]

I wonder if you'd be amenable to splitting up the jgrapht-ext module.

I find quite an effective place to draw module boundaries is along the lines of mandatory dependencies. So for example, the jgrapht-ext module depends on both jgraph and jgraphx. I might only want to use the jgraph code, or I might only want to use the jgraphx code, but the module doesn't give me the choice. I either get both or neither. I think a more effective way (assuming that there isn't any interdependency between the classes) would be to publish jgrapht-ext-jgraph and jgrapht-ext-jgraphx modules instead. That way, we could publish a jgrapht-ext-jgraph module under the assumption that there's never going to be a new jgraph release and therefore it doesn't matter that the module depends on a non-modular artifact (as there's no chance of that artifact being later upgraded to a module with a proper Automatic-Module-Name or a real module descriptor). The jgrapht-ext-jgraphx module would depend on the PR I'm about to open for that project.

I have a large number of projects that are all in the process of modularization and a large number of them depend on jgrapht-core, so I'm pretty anxious to get modularization happening as soon as possible. :sweat_smile:

[io7m]

Note that this would require splitting the org.jgrapht.ext package and would therefore be an API-breaking change. It's not permitted for two modules to contain the same package ("the split package problem") and so we'd have to move the classes to org.jgrapht.ext.jgraph and org.jgrapht.ext.jgraphx.

[jsichi]

I'd like to first understand why we still have the jgraph dependency at all. If it's obsolete, we should just tell users that if they want to use it, then they should use an old version of JGraphT as well.

[io7m]

I'm always in favour of ripping out dead code. I assumed it was being kept for some important reason, but ruthlessly eliminating it is an option too!

[jsichi]

@d-michail from what I can tell, you were the last person to wade into JGraph territory...do you have an opinion on whether we can ditch it?

[io7m]

Terence Parr has just let me know that he intends to look at the ANTLR PR in three days or so, so hopefully that'll be moving soon.

[jbduncan]

Sounds like you're doing great work with helping projects move forward with modularization, @io7m, so as an observer, I just wanted to say that I applaud your efforts and I hope things go positively across the board! :)

[d-michail]

@jsichi I am not really sure whether people still use this.

Maybe we can consider creating a side project which contains the two adapters and the demo app (and uses a specific version of the jgrapht library as a dependency).

[jsichi]

@d-michail good idea. I'm going to post on jgrapht-users...if we get the usual crickets as response, then I'd say we should just delete the JGraph dependency, leaving only JGraphX. If someone responds, we can suggest the side project as a way forward.

[io7m]

Does jgrapht follow semantic versioning? I vaguely remember reading somewhere that it did, but now I can't find it.

Point is, removing the class should bump the major version.

[jsichi]

We currently use cowboy versioning; i.e. "how big does this release feel and how long has it been since the last one?" :) We should probably get more serious about that.

We do try to follow a deprecation policy, but in this case, since JGraph itself has been deprecated for ages, I think we can skip it. So far, I haven't heard any responses from the mailing list, so I'll start working on a pull request to remove the JGraph dependency.

[io7m]

ANTLR PR was just merged.

[jbduncan]

@io7m If PR https://github.com/jgrapht/jgrapht/pull/532 gets merged, you'll probably also need to add an automatic module name to the Guava adapter module being developed there.

[jkinable]

Get jgraphx to publish a module name (jgraph/jgraphx#93)
It seems that travis throws some error on jgraph/jgraphx#93 ? Could you have a look at this such that we can wrap this one up? The jgraph dependency has been removed, so we should be getting pretty close.

[io7m]

@jkinable It seems like their Travis CI builds have been failing for a while. If you check their build history, it's just one failure after another... :sweat:

I didn't think I'd made it any worse than it already was.

[jkinable]

I agree. Is this the only thing that is blocking us at the moment?

[io7m]

I don't think a new release of ANTLR has been made yet. The PR was merged, but we obviously need new artifacts to make it to Maven Central.

[jkinable]

Alright, jgraphx merged your PR. Now we have to wait for Antlr and jgraphx to release and publish new artifacts. We are getting closer :)

[io7m]

[jkinable]

update: a new version of jgraphx has been released :)
Now we just have to wait for antlr...

[io7m]

I've just gotten some indication that the release of ANTLR won't be ... soon.

I'm rather impatient to get this moving. How would you feel if i pushed a copy of what will presumably become the 4.7.2 release of ANTLR to a com.io7m.antlr group ID? We could use that as a temporary dependency until the real release of ANTLR is made. It would only involve pointing the POM at com.io7m.antlr:antlr:4.7.2, the module descriptors could remain the same.

[jsichi]

Is there a public discussion thread for the ANTLR release?

[io7m]

Only the PR: https://github.com/antlr/antlr4/pull/2223

They seem to average about nine months between releases. The last release was December 2017, so I assume the next one will be in about four months.

[jsichi]

I don't think we were planning on another release before then ourselves, so it probably makes sense to just wait instead of muddying the dependencies. But I wouldn't want to be charged with the murder (mercy killing?) of Sisyphus. @d-michail and @jkinable any opinions?

[io7m]

Hehe, no worries. My impatience is due to the fact that I have this giant graph of dependencies, and I need to update one at the bottom in order to be able to update all of the rest. The one at the bottom needs a new release of jgrapht in order to be releasable (because modularization is a condition of the release).

[d-michail]

I would prefer to just wait for the next Antlr release. It will likely happen before our next release!

[io7m]

I guess I could push my own version of jgrapht to a com.io7m.jgrapht groupId just to get this stuff out the door. I'll push it with a different qualifier as well so that it's clear to everyone that it's not equivalent to whatever will eventually be the 1.3.0 jgrapht release.

[jsichi]

That should be fine; we'll try not to introduce any new non-modularized depencies in the meantime :)

[jsichi]

Looks like there's finally some movement on this:

https://github.com/antlr/antlr4/issues/2395

[d-michail]

@io7m Finally there! Thanks for the hard work. Do we need to make any more changes? Not sure whether our latest dependencies are ok!

[io7m]

Great!

I'm checking the dependencies now.

[d-michail]

The jheaps library probably needs adjusting. No problem since I write that one.

[io7m]

Yep, that's right. I don't see an Automatic-Module-Name or a module-info.class in the jheaps 0.9 jar.

[io7m]

@d-michail: I think it'd also be helpful if jheaps published OSGi metadata. The current jgrapht modules already do this with the maven-bundle-plugin.

[d-michail]

Good point

[jsichi]

github closed this automatically when I merged #731...please reopen if there's more to do

[jkinable]

@io7m We are currently moving jgrapht from java 8 to java 11. This process has proven to be a bit bumpy.

• do you know what it would take to add proper module support to jgrapht?
• what would be the impact of adding modules to jgrapht for the users, e.g. will this mean that users also have to use modules?
• would you be willing to submit a PR with module support?

The reason I'm asking this is that we are facing some problems with our pre-release build. When trying to compile this branch of jgrapht with Java JDK 14 using the command mvn package -DskipTests, we get the error below. We suspect that this is related to some module issue. It would be great if you could have a look at this!

[INFO] Reactor Summary for JGraphT - Parent 1.4.1-SNAPSHOT:
[INFO] 
[INFO] JGraphT - Parent ................................... SUCCESS [  0.690 s]
[INFO] JGraphT - Core ..................................... SUCCESS [ 26.257 s]
[INFO] JGraphT - I/O ...................................... FAILURE [  5.957 s]
[INFO] JGraphT - Optimized Graphs ......................... SKIPPED
[INFO] JGraphT - Ext ...................................... SKIPPED
[INFO] JGraphT - Guava Adapter ............................ SKIPPED
[INFO] JGraphT - Demo ..................................... SKIPPED
[INFO] JGraphT - Bundle ................................... SKIPPED
[INFO] JGraphT - Distributable Archives ................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  33.369 s
[INFO] Finished at: 2020-06-04T09:38:41-07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.1:jar (attach-javadoc) on project jgrapht-io: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1 - /home/ANT.AMAZON.COM/kinable/workspace/research/external/jgrapht/jgrapht/jgrapht-io/src/main/java/org/jgrapht/nio/gexf/SimpleGEXFEventDrivenImporter.java:22: error: package org.xml.sax is not visible
[ERROR] import org.xml.sax.*;
[ERROR]               ^
[ERROR]   (package org.xml.sax is declared in module java.xml, but module org.jgrapht.io does not read it)
[ERROR] /home/ANT.AMAZON.COM/kinable/workspace/research/external/jgrapht/jgrapht/jgrapht-io/src/main/java/org/jgrapht/nio/gexf/SimpleGEXFEventDrivenImporter.java:23: error: package org.xml.sax.helpers is not visible
[ERROR] import org.xml.sax.helpers.*;
[ERROR]                   ^
[ERROR]   (package org.xml.sax.helpers is declared in module java.xml, but module org.jgrapht.io does not read it)
[ERROR] /home/ANT.AMAZON.COM/kinable/workspace/research/external/jgrapht/jgrapht/jgrapht-io/src/main/java/org/jgrapht/nio/gexf/SimpleGEXFEventDrivenImporter.java:25: error: package javax.xml is not visible
[ERROR] import javax.xml.*;
...

[io7m]

Hello!

Yes, I can take a look at this in a couple of days. Having Java 11 as a minimum dependency certainly makes things easier as you don't have to do anything like placing the module descriptors into META-INF/versions in order to appease people with old JDKs. It looks like I did write module descriptors a while back, but they're sitting in a stale branch in my fork. Shouldn't be hard to bring it all up-to-date though.

what would be the impact of adding modules to jgrapht for the users, e.g. will this mean that users also have to use modules?

It doesn't mean that users will also have to use modules; if this were true then anyone that wasn't currently using modules wouldn't be able to use the JDK at all right now. :smile:

I believe that, because you started publishing an Automatic-Module-Name a long time ago, most IDEs and build tools will already be placing the jgrapht jars into the module path as opposed to the old-style classpath. Publishing explicit descriptors will strengthen the encapsulation and will break the code of anyone that's currently doing something naughty such as looking at private fields in jgrapht reflectively. You can avoid this breakage by marking packages as open, but personally I think anyone doing that kind of unsanctioned reflective access is asking for trouble in the first place!

[jkinable]

Awesome, thank you Mark!

On Thu, Jun 4, 2020 at 2:10 PM Mark Raynsford [e-mail]
wrote:

Hello!

Yes, I can take a look at this in a couple of days. Having Java 11 as a
minimum dependency certainly makes things easier as you don't have to do
anything like placing the module descriptors into META-INF/versions in
order to appease people with old JDKs. It looks like I did write module
descriptors a while back, but they're sitting in a stale branch in my fork.
Shouldn't be hard to bring it all up-to-date though.

what would be the impact of adding modules to jgrapht for the users, e.g.
will this mean that users also have to use modules?

It doesn't mean that users will also have to use modules; if this were
true then anyone that wasn't currently using modules wouldn't be able to
use the JDK at all right now. 😄

I believe that, because you started publishing an Automatic-Module-Name a
long time ago, most IDEs and build tools will already be placing the
jgrapht jars into the module path as opposed to the old-style classpath.
Publishing explicit descriptors will strengthen the encapsulation and will
break the code of anyone that's currently doing something naughty such as
looking at private fields in jgrapht reflectively. You can avoid this
breakage by marking packages as open, but personally I think anyone doing
that kind of unsanctioned reflective access is asking for trouble in the
first place!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jgrapht/jgrapht/issues/484#issuecomment-639118043,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAU5RSSGYJA22V5F24CZM2DRVAEUVANCNFSM4EQFK3TA
.