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

JDK 9 Modularization #484

Closed
io7m opened this Issue Feb 11, 2018 · 50 comments

Comments

Projects
None yet
5 participants
@io7m
Copy link

io7m commented Feb 11, 2018

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 #458.

  • Update plugins to latest versions
  • Update JMH to latest version to correct @Generated annotation issues
  • Get Antlr4 to publish a module name (antlr/antlr4#2223)
    • Wait for new release
  • Fix failing unit tests
    • 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.
  • Write module descriptors (almost complete, just need the last couple of module names for jgraphx and ANTLR)
  • Get jgraphx to publish a module name (jgraph/jgraphx#93)
    • Wait for new release
  • 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

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

Thanks!

@d-michail

This comment has been minimized.

Copy link
Collaborator

d-michail commented Feb 11, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

Will do!

@d-michail

This comment has been minimized.

Copy link
Collaborator

d-michail commented Feb 11, 2018

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

io7m added a commit to io7m/jgrapht that referenced this issue Feb 11, 2018

Merge in changes from upstream master
This required some manual merging as master had diverged from my
jdk9 branch.

Affects jgrapht#484
@d-michail

This comment has been minimized.

Copy link
Collaborator

d-michail commented Feb 11, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

There is also a typo in the touchgraph module

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

io7m added a commit to io7m/jgrapht that referenced this issue Feb 11, 2018

Require JDK 9 to build
This switches the pom files to requiring (via the enforcer plugin)
JDK 9 to build the code. The code is built as Java 8 bytecode and
the --release flag is used to ensure that no accidental uses of APIs
outside of Java 8 are used.

Affects jgrapht#484

io7m added a commit to io7m/jgrapht that referenced this issue Feb 11, 2018

Write initial experimental module descriptors
This provides module-info.java files for the core and io
modules. These absolutely should *NOT* make it into any release,
because the io descriptor has a "requires" clause that mentions
the automatically-generated "antlr4.runtime" name as opposed to the
correct "org.antlr.antlr4.runtime" name that must be used if and when
the Antlr4 Automatic-Module-Name is published. The old name
is used purely to test whether or not the code works when modularized.

See antlr/antlr4#2223

Affects jgrapht#484
@io7m

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

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

This comment has been minimized.

Copy link

io7m commented Feb 11, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Feb 12, 2018

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

This comment has been minimized.

Copy link
Collaborator

jkinable commented Feb 12, 2018

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

This comment has been minimized.

Copy link

io7m commented Feb 12, 2018

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 added a commit to io7m/jgrapht that referenced this issue Feb 23, 2018

Write the rest of the module descriptors
This writes the rest of the module descriptors. It, unfortunately,
has to depend on some module names that have not yet been turned
into Automatic-Module-Names.

See https://github.com/jgraph/jgraphx/issues/91

Affects jgrapht#484
@io7m

This comment has been minimized.

Copy link

io7m commented Mar 1, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented Mar 2, 2018

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

This comment has been minimized.

Copy link

io7m commented Mar 12, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented Mar 12, 2018

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

This comment has been minimized.

Copy link

io7m commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented Mar 12, 2018

@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

This comment has been minimized.

Copy link

io7m commented Mar 12, 2018

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

This comment has been minimized.

Copy link

jbduncan commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Collaborator

d-michail commented Mar 13, 2018

@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

This comment has been minimized.

Copy link
Member

jsichi commented Mar 13, 2018

@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

This comment has been minimized.

Copy link

io7m commented Mar 14, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented Mar 14, 2018

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

This comment has been minimized.

Copy link

io7m commented Mar 18, 2018

ANTLR PR was just merged.

@jbduncan

This comment has been minimized.

Copy link

jbduncan commented Apr 2, 2018

@io7m If PR #532 gets merged, you'll probably also need to add an automatic module name to the Guava adapter module being developed there.

@jbduncan jbduncan referenced this issue Apr 2, 2018

Merged

Guava adapter #532

@jkinable

This comment has been minimized.

Copy link
Collaborator

jkinable commented Apr 4, 2018

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

This comment has been minimized.

Copy link

io7m commented Apr 4, 2018

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

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

@jkinable

This comment has been minimized.

Copy link
Collaborator

jkinable commented Apr 4, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Apr 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

jkinable commented Apr 5, 2018

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

This comment has been minimized.

Copy link

io7m commented Apr 5, 2018

Uphill

@jkinable

This comment has been minimized.

Copy link
Collaborator

jkinable commented May 18, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented May 25, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented May 28, 2018

Is there a public discussion thread for the ANTLR release?

@io7m

This comment has been minimized.

Copy link

io7m commented May 28, 2018

Only the PR: antlr/antlr4#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

This comment has been minimized.

Copy link
Member

jsichi commented May 28, 2018

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

This comment has been minimized.

Copy link

io7m commented May 28, 2018

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

This comment has been minimized.

Copy link
Collaborator

d-michail commented May 29, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented May 29, 2018

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

This comment has been minimized.

Copy link
Member

jsichi commented May 29, 2018

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

@jsichi jsichi added the enhancement label Jun 19, 2018

@jsichi

This comment has been minimized.

Copy link
Member

jsichi commented Dec 5, 2018

Looks like there's finally some movement on this:

antlr/antlr4#2395

@d-michail d-michail referenced this issue Dec 21, 2018

Merged

Upgraded antlr to 4.7.2 #731

7 of 7 tasks complete
@d-michail

This comment has been minimized.

Copy link
Collaborator

d-michail commented Dec 21, 2018

@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

This comment has been minimized.

Copy link

io7m commented Dec 21, 2018

Great!

I'm checking the dependencies now.

@d-michail

This comment has been minimized.

Copy link
Collaborator

d-michail commented Dec 21, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Dec 21, 2018

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

@io7m

This comment has been minimized.

Copy link

io7m commented Dec 21, 2018

@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

This comment has been minimized.

Copy link
Collaborator

d-michail commented Dec 21, 2018

Good point

@jsichi jsichi closed this in #731 Dec 25, 2018

@jsichi

This comment has been minimized.

Copy link
Member

jsichi commented Dec 25, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment