Skip to content

added jaxb-api so that jdk 9+ classpath works without configuration#8382

Merged
schmitch merged 2 commits intoplayframework:masterfrom
schmitch:better-jdk-10
May 30, 2018
Merged

added jaxb-api so that jdk 9+ classpath works without configuration#8382
schmitch merged 2 commits intoplayframework:masterfrom
schmitch:better-jdk-10

Conversation

@schmitch
Copy link
Contributor

@schmitch schmitch commented Apr 25, 2018 •
edited
Loading

currently this adds the jaxb-api jar (122kb) so that play will work without any flag under jdk 9+.
An alternative would be to either fork jjwt and fix the outstanding issue or use a different JWT implementation.
We could also wait for a fix directly in jjwt however I guess it's fine to add jaxb-api.jar for known since it is only 122kb and than we could run play under jdk 9+ without any flags/problems.

After we merge that Play will still print:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$1 (guice.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$1
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

However this is probably not easy to fix, but can be ignored until we add support for JPMS (which can't work without the help of the Scala Compiler anyway).

(We could also add tests for JDK 10 if scala 2.12.6 hits

@schmitch
Copy link
Contributor Author

schmitch commented Apr 26, 2018 •
edited
Loading

wierd mima issue:

java.lang.RuntimeException: bad constant pool tag 19 at byte 54
	at com.typesafe.tools.mima.core.ClassfileParser$ConstantPool.errorBadTag(ClassfileParser.scala:192)

Guess the problem is, that this is a Multi Version Jar and Mima tries to read the Class under META-INF/versions/9

@schmitch
Copy link
Contributor Author

schmitch commented Apr 27, 2018

Blocked by lightbend-labs/mima#206

@marcospereira
Copy link
Member

marcospereira commented Apr 27, 2018

We could also wait for a fix directly in jjwt however I guess it's fine to add jaxb-api.jar for known since it is only 122kb and than we could run play under jdk 9+ without any flags/problems.

I wonder if we should instead have the flags automatically added by Play plugin when using Java 9 (we would need to do that for sbt, gradle, and mvn though).

@schmitch
Copy link
Contributor Author

schmitch commented Apr 27, 2018 •
edited
Loading

and in production it would not be possible or building with 8 and running with 9+ or we would need to change all launcher scripts, etc. and if assembly is used people need to do it themself.

I guess using a 122kb jar is better/good enough. in the future it can probably be removed. play is already really big, so adding 122kb is not a big deal. Especially since play is a opinionated framework anyway, it should just work.

or we fork jjwt under the playframework banner and remove jaxb from that library and publish it ourself, it's not that maintained anyway. (the library does not need jaxb since it only uses the Base64 decoder which exists under Java8+ (java.base module) anyway)

@wsargent
Copy link
Member

wsargent commented May 8, 2018 •
edited
Loading

We don't need to use jjwt at all -- it's not exposed as a public API. We use JWT but we could easily switch to a different library. Maybe file a PR with https://github.com/jwtk/jjwt about it?

@schmitch
Copy link
Contributor Author

schmitch commented May 8, 2018

Actually jjwt has PRs for the matter:

@schmitch
Copy link
Contributor Author

schmitch commented May 29, 2018

updates mima, so this can be looked at (merge ready)

gmethvin
gmethvin approved these changes May 30, 2018
View reviewed changes
Copy link
Member

@gmethvin gmethvin left a comment

Choose a reason for hiding this comment

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

I'm fine with this at least as a temporary solution until the issue is fixed in jjwt.

@schmitch schmitch mentioned this pull request May 30, 2018
Closed
@schmitch schmitch merged commit 8ef9e47 into playframework:master May 30, 2018
@schmitch
Copy link
Contributor Author

schmitch commented May 30, 2018

I merged it, it's easy to revert and I also added a follow up issue on #8446 to discuss what we could do.

@marcospereira
Copy link
Member

marcospereira commented May 30, 2018

@schmitch I think we should probably backport this to 2.6.x. See playframework/play-scala-starter-example#86.

@schmitch
Copy link
Contributor Author

schmitch commented May 30, 2018

yeah I guess we could, I mean it just adds a dependency, which should not break any builds.

@schmitch
Copy link
Contributor Author

schmitch commented May 30, 2018

Backport is here: #8451

@xuwei-k xuwei-k mentioned this pull request Jun 26, 2018
Closed
1 task
@PromanSEW
Copy link
Contributor

PromanSEW commented Jul 17, 2018

jwtk/jjwt#341

@TimMoore TimMoore added this to the Play 2.7.0 milestone Dec 10, 2018
@benmccann benmccann mentioned this pull request Apr 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmethvin gmethvin gmethvin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Play 2.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@schmitch @marcospereira @wsargent @PromanSEW @gmethvin @TimMoore