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

Enable JDK9+ module-info #519

Open
skjolber opened this issue Oct 18, 2019 · 18 comments · May be fixed by #722
Open

Enable JDK9+ module-info #519

skjolber opened this issue Oct 18, 2019 · 18 comments · May be fixed by #722
Milestone

Comments

@skjolber
Copy link

I'm trying to use modules in a support library, but there seems to be no module-info for jjwt. I suggest following either Jackson's or SLF4J's approach.

@lhazlewood lhazlewood changed the title Add module-info Enable JDK9+ module-info Oct 18, 2019
@skjolber
Copy link
Author

First add a default module name, then later module info (a bigger job).

@stale
Copy link

stale bot commented Dec 20, 2019

This issue has been automatically marked as stale due to inactivity for 60 or more days. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale Stale issues pending deletion due to inactivity label Dec 20, 2019
@skjolber
Copy link
Author

Bump

@stale stale bot removed the stale Stale issues pending deletion due to inactivity label Dec 20, 2019
@lhazlewood
Copy link
Contributor

@skjolber now that 0.11.0 is out and builds/works on JDK 11, is this still needed?

@skjolber
Copy link
Author

skjolber commented Feb 6, 2020

@lhazlewood can't seem to find any module descriptor or automatic module name entry in the manifest? Ref https://repo1.maven.org/maven2/io/jsonwebtoken/jjwt-impl/0.11.0/

@lhazlewood
Copy link
Contributor

@skjolber correct, we didn't add them to the release - I was curious however if they are still necessary to use in JDK11 given that our builds run and work on JDK11.

@lhazlewood lhazlewood added this to the 0.12.0 milestone Feb 6, 2020
@skjolber
Copy link
Author

skjolber commented Feb 9, 2020

So disabling the module system is possible, and then all will run 'as before'. But that is hardly proper support. You might be interested in this: https://dzone.com/articles/automatic-module-name-calling-all-java-library-maintainers

I have best experience with using the moditech plugin, see this example.

@andrewhrytsiv
Copy link

Hi @lhazlewood. Is there a due date for the 0.12.0 milestone?

@lhazlewood
Copy link
Contributor

@andrewhrytsiv Hi! All I can say as an unpaid team of volunteers, we move as quickly as we're able, so we cannot commit to any dates or timelines.

I will say however that there has been a tremendous amount of active work in the last 3 months on the jwe branch and other PRs, so all I can say is that 1) we're working as hard as we can and 2) there are a lot of awesome things about to drop in the next release as soon as we can get it out. I hope that helps!

@andrewhrytsiv
Copy link

@lhazlewood I appreciate your work. Thank you for the feedback.

@bmarwell
Copy link

@skjolber @lhazlewood I could create such a file using the moditect plugin if you are interested.
I did the same for snakeyaml: https://blog.bmarwell.de/2020/12/08/use-snakeyaml-in-a-modular-jlink-distribution.html

WDYT? We would even still be Java 7 compatible if you needed that.

@lhazlewood
Copy link
Contributor

@bmarwell that would be great! Any help here would be much appreciated.

@bmarwell
Copy link

@lhazlewood I do have a local sketch-up now, but various things will fail.
Some Groovy tests are now restricted to the same level as Java is, e.g. DecodersTest will instantiate new Decoders() which will not be possible anymore.
Another one is access to Base64.DEFAULT (which is package-private and fails with 'no such property exception').

Or the call to this package-private constructor in another test:

def decoder = new ExceptionPropagatingDecoder(new Decoder() { … }

There are 42 (of course!) of those in the api project alone.

Anyway, moditect is Java 8 and the jsonb-extension is also java8-only. That means that you wouldn’t be able to verify every feature using Java 7 anymore and release can only be done on Java 8 and above.

I will look further into it, but I am still far away from creating a branch with helpful commits.

bmarwell added a commit to bmarwell/jjwt that referenced this issue Apr 20, 2022
… IT.

- uses lots of more profiles
- requires Java 8+ for a release
- requires Java 9+ for running the integration test
- does not deploy integration tests
- maven.compiler.release is set instead of source and target for Java9+
- maven.compiler.source/target is now only set on Java7 and Java8 so the javadoc and sources plugin will work correctly.
- Does not contain ITs for neither GSON nor org.json.
- Ugly workaround for the fact that the old bundle-plugin crashes on Java9+ module-info.java files,
  had to provide an empty static MANIFEST.MF file and disable the bundle-plugin on ITs.
- I found it odd that the extensions do not reference their correct parent pom as parent pom, instead the root is chosen.
  This is probably a maven anti-pattern and was corrected by khmarbaise for Shiro.
- parentLocation = ../pom.xml is redundant for /api and /impl, but did not bother to correct it here
- some pom.xml files were missing a final line break as required by unix spec.
bmarwell added a commit to bmarwell/jjwt that referenced this issue Apr 20, 2022
bmarwell added a commit to bmarwell/jjwt that referenced this issue Apr 20, 2022
@lhazlewood
Copy link
Contributor

@bmarwell makes sense. I'm fine waiting for 1.0 to introduce these changes. I don't want to cause any more churn than is necessary. jwe is by far the highest priority, so anything else that requires more than trivial changes to the codebase or project configuration is best left for after that release.

@cowwoc
Copy link

cowwoc commented Mar 14, 2024

I'm not sure why you guys made this more complex than it has to be. You just need to add a few lines to pom.xml to turn your releases into multirelease JAR files and you're done. Here is the code I lifted from https://medium.com/@ankitagrahari.rkgit/multi-release-functionality-8fc423b6c94e:

<execution>
    <id>java11</id>
    <goals><goal>compile</goal></goals>
    <configuration>
        <release>11</release>
        <compileSourceRoots>${project.basedir}/src/main/java11</compileSourceRoots>
        <multiReleaseOutput>true</multiReleaseOutput>
    </configuration>
</execution>

No need to mess around with Moditect or any 3rd-party plugin. Don't get me wrong, Moditect is a great plugin, but there is an easier way to get this done in this case...

@bmarwell
Copy link

No need to mess around with Moditect or any 3rd-party plugin. Don't get me wrong, Moditect is a great plugin, but there is an easier way to get this done in this case...

Yes, but back then it was probably necessary because jjwt did support very old Java versions (at least 7, maybe even 6) and needed to be compile on exactly java 8. Using MRJars would have meant introducing toolchains which is even more configuration compared to moditect which runs on 8. 😃

But yes,, the MR is now probably the better solution.

@lhazlewood
Copy link
Contributor

@cowwoc JJWT still supports JDK 7, so if you'd like to open a PR that supports JDK 7 through 21, we'd appreciate it! 😉

@cowwoc
Copy link

cowwoc commented Mar 16, 2024

@lhazlewood Sorry. No time at the moment... I'll revisit this eventually but it could be months away.

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

Successfully merging a pull request may close this issue.

5 participants