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

Java modules #640

Open
robertvazan opened this issue Dec 28, 2021 · 6 comments
Open

Java modules #640

robertvazan opened this issue Dec 28, 2021 · 6 comments

Comments

@robertvazan
Copy link
Collaborator

I have been very busy during the last few months, so I couldn't contribute to WDTK. I would like to resume contributions, but meantime my projects moved to Java 11 or 17 and they are now all Java modules, which makes it much harder to test WDTK with them. So I would like to open discussion about how to introduce JPMS in WDTK. I see several options:

  1. Do nothing. Cons: Downstream libs and apps will see warnings about unstable automatic module name. Dependency resolution in workspace does not work, so libs/app cannot be easily tested with modified WDTK.
  2. Add Automatic-Module-Name to MANIFEST.MF. This will fix the warning about unstable module name. Cons: Workspace dependency resolution still does not work.
  3. Create multi-release JARs that target both Java 8 and Java 11. Java 11 version will have module-info.java. Cons: Complicated, poorly supported by Maven, likely unsupported in workspace dependency resolution, and likely to unearth a ton of bugs in various tools.
  4. Upgrade to Java 11 and add module-info.java. This is easy and trouble-free on WDTK side. Workspace dependency resolution works. Cons: Some downstream libs and apps are probably still on Java 8. Some dependencies of WDTK might not support JPMS yet, which will result in some warnings.

I am obviously in favor of option (4), because it makes my life much easier. It is also possible to upgrade only to Java 9, but AFAIK Java 9 provides negligible improvement in compatibility over Java 11. Java 11 also brings some nice features, notably var, possibly HttpClient. Downstream libs and apps still on Java 8 can likely upgrade to Java 11 without any downsides.

Second best choice is (2). I can still contribute in this case, but I would not be able to test my changes against my own libs and apps due to the broken dependency resolution in workspace. There are workarounds using local repository, but that's unwieldy and unproductive, so I would just submit stuff verified only using unit tests and then fix any integration issues with followup PR.

@Tpt @wetneb Opinion?

@wetneb
Copy link
Member

wetneb commented Dec 28, 2021

Intuitively I would rather not precipitate dropping support for Java 8, so I would start with your option 2. But my understanding of JPMS is still very limited, so I might not fully grasp the benefits it offers.

I am curious about why the fact that your other projects use Java 11 or 17 makes it much harder to test WDTK with them: why is it harder than before? Has it become harder to install a snapshot in your local maven repository?

If it makes a big difference to you in terms of ease of development, would it be imaginable to put a module-info.java in your local clone of WDTK without committing this file to the repository?

There are workarounds using local repository, but that's unwieldy and unproductive, so I would just submit stuff verified only using unit tests and then fix any integration issues with followup PR.

I think it's a rather healthy practice to encourage relying more on unit tests, instead of one-off testing in another environment.

@robertvazan
Copy link
Collaborator Author

@wetneb The usual way to test libraries with my other projects is to just enable workspace dependency resolution in Eclipse. I just open the library (WDTK in this case) in my workspace and all projects in the workspace automatically start using it instead of pulling JARs from Maven Central. It's very convenient, because I get full benefits of the IDE (cross-project refactoring, instant error reports). This worked smoothly before modules, but sadly Eclipse does not support this workflow when module project depends on non-module project (i.e. one without module-info.java).

Deploying to local repository kind of works, but it requires redeployment every time I need to test a small bugfix and I lose IDE integration. Maintaining Java 11 fork of WDTK is even more laborious. I am making no secret of the fact this all for my convenience :-) I don't see any value in Java 8 now that Java 17 is out, but if anyone still values Java 8 compatibility, then let's keep it for now.

I think it's a rather healthy practice to encourage relying more on unit tests, instead of one-off testing in another environment.

Just to explain, unit tests would still be done, but integration testing against real apps is useful to detect issues with API, performance, Wikibase server compatibility, etc.

@wetneb
Copy link
Member

wetneb commented Dec 28, 2021

Thanks for the explanation! For sure it's not a hill I will die on (OpenRefine itself dropped support for Java 8 for other reasons, but there might be other people out there stuck with Java 8 for some reason).

@robertvazan
Copy link
Collaborator Author

After some research, I think multi-release JARs might not be that hard. They are hard when supplying different classes for different JREs, but just adding module-info.java into Java 8 JARs should be easy. There are Maven plugins that can assist with this: modulemaker and moditect. The first one is simpler, but moditect is neat in that it supports standard module-info.java syntax (example) with only short config in pom.xml (example). We can have our cake and eat it too. IDE support for multi-release JARs might still be a bummer though.

@robertvazan
Copy link
Collaborator Author

While experimenting with JPMS in WDTK, I came across blocker for JPMS support. WDTK uses two OkHttp libraries that present themselves as two modules (okhttp3 and okhttp3.urlconnection) but claim the same package (okhttp3). Compiler wouldn't accept that. I have reported it upstream (square/okhttp#4184 (comment)), but fix may take months to get released. I don't know any good alternative to OkHttp. The only workaround is to refrain from using JPMS in WDTK and downstream projects until the OkHttp issue is fixed.

@cowwoc
Copy link

cowwoc commented Dec 29, 2021

There are plenty of good alternatives to OkHttp to offer JPMS support. It's just a question of what features you need. jetty-client's design is less pleasing but its implementation is solid. I'm sure there are any others.

In my experience, most/all Google libraries will stay on Java8 forever. This seems to be due to Google politics more than anything else. They have been ignoring JPMS-related issues across the board.

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

No branches or pull requests

3 participants