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

Add module-info support #3222

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Sineaggi
Copy link

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does ./gradlew styleCheck pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Fixes #1979, #2657

  • Adds module-info.java file.
  • Adds new black-box test suite to verify the jar we created is modular.
  • Bumped a few dependencies to get updated automatic-module-names or modular jars (waffle, osgi).
  • Didn't upgrade scram version, as it's shaded in the released jar.
  • Added a new sourceSet that compiled the module-info.java file.

@davecramer
Copy link
Member

Unfortunately we need to release java 8.
Until that changes I don't see how this works ?

api("com.github.waffle:waffle-jna:1.9.1")
api("org.osgi:org.osgi.core:6.0.0")
api("org.osgi:org.osgi.service.jdbc:1.0.0")
api("com.github.waffle:waffle-jna:3.3.0")
Copy link
Member

Choose a reason for hiding this comment

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

Is this change required for module?

Copy link
Author

Choose a reason for hiding this comment

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

As-is, the dependencies don't have automatic module names, which causes compilation failure. The versions I bumped to do.

Copy link
Author

Choose a reason for hiding this comment

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

I could downgrade and remove the static requirements

  requires static com.sun.jna;
  requires static com.sun.jna.platform;
  requires static waffle.jna;
  requires static org.osgi.service.jdbc;
  requires static osgi.core;

from the module, but then if those classes (like PGBundleActivator) are loaded you'd see an exception

Exception in thread "main" java.lang.IllegalAccessError: superinterface check failed: class org.postgresql.osgi.PGBundleActivator (in module org.postgresql.jdbc) cannot access class org.osgi.framework.BundleActivator (in unnamed module @0x4fca772d) because module org.postgresql.jdbc does not read unnamed module @0x4fca772d

Copy link
Author

Choose a reason for hiding this comment

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

We could leave the requirements out and just mark waffle and osgi as non-required under modules. Not sure how that would affect future modular versions of waffle or osgi.

@@ -218,6 +243,7 @@ tasks.configureEach<Jar> {
manifest {
attributes["Main-Class"] = "org.postgresql.util.PGJDBCMain"
attributes["Automatic-Module-Name"] = "org.postgresql.jdbc"
Copy link
Member

Choose a reason for hiding this comment

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

Should Automatic-Module-Name be removed if we move to non-automatic modules?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes. Actually, I was going to make a comment about this block. Doesn't this configure every jar, including javadoc and sources? Perhaps these should only be made in the main and shadow jar?

requires static org.osgi.service.jdbc;
requires static osgi.core;

exports org.postgresql;
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need to declare more packages for the exports

Copy link
Author

Choose a reason for hiding this comment

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

We could mark the whole module as open, or try to be more selective. Is there a list of packages that should be exported?

@Sineaggi
Copy link
Author

Sineaggi commented Apr 19, 2024

@davecramer

Unfortunately we need to release java 8. Until that changes I don't see how this works ?

This created a multi-release jar meaning the main jar targets 8+, and the module-info.class file is only visible to java versions 11+.

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.

Support Java 9 Jigsaw support implementing module-info.java file
3 participants