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

Support Java 9 Jigsaw support implementing module-info.java file #1979

Open
1 of 2 tasks
bcmedeiros opened this issue Dec 11, 2020 · 24 comments · May be fixed by #3222
Open
1 of 2 tasks

Support Java 9 Jigsaw support implementing module-info.java file #1979

bcmedeiros opened this issue Dec 11, 2020 · 24 comments · May be fixed by #3222
Labels
jdk/jpms Java Platform Module System

Comments

@bcmedeiros
Copy link

I'm submitting a ...

  • bug report
  • feature request

Describe the issue
I'm trying to convert a app to be packaged using jlink/jdeps, and PostgreSQL is my only dependency that does not provide module information. That is the error due to the fact that PostreSQL is an automodule:

brunomed@brunojcm-macbook $ jdeps --module-path ./eventstore-postgresclient/target/postgresclient-1-local-SNAPSHOT.jar:./eventstore-common/target/common-1-local-SNAPSHOT.jar:./eventstore-serialization/target/serialization-1-local-SNAPSHOT.jar --add-modules p.eventstore.postgresclient co.c.p.eventstore.Bookmark
Exception in thread "main" java.lang.module.FindException: Module postgresql not found, required by p.eventstore.postgresclient

Would you consider implementing support for that? I can propose a PR for that in case you do.

Driver Version?
any

Java Version?
11

OS Version?
any

PostgreSQL Version?
any

@bcmedeiros bcmedeiros changed the title Support Java 9 Jigsaw providing module-info.java file Support Java 9 Jigsaw support implementing module-info.java file Dec 11, 2020
@davecramer
Copy link
Member

Don't see why not. Seems innocuous enough

@bcmedeiros
Copy link
Author

I started to work on this feature today, @davecramer, seems doable but we would have to bump the JDK requirement to build the project to Java 11 (Java 9 to be more precise, but I don't think a non-LTS is an good idea). Of course we could still be building jars compatible with Java 6/7/8, we would be just changing the requirements for contributors and CI.

How reasonable is that?

@bokken
Copy link
Member

bokken commented Dec 13, 2020

Next release is dropping support for Java 6 and 7.
@vlsi and @davecramer need to weigh in, but personally I am +1 to building with jdk 11 (targeting Java 8).

@rbygrave
Copy link
Contributor

but we would have to bump the JDK requirement to build the project to Java 11

Note that we don't need to go to Java 9+ if we instead build a multi-version jar. If we were using maven we could use the org.moditect:moditect-maven-plugin to do this and continue to build the project with 8 (as it uses asm to generate the java 9 bytecode for module-info). I have done this on a number of projects that needed to continue compiling to java 8. I am not sure how to do this with Gradle but maybe someone else does.

@jorsol
Copy link
Member

jorsol commented Dec 30, 2020

I'm not an expert in JPMS but the driver already defines an Automatic-Module-Name in the manifest and should work with it. I just noted that your jdeps example doesn't link to the driver.

Just for the record, I have played a little with a sample project, and adding a module-info.java is not trivial, not at least if we want to do it correctly, meaning that we should refactor the code to be modularized in the recommended way.

@jorsol jorsol added the jdk/jpms Java Platform Module System label Feb 18, 2021
@rackaracka
Copy link

I didn't see that this issue is still open, I raised one here #2657

The issue is that automatic modules cannot be used with jlink.

I don't think that adding a module-info requires refactoring of the code. I generated a proper module-info for this project (see issue 2657). This one just needs to be added to the code, no other changes are required. A multi-release jar would allow to still support Java 8.

@vlsi
Copy link
Member

vlsi commented Nov 2, 2022

I think we could bump the minimum Java to 11 for build purposes.
Then we would use --release 8 for the main compilation, and we would compile module-info.java with the same Java 11.

@davecramer
Copy link
Member

I'd want to check with the packagers to make sure they have java 11 available on debian and redhat

@davecramer
Copy link
Member

Both of them have java 11

@cowwoc
Copy link
Contributor

cowwoc commented Mar 29, 2023

Any update on this issue?

@davecramer
Copy link
Member

We would love a PR.

@cowwoc
Copy link
Contributor

cowwoc commented Apr 1, 2023

One step short of a PR (I don't have a coding environment handy at the moment). The following module-info.java worked for me:

module org.postgresql.jdbc
{
  requires java.sql;
  requires java.management;
  requires java.security.sasl;
  provides java.sql.Driver with org.postgresql.Driver;

  exports org.postgresql;
}

Pop it in your project and away we go.

@rackaracka
Copy link

One step short of a PR (I don't have a coding environment handy at the moment). The following module-info.java worked for me:

module org.postgresql.jdbc
{
  requires java.sql;
  requires java.management;
  requires java.security.sasl;
  provides java.sql.Driver with org.postgresql.Driver;

  exports org.postgresql;
}

Pop it in your project and away we go.

The above code is wrong, a service provider does not export its code. The correct module-info should look like this:

module org.postgresql.jdbc {
    requires java.management;

    requires transitive java.desktop;
    requires transitive java.logging;
    requires transitive java.naming;
    requires transitive java.security.jgss;
    requires transitive java.security.sasl;
    requires transitive java.sql;
    requires transitive java.transaction.xa;
    requires transitive java.xml;

    provides java.sql.Driver with
        org.postgresql.Driver;

}

@rbygrave
Copy link
Contributor

rbygrave commented Apr 2, 2023 via email

@bowbahdoe
Copy link

bowbahdoe commented Jun 25, 2023

@rbygrave

The dependency on java.desktop comes from this one method.

https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/geometric/PGpoint.java

  /**
   * Moves the point to the supplied java.awt.Point refer to java.awt.Point for description of this.
   *
   * @param p Point to move to
   * @see java.awt.Point
   */
  public void setLocation(Point p) {
    setLocation(p.x, p.y);
  }

I'm not sure the right course of action, but the options are.

  1. Do a corpus search of some sort. Maybe this method is rarely enough used that it can just be deleted.

  2. Mark java.desktop as requires static. This feels fine since without java.desktop they won't be able to call the method regardless.

  3. Mark java.desktop as requires static transitive. I'm not sure what the exact semantics of this are, but I think it will make dependent modules also have java.desktop available. This feels heavy but it might be fine? The only downside would be that client code couldn't compile on a jdk without java.desktop present. I don't think thats too common, and i'm only 30% sure thats what it would mean.

@cowwoc How goes that PR?

I know there is a cloud over modular java and dealing with Java 8 compatibility is a PITA, but "core" libraries like database drivers are the things that are most needed to make whole dependency trees modularizable.

Today all that means is folks can use jlink, but some of the discussed directions for leyden make use of modules for static exes and there are some interesting build tools built on top of the modules -> image pipeline.

@cowwoc
Copy link
Contributor

cowwoc commented Jun 25, 2023

@bokken

Sorry, I don't have time to work on a PR. I'm up to my eyeballs at work... I worked around this problem by using moditect-maven-plugin to inject module-info.java into the existing library.

I am quite surprised that pgjdbc has a reference to java.desktop.awt. This seems out of scope for a JDBC driver. Hopefully we can just drop the method as you mentioned. Another possiblity would be to extract such classes into a separate/optional JAR.

@rbygrave
Copy link
Contributor

directions for leyden make use of modules for static exes ...

I agree, module-info is going to get more important over time.

The dependency on java.desktop comes from this one method. ... PGPoint.setLocation(java.awt.Point)

IMO I think this method should be removed at some point. The question is should it be deprecated for some period first? I suspect very little code uses that method at all - for example, PGPoint is not even used by net.postgis / org.postgis.

Mark java.desktop as requires static

I agree. I think that is the right thing to do while that PGPoint.setLocation() method exists.

org.postgresql.util.PGobject

FYI Also just to note that net.postgis:postgis-jdbc:2.5.1 has org.postgis.PGgeo that directly depends on org.postgresql.util.PGobject. These things all suggest that we really need PGobject to be in an exported package.

Given that it won't be easy or practical to move org.postgresql.util.PGobject then org.postgresql.util package needs to be exported and part of of the long term public API. That probably means that longer term, anything in org.postgresql.util that should be for internal use only should be moved out of that package or made not public.

As a side note / opinion / thought: The process of adding a module-info to a library does tend to highlight all the historic ugly little things in an API like dependencies we don't really want (e.g. java.desktop), dependencies we want to be optional (e.g. requires static java.xml ?) and what we desire as the truely public API (e.g. org.postgresql.util.PGobject). In this sense it's a healthy thing for a library to do regardless of the uptake of modules, jlink, leyden etc.

@davecramer
Copy link
Member

IMO I think this method should be removed at some point. The question is should it be deprecated for some period first? I suspect very little code uses that method at all - for example, PGPoint is not even used by net.postgis / org.postgis.

We can deprecate that.

Given that it won't be easy or practical to move org.postgresql.util.PGobject then org.postgresql.util package needs to be exported and part of of the long term public API. That probably means that longer term, anything in org.postgresql.util that should be for internal use only should be moved out of that package or made not public.

Hmmm... nobody should be using internal functions. That said PostGIS is a pretty important part of Postgres. I'm loathe to play favourites like that though.

It will be interesting to see who else depends on what internally if and when we do this .

@bowbahdoe
Copy link

bowbahdoe commented Jun 26, 2023

I use PGObject so much in my clojure code I'm shocked it would even be considered internal only.

@jorsol
Copy link
Member

jorsol commented Jun 26, 2023

My comment from 2020:

adding a module-info.java is not trivial, not at least if we want to do it correctly, meaning that we should refactor the code to be modularized in the recommended way.

One of the points of using modules is to hide the internals of some packages (better encapsulation), right now for better or for worse (and I don't think it's for better), there is no clear definition of what is internal and what is not, and this leaves us in a complicated scenario where to properly modularize the driver might require to refactor some parts of it.

Ideally, to correctly modularize the driver, we should split functionality into different modules, one module could be for the core JDBC classes, one (or more) module for extensions of the driver (replication, copymanager, geometric data types, etc), and extra care should be taken to not depend on more modules that strictly necessarily, depending on java.desktop module is a big fail for a JDBC driver. A good modularized driver should only depend on java.sql and its transitive modules, no more than that, additional functionality should be built around another module (if the geometric data types really, really require awt, then it's fine as long the geometric data types are on a different module).

There was a discussion started a couple of years ago about this: #2059 and I still maintain that properly modularizing the driver is a bigger effort than just putting a module-info.

The ugly easiest path is to simply export everything and require everything that is currently used.

@davecramer
Copy link
Member

I use PGObject so much in my clojure code I'm shocked it would even be considered internal only.

Can you share how you use it ?

@bowbahdoe
Copy link

(doto (PGobject.)
  (.setType "jsonb")
  (.setValue (json/generate-string txn-data)))

Mostly stuff like this

@bowbahdoe
Copy link

bowbahdoe commented Jun 26, 2023

@jorsol

One of the points of using modules is to hide the internals of some packages (better encapsulation), right now for better or for worse (and I don't think it's for better), there is no clear definition of what is internal and what is not, and this leaves us in a complicated scenario where to properly modularize the driver might require to refactor some parts of it.

This is a very specific definition of "proper". This module cannot be split into sub-modules without breaking user experience.

I'm willing to bet that someone, somewhere, has a curriculum for students or a production setup that relies on physically downloading the published jar and putting it on the classpath. A "properly modularized" postgres driver by your considerations would require multiple jars.

The ugly easiest path is to simply export everything and require everything that is currently used.

I think this is the right path. requires static whatever we can sure - but this artifact should stay self contained.

There are two parties to a modularization

  • downstream consumers
  • maintainers

As it is, if you have any non-modularized dependency the whole "module" toolchain (jlink -> j*) just doesn't work (afaik). So while hypothetically a downstream consumer might benefit from splitting up the driver into multiple modules (and that overlaps with what would be nice from a maintenance perspective) the more pressing issue is that incompatibility.

@cowwoc
Copy link
Contributor

cowwoc commented Jun 26, 2023

This is a very specific definition of "proper". This module cannot be split into sub-modules without breaking user experience.

I'm willing to bet that someone, somewhere, has a curriculum for students or a production setup that relies on physically downloading the published jar and putting it on the classpath. A "properly modularized" postgres driver by your considerations would require multiple jars.

Yes, I think splitting the code up into multiple JARs would provide the best cost/benefit over the long-term. That said, nothing prevents you from offering an all-in-one non-modular JAR alongside that if you so wish.

It is likely that the major version number would increase, to signal a backward-incompatible change. So the theoretical professor wouldn't get caught off guard.

That said, we have yet to establish how many people would be negatively affected by this change. Arguing in the abstract tends to end badly :)

As it is, if you have any non-modularized dependency the whole "module" toolchain (jlink -> j*) just doesn't work (afaik).

org.moditect:moditect-maven-plugin solves that problem, though obviously the less dependencies have this problem the easier the process will be. As more libraries adopt Java Modules, the number of workarounds will shrink. Everyone's projects are different, but in my own case the majority of dependencies have already adopted them.

@Sineaggi Sineaggi linked a pull request Apr 19, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jdk/jpms Java Platform Module System
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants