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

Update Play source for build compability with JDK9+ #1252

Merged
merged 10 commits into from
Aug 7, 2018
Merged

Update Play source for build compability with JDK9+ #1252

merged 10 commits into from
Aug 7, 2018

Conversation

tomparle
Copy link
Contributor

@tomparle tomparle commented Aug 3, 2018

Hi,

after the work for JDK9 compatibility of #1200, I wanted to check if Play could be built with Java 9 or 10.
This pull request contains the modifications I made to make this compatibility possible : it fixes some deprecated or missing methods, and upgrade a few dependencies, so it prepares Play for future builds with newer JDKs.

Details :

  • removed "source" and "target" from all ant build files so the user can set it through an environment varible
  • libraries update : gson 2.8.0 => 2.8.5, HikariCP 2.7.9 => 3.2.0, cglib 3.2.4 => 3.2.7
  • groovy 2.4.15 => 2.5.1. The previous groovy-all jar package is no more maintained, it is now just a POM which references a like a dozen of jars that Play does not need (they say this is the result of the work in progress for JDK9 full modularization). It seems that only groovy and groovy-xml are needed.
  • hibernate-entitymanager has been removed since it is now included in hibernate-core. See https://github.com/hibernate/hibernate-orm/blob/5.2/migration-guide.adoc#hibernate-entitymanager-merged-into-hibernate-core
  • in GroovyTemplate.java, the method named with an underscore generated a compilation error and has been removed. The hard part for me was to figure out where to replace these calls still existing in compiled Groovy templates.

Additional note : the test DataBinding failed on my machine because the language cookie seems to be ignored for some reason. I will see if it is ok on Travis.

@tomparle
Copy link
Contributor Author

tomparle commented Aug 3, 2018

Additional note : after a syntax test (using the "private" identifier on a method implementation in an interface, a new JDK9 feature), the compilation failed, so :

  • I also updated the libraries JDT and ASM
  • I improved the java source detection mechanism to use the value specified in the application.conf file. Current compatible versions, that match JDT's, are now 1.8, 9 and 10, and I added an explicit error message if the requested source version is not available.

…remove as much compilation warnings as possible
…ithout quotes, because I do not know how to assert an OR condition in Selenium IDE
@@ -64,7 +64,7 @@

<target name="compile" description="compile without cleaning">
<mkdir dir="classes"/>
<javac encoding="utf-8" srcdir="src" destdir="classes" debug="true" source="1.8" target="1.8">
<javac encoding="utf-8" srcdir="src" destdir="classes" debug="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it needed? I guess it's still a good idea to explicitly declare that Play itself is runnable with Java8 (even if somebody occasionally builds it with Play9+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asolntsev thank you for your comments !
I wanted to give more freedom to choose the source and target versions by specifying them as environment variables, especially to test compatibility with building with jdk9 or 10.
Indeed it may be a good idea to keep target and sources at "1.8" for now, I'll make these changes.

@@ -0,0 +1,121 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file needed? I don't see that it's used somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure why this file is still required, ant thought maybe it's required for projects that use Play.
I guess it's still a quick way to check used dependencies, but requires additional maintenance.
If you confirm it's never used, we can safely delete it, but I wonder what was the purpose of this file then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomparle There is two files:

  1. framework/dependencies.yml - it's actually used
  2. framework/conf/dependencies.yml - it's not used. You added this file in your PR.

Yes, file framework/conf/dependencies.yml is not used and should be deleted.

@tomparle
Copy link
Contributor Author

tomparle commented Aug 5, 2018

@asolntsev my bad, I unexpectedly duplicated this file by mistake. Thanks for pointing this out !
I removed it and specified back the source and target compilation versions as requested.

Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

@tomparle Thank you, now PR looks good! I have tested it with Java 8, 9 and 10 - everything works.

I am almost ready to merge it, except one minor comment (see static method __reverseWithCheck).

@@ -521,7 +508,7 @@ public String __reverseWithCheck_absolute_false(String action) {
return __reverseWithCheck(action, false);
}

private String __reverseWithCheck(String action, boolean absolute) {
private static String __reverseWithCheck(String action, boolean absolute) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you made it static? It's private method, it cannot be used from outside. Please make it non-static back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback and having taken the time to test for multiple versions !
I made this method static because my editor automatically notifies me when a method could be declared static (as an utility method), which I did here because it does not use any instance field or method.
No problem if you still prefer to make it non-static back !

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomparle Ok, I know that editor notification. I also used it many years ago.
Now I am pretty sure that static is a bad practice in most cases. For many reasons. The fact that the method doesn't use any fields is not the key factor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asolntsev thanks for the feedback. I removed the static identifier from the method. I'd also be glad if you could share me some reasons of why it can be a bad practice, or give me a link about it !

Also, I just found out a regression that I did not see before, in the #{form} tag of the CRUD module (this module is separated from the framework, which is why I did not see it at first). This has been fixed.

I think it would also be neat to add the following kind of migration note about the removed method "_" in Groovy templates - even if I do not think it has been much used anywhere except from the framework itself (how do we add this to the future release notes ?) :
The shorthand method _(String className) to load a class in Groovy templates has been removed since the underscore is now a reserved keyword in JDK9 and later versions. Please use the method __loadClass(String className) instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tomparle will this restriction of JDK9 on the Groovy templates have a knock on effect on https://github.com/codeborne/play-fastergt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tazmaniax indeed, I proposed some changes to the form and types tag implementation, see codeborne/play-fastergt#1
As I do not use this tag implementation, I wish someone who does could test it in his app.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tomparle I'll test shortly, thx! Out of interest is there a reason why you don't use this rendering implementation as it would appear to be an improvement over the standard implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tazmaniax First, thank you for make me discover this implementation I was not aware of. I guess it's the main raison I was not using it ! But if indeed this is an improvement I will try it for sure.
Another issue is that I have already customized heavily CRUD tags to support additional features, and I guess I would have to merge it with faster GT implementation which would require some work. Anyway, please let me know of the test results !

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've also done some customisation of the CRUD tags and it would be interesting to maybe coalesce them into this project if they would be of value to others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea ! I will see that after I test to integrate them on my project, and tell you if I can add some features useful to others.

@asolntsev asolntsev self-assigned this Aug 7, 2018
…laced by __loadClass because '_' is a reserved keyword from jdk9
@asolntsev
Copy link
Contributor

@tomparle Thank you! Now PR is good, going to merge it.

  1. I don't know how to add it to Play 1 release notes. @xael-fry typically does it somehow.
  2. it's a pity that CRUD module is not covered by automated tests :(
  3. Here are a couple of links about static evilness for well-known guy Yegor Bugayenko. I am not always agree with his points, but these are good enough for initial reading:

@asolntsev asolntsev merged commit 5b4cba2 into playframework:master Aug 7, 2018
@tomparle
Copy link
Contributor Author

tomparle commented Aug 7, 2018

Thanks for the tips @asolntsev !

  1. Ok, let's hope @xael-fry will be notified of the request to update the documentation. If not, I will keep in touch with him before the next release.
  2. I agree. I will create one next time I deal with views
  3. Thanks a lot for these links ! Interesting to read indeed.

@xael-fry
Copy link
Member

@tomparle witch page of the documentation do you want to update.
You can directly update the doc cf https://github.com/playframework/play1/blob/master/documentation/manual/configuration.textile for java source.. should be done in the PR

@xael-fry xael-fry added this to the 1.5.2 milestone Aug 22, 2018
@tomparle
Copy link
Contributor Author

@xael-fry I wanted to update the java compatible versions for the java.version property, adding 9 and 10 as compatible versions. Unfortunately I was unable to add it to the PR because recent merges in the master created conflicts I do not know how to resolve (conflict with dependencies upgrade with another PR).
Additionally, I also wanted to precise the following migration notes :
The shorthand method _(String className) to load a class in Groovy templates has been removed since the underscore is now a reserved keyword in JDK9 and later versions. Please use the method __loadClass(String className) instead

xael-fry pushed a commit to xael-fry/play that referenced this pull request Aug 23, 2018
…cated '_' method, replaced by __loadClass because '_' is a reserved keyword from jdk9

* [playframework#1252] set java source and target version to use for the build, and remove unused conf file created by mistake
* [playframework#1252] ugly fix to make test pass with annotation parameter values with or without quotes, because I do not know how to assert an OR condition in Selenium IDE
* [playframework#1252] fix failing tests due to upgrade to JDK9. Upgrade test libraries and remove as much compilation warnings as possible
* [playframework#1252]  add version 9
* [playframework#1252] update java source detection mechanism to use the version specified in application conf file. Currently supported versions (by jdt compiler) are JDK 1.8 and 10
* [playframework#1252] fix selenium test
* [playframework#1252] fix previous loading class method using _ character now reserved in jdk9+
* [playframework#1252] update Play for build with jdk9+. Update libs
* update documentation for java.source
xael-fry added a commit that referenced this pull request Aug 23, 2018
[#1252] revert back method to static. Fix calls to deprecated '_' met…
@xael-fry
Copy link
Member

@tomparle ok so for java.version I add it.
The 2nd point will be handle in #1261

@tomparle
Copy link
Contributor Author

Thanks @xael-fry !

@jvosloo
Copy link

jvosloo commented Sep 5, 2018

I have nothing technical to add at this point, except to give a HUGE thank you to all you guys for keeping Play1 up to date with the new Java versions. Whenever you are in Berlin - the 🍺 is on me!

@tomparle
Copy link
Contributor Author

tomparle commented Sep 5, 2018

Thank you for this very kind comment @jvosloo !
The 🍻 will be a pleasure 😉

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

Successfully merging this pull request may close these issues.

5 participants