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

Add Automatic-Module-Name values #458

Merged
merged 2 commits into from Feb 11, 2018

Conversation

Projects
None yet
3 participants
@io7m

io7m commented Dec 15, 2017

This sets the following module names:

jgrapht-core → org.jgrapht.core
jgrapht-ext → org.jgrapht.ext
jgrapht-io → org.jgrapht.io

This gives the modules stable names in preparation for possible full
modularization.

Affects #448

Add Automatic-Module-Name values
This sets the following module names:

  jgrapht-core → org.jgrapht.core
  jgrapht-ext  → org.jgrapht.ext
  jgrapht-io   → org.jgrapht.io

This gives the modules stable names in preparation for possible full
modularization.

Affects #448
@io7m

This comment has been minimized.

io7m commented Dec 15, 2017

This is the simplest route to making jgrapht compatible with JDK 9 modules. I'm still investigating full modularization, but that involves a few dependency updates and some other changes.

@d-michail

This comment has been minimized.

Collaborator

d-michail commented Dec 18, 2017

Thanks!

There are two more packages, demo and touchgraph.

@jsichi, @jkinable Not sure whether the modules should be called org.jgrapht.core or org.jgrapht.jgrapht-core? What do you think?

@io7m

This comment has been minimized.

io7m commented Dec 18, 2017

There are two more packages, demo and touchgraph.

I left those out because I didn't think people would depend on them as modules (although I think I was wrong with regards to touchgraph!)

Not sure whether the modules should be called org.jgrapht.core or org.jgrapht.jgrapht-core

I'd advise against putting a hyphen in the name. Module names can't contain hyphens at the language level (but there are fewer restrictions at the VM level). With a hyphen in the name, noone would be able to write a requires clause in their module-info.java file...

@jkinable

This comment has been minimized.

Collaborator

jkinable commented Dec 18, 2017

I would go for the org.jgrapht.core/org.jgrapht.demo/etc style.

@jkinable

This comment has been minimized.

Collaborator

jkinable commented Feb 11, 2018

@io7m just to finish this pull-request:

  • could you also add the module names for touchgraph? Would it be bad to add the module names for demo too? If there's no disadvantage, I would recommend to add demo too, even though it's highly unlikely that people will import the 'demo' module

  • could you state what needs to be done in order to make jgrapht fully compatible with JDK 9 modules? In fact, it would be great if you could write a separate pull request in which you make jgrapht fully compatible. Here you can assume that jgrapht requires JDK 9 or above. We will not immediately merge this new PR, but we will keep it around for the future (it happened before that we had a PR that could only be accepted a year or so after its submission due to an issue with the ownership of a proprietary algorithm).
    This is probably easier to write than some 'intermediate' work-around solution.

@io7m

This comment has been minimized.

io7m commented Feb 11, 2018

Hello!

could you also add the module names for touchgraph? Would it be bad to add the module names for demo too?

I'll do this today. There's no disadvantage to adding the demo module, so that'll be included.

could you state what needs to be done in order to make jgrapht fully compatible with JDK 9 modules?

I'll have to go back and check the work I did. I think it's really just a matter of making sure that all dependencies are modularized first, writing module descriptors, and then testing that everything works.

Add Automatic-Module-Name values
This sets the following module names:

  jgrapht-demo       → org.jgrapht.demo
  jgrapht-touchgraph → org.jgrapht.touchgraph

This gives the modules stable names in preparation for possible full
modularization.

Affects #448
@io7m

This comment has been minimized.

io7m commented Feb 11, 2018

You can see the full modularization work I'm doing in my jdk9 branch:

https://github.com/io7m/jgrapht/commits/jdk9

I first decided to clean up the pom.xml files by putting all dependency and version information in one place. This makes it easier to bring all of the plugins to their latest versions in one step, rather than having a mix of plugin versions across the project.

I also needed to update the version of commons-lang to its latest version (because the latest version of commons-lang has an Automatic-Module-Name entry). Unfortunately, this broke a couple of tests: I think the tests are comparing XML and the resulting XML differs in whitespace content when using the latest commons-lang.

Also, there's an issue in that the benchmark tests use generated code that contains @Generated annotations. This annotation has been removed on JDK 9 (and replaced with something else). I think upgrading to the latest version of jmh will fix this.

With those issues out of the way, it should be easy to write module descriptors and have full modularization. I'll work on this today.

@jkinable

This comment has been minimized.

Collaborator

jkinable commented Feb 11, 2018

@io7m sounds great! I'm looking forward to this change. In the mean time, I will merge this one. Thanks for the contribution.

@jkinable jkinable merged commit 7b43a83 into jgrapht:master Feb 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@io7m io7m referenced this pull request Feb 11, 2018

Open

JDK 9 Modularization #484

9 of 10 tasks complete
@d-michail

This comment has been minimized.

Collaborator

d-michail commented Feb 11, 2018

@io7m I have already fixed the issues that you are referring to. You should pull our latest master branch and continue from there.

koppor pushed a commit to winery/jgrapht that referenced this pull request Jun 12, 2018

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