search

jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.55k stars 4.02k forks source link

gradle 5.x support #9081

Closed atomfrede closed 5 years ago

atomfrede commented 5 years ago

Gradle 5 is already a few month old. We should support gradle 5 out of box for jhipster 6. There should be no big changes required, but I am pretty sure we will have some deprecation warnings regarding gradle 6. Need to investigate a little further, but pretty sure these are from some of the plugins we use, so most likely we just need to wait for new plugins releases.

Edit. Should consider removing the apt plugins and use default gradle features https://github.com/gradle/gradle/issues/2300#issuecomment-460065789

pvliss commented 5 years ago

@atomfrede I assume you already have started work on this one but in any case I would be very interested in helping out on this one. BTW are you also considering switching to Kotlin gradle scripts or maybe even consider having it as another user option?

PierreBesson commented 5 years ago

@pvliss, If you are interested to work on this, we can assign the issue to you. As for switching to a Kotlin based gradle build I don't see how it would be an improvement.

If you want to improve our gradle build, there was a great audit by a member of the Gradle team: https://github.com/jhipster/generator-jhipster/issues/6252 and a lot of his ideas were not actually implemented because we moved to other concerns. @atomfrede also started experimental work on a jhipster gradle plugin.

atomfrede commented 5 years ago

@pvliss I also see no value currently in switching to kotlin as the current script with groovy works just fine for us. What will most likely not happen is, that we support both groovy and kotlin. There are a lot of things we can optimize for sure, but nothing urgent imho. The greatest thing would be to make the jhpster gradle plugin reality, if you would like to know more I can tell you in detail more, but the linked issue describes quite good a few points.

Some of them are more or less resolve (e.g. repositories, plugin usage). We still have a lot of -P flags, but some of them won't go away I guess. The testing part could be done know as I remember we had a quite long conversation about it and decided not to split unit and integration tests as it would be a breaking change (but let me check). We have optimized the up-to-date checking for node/frontend builds already, not sure if the boot plugin has still some issues with that, need to check in detail.

EDIT: What we should defnitly do is to cleanup how we apply plugins, check if we can move some plugins from sub build files to the root build script as the plugins can be applied in any case.

EDIT2: And the splitting of "real" units tests and integration makes totally sense to me.

pvliss commented 5 years ago

@atomfrede + @PierreBesson Thanks for your input.

So the benefit of using Kotlin build scripts is the enhanced editing experience(code completion, refactoring, etc. ) as is also outlined here and to be honest the thing I miss the most when switching between Maven and Gradle. But is currently limited mainly to IntelliJ IDEA users and I realize that it is not so urgent or everyone will be happy with it. So I agree with you that currently there is not big benefit and of-course as is it would create a maintenance burden to have both Groovy and Kotlin

I will definitely take a look at the linked issue and work done already by @atomfrede for the plugin but since I have not written a gradle plugin before I can not promise anything. Then again I am always up for a challenge and currently have some spare time to dedicate to it. To that end, it would really help if someone could make a task list of some sort of to know what has already been done in order to save some time.

So what I purpose if you don't mind is the following:

  1. Upgrade to Gradle 5.x as is in order to avoid also halting jhipster 6.
  2. Try to implement as many of the ideas not already done from #6252
  3. Consider creating a gradle plugin in order to also abstract all the complexity from the users and out of the main generator. Then, if done right, we could possibly offer both options(groovy or kotlin)

What do you think?

atomfrede commented 5 years ago

Sounds like a plan. I have just tried to make a list of things I would like to change in the short term to make our gradle script cleaner and the build better:

pvliss commented 5 years ago

Nice!!! Just let me know what you want me to help with

pvliss commented 5 years ago

@atomfrede Hello again.

So I took the liberty of doing some performance tests last night of the current gradle build process in order to be able to compare any new changes against the old build script. I was about to put them in a google doc spreadsheet and realized I had some numbers the same( I guess that's what you get when working tired). Anyhow, I will redo them now and post back some results and some immediate improvements that we can make to improve the build.

So in order to get as much as I could in there and have a reasonable build time I followed the following process:

  1. Get ngx-default as the base
  2. Switch build tool to gradle
  3. Add elastic search
  4. Add Cucumber tests

Finally I used the sql sample for entities.

The comparison will include 3 runs for each of the following combinations:

  1. JHipster 5.7.3 (master) + Spring Boot 2.0.7 + Gradle 4.2.10 + JDK 8
  2. JHipster 5.7.3 (master) + Spring Boot 2.0.7 + Gradle 5.1.1 + JDK 8
  3. JHipster 6.x (sb2.1 branch) + Spring Boot 2.1.8 + Gradle 4.2.10 + JDK 8
  4. JHipster 6.x (sb2.1 branch) + Spring Boot 2.1.8 + Gradle 5.1.1 + JDK 8
  5. JHipster 6.x (sb2.1 branch) + Spring Boot 2.1.8 + Gradle 5.1.1 + JDK 11

The first run will be a clean full build, the 2nd one will be a simple build and the third one a simple build with parallel enabled

Then I will probably perform some minor changes and re-compare some of the combinations(most likely 2, 4 and 5). After you perform all the required changes will can also re-check and then maybe with the plugin, etc.

What do you guys think? Anything that I am missing e.g. some other combination that might be useful?

pvliss commented 5 years ago

OK. So it will take some time to post the results as I found a better way to profile the build than doing it manually. After reading the Gradle Performance Guide it seems there is a way to automate the profiling process by the use of the Gradle Profiler.

I have created a scenario file and a bash script to help me automate the process a bit more for the combinations mentioned in my previous comment. Let me know if you want me to post a gist or something of the script. BTW, currently using the benchmark option and decreased the warm ups and iterations to half of the defaults

pvliss commented 5 years ago

Hello again and sorry for the delay in posting the results.

@atomfrede Did you have any chance to look at this yet? Also, more of what I am about to write is mainly related to the performance of the build rather than the actual Gradle 5.x upgrade whgich is the main task of this issue. Maybe it would be more appropriate to move the performance discussion to another issue or even re-open #6252 and post there?

Benchmarks

So I have used the Gradle Profiler as explain in my last comment as it provides much more accurate results and helps automate the process considerably. I have uploaded my findings to my google drive here. You can find the benchmark results for each configuration by going into the respective folder and I have also added the bash scripts I used to further automate the process. I will also do another benchmark with the changes that I have done to improve the build speed. I was also keen to the idea of merging all these together but I think it would be better to extend gradle-profiler to execute scenarios using different project dirs and have them placed nicely into one single report file.

So the gist of the performance results is the following:

  1. Using --parallel in general does not seem to improve performance which was kind of expected as it is a single module project and currently not many tasks that can actually run in parallel. I guess if you separate unit and integration tests that could be a candidate. Nervelessness, it seems to actually improve speed by some seconds when used with JDK 11.
  2. Upgrading to Gradle 5.x does not make a difference performance wise. Times are about the same.
  3. Upgrade to Spring Boot 2.1.x does actually improve the initial run(i.e. warm up # 1) performance and slightly by a few seconds the mean performance.
  4. Upgrade to Java 11 also improves performance by a few more seconds(bear in mind that is also combined with SB 2.1.x) and moreover when used with --parallel.

In summary, upgrading to Gradle 5.x, Spring Boot 2.1.x and Java 11 and using --parallel can drop performance from ~2 mins to ~1min and 36s which is a considerable gain even more so for development.

Build improvements

After reading the comments of @oehme at #6252 what struck me the most was the part that

Tasks are always out-of-date ... - the build script contains processResources.dependsOn cleanResources. Gradle is actually very good at detecting and cleaning stale output files, so this might be a historic artifact...

which actually means that the code is actually performing a clean build everytime, i.e. same as executing: ./gradlew clean build

Hence, I started investigating and fixed some issues in the build which resulted to build time dropping down to ~3 seconds using the GRadle 5.x, SB 2.1.x and JDK 11 configuration. These include:

@atomfrede As I do not know what changes you have already done. Would you like me to post a PR and maybe on a different issue or have them pasted as code blocks in another comment?

EDIT: Forgot to mention that these changes are applicable to current master and Gradle 4.2.x as well and do not require an upgrade EDIT 2: Here's a build scan after applying the changes. Will do a benchmark as well and update the results EDIT 3: I have now generated and uploaded reports with the optimizations and I have also bumped the iterations to 10 as it is now much faster to execute them. The results are indeed ~3 seconds :champagne: . I have also pushed the changes to a new branch at my fork and lets hope that all CI tests pass too :pray: . Here's the commit for anyone interested to check/test them out.

atomfrede commented 5 years ago

@pvliss Nice investigations, great work, thx! If you have already the code I would suggest you apply a PR, so everyone can review at and we have our regular build pipeline in place. I would say let's move forward stepwise, so first some optimizations to further decrease build times, upgrade to gradle 5 (maybe after sb 2.1 is on master?) afterwards lets check how to further improve the gradle build script or maybe invest some time in a jhipster plugin.

pvliss commented 5 years ago

@atomfrede Thanks, glad to be of help and really enjoyed this one(I am a sucker for perf related issues). Sounds like a good plan. I agree that next step is updating to Gradle 5.x and the rest of the items you listed at https://github.com/jhipster/generator-jhipster/issues/9081#issuecomment-455259591 so that we have a final clean build before moving on to migrate to a gradle plugin. BTW, any chance that the sb 2.1 branch will be merged to master soon?

jdubois commented 5 years ago

As JHipster 6 is getting close, I believe this is an important feature: @atomfrede @pvliss do you think this can be done in the next few weeks? This won't block our beta releases so we can still do them without this.

pvliss commented 5 years ago

@jdubois + @atomfrede I could work on this if @atomfrede hasn't already or does not have the time. Just let me know

atomfrede commented 5 years ago

Yes the first upgrade can be done. There will be some deprecation warnings (sadly we can't do much about them right now), but I can live with them. @pvliss If you find time go ahead. I would also suggest we replace the dependency scope compile with implementation (same for test etc).

pvliss commented 5 years ago

@atomfrede OK, I will start today. Should not be that difficult.

BTW, @pascalgrimaud while I have not checked what's changed should #9328 be also merged to avoid any possible conflicts?

atomfrede commented 5 years ago

Regarding https://github.com/jhipster/generator-jhipster/pull/9328 we are basically waiting on the maven part, but from my point of view we do not need to have the exact same behavior with maven/gradle

raychenon commented 5 years ago

Hello everyone, I have time to contribute on Gradle support and I'm catching up with the history and related tickets.

Ray

pvliss commented 5 years ago

Hello @raychenon and thanks for offering to help out. I have already worked on this and just submitted PR #9381. Once @atomfrede reviews it then I guess you can also help out with any other tasks remaining. Lets wait for @atomfrede who is the gradle stream lead

pvliss commented 5 years ago

@atomfrede It seems that Gradle now supports BOMs natively. I tested and made it work in a separate branch(based on #9381) but I have a few notes that I am not sure about : 1) The io.spring.dependency-management plugin is no longer needed and I have removed it. Removing this will also need to remove the dependencyManagement {} block. There is a needle in there jhipster-needle-gradle-dependency-management which I have also removed as I did a search in the code base and I did not find any usages(Kotlin blueprint also does not use it) but this is a backwards incompatible change that may affect other modules, blueprints or apps. 2) The versions of dependencies org.hibernate:hibernate-jpamodelgen and spring-boot-configuration-processor have to be explicitly defined or otherwise io.spring.dependency-management plugin needs to remain.

Another thing I would like to ask you is if its ok with you to convert all usages of single quotes to double as they are currently mixed and I think it would be best to stick with one style and we need double for variable interpolation?

atomfrede commented 5 years ago

@pvliss Great findings. I didn't test the bom support yet but it seems to work quite well. So if we can get rid of some plugins that would be great. As we do this for jh6 we can introduce breaking changes imho (and we have already).

Another feature we might exchange a plugin with native code is annotation processing, in fact the plugin is a noop plugin when using gradle 5.1. But thats nothing I would rush.

And yes unifying the quotes was also on my list. :+1:

pvliss commented 5 years ago

@atomfrede So should I push changes for native BOMs and double quotes to #9381 as well?

I did actually read about the annotation changes as well and checked out the apt plugin but the only thing that prevented me going forward was the fact that there is no replacement for eclipse users .See here: https://github.com/tbroyer/gradle-apt-plugin#do-without-netltgtapt-eclipse

atomfrede commented 5 years ago

Maybe push the changes for the quotes here and the bom part in another PR, easier to review and test. Yes, thats why I would not rush the apt plugin changes. But as far as I understand the eclipse plugin will work now and most properly in the future too and if I got it correctly it can be used and should only affect eclipse, but in case we find there is no viable solution right now, I am totally ok with keeping the apt plugins for now.

pvliss commented 5 years ago

Sure I will create separate PRs for the other changes.

@raychenon also offered to help. Maybe he could look into the apt plugin changes on a separate PR as well?

Maybe we should also create a checklist to keep track of what is done and by whom to avoid conflicts. What do you think?

raychenon commented 5 years ago

@pvliss , I can replace the single quotes to double quotes :) . If you've already done it, push it.

Tasks left :

pvliss commented 5 years ago

@raychenon Yes, I have done both native BOMs and double quotes and will create PRs tomorrow.

@atomfrede at his comment referenced some more although some have already been done(e.g. separation of Int tests). I also had a look at https://docs.gradle.org/current/userguide/upgrading_version_4.html but maybe missed something.

BTW, moving all plugins to the new plugins{} format(falls in the Cleanup and align plugin application item) would be nice but it seems that versions need to be specified as literals(tested earlier today) , hence we can not use the springboot_versionand that might not be desirable?

atomfrede commented 5 years ago

@pvliss The openapi plugin is not yet in the gradle plugin portal, so that why it is still in buildscript block. And totally correct with springboot_version, thats why the boot plugin is still in buildscript block :)

Regardign the native annotation processor stuff. When I got it correctly currently idea does not forward to gradle by default (will be from the upcoming 2019 version). So maybe we should keep the plugins for now, but maybe let's give it a try and see how it goes. At least we mention on out documentation to froward all ide actions to gradle.

I would also align our dev tool support https://docs.spring.io/spring-boot/docs/current/reference/html/using-boot-devtools.html Furthermore npm install is always out of date. We use the convention task from the node plugin, where we can't add inputs and outputs. So maybe let us add a custom task, e.g. 

task installFrontendDependencies(type: NpmTask) {
    description = "Installs frontend dependencies from package.json"
    inputs.file("package.json")
    outputs.dir("node_modules")
    args = ['install']
}
pvliss commented 5 years ago

@atomfrede Regarding node plugin, I tested your script which while it works it took around 8.5 minutes to complete the first time on my Windows laptop. I have instead tested with https://github.com/node-gradle/gradle-node-plugin which is a fork of https://github.com/srs/gradle-node-plugin as the one used now seems to be stale and with a few changes seems to work just fine. I will create a PR for that except if you want to stick with https://github.com/srs/gradle-node-plugin.

atomfrede commented 5 years ago

Fun fact https://github.com/node-gradle/gradle-node-plugin/issues/2 But as the activity in the fork is also not that high I didn't see any pressure to migrate. I also had a look a multiple other plugins which are way better maintained, but they are all much less convenient to use or support only npm and not yarn.

pvliss commented 5 years ago

😄 OK so I guess that means you are ok if we migrate to that one, right? Have you already done the changes or do you want me to do a PR?

atomfrede commented 5 years ago

Go ahead. For reference I looked at

pvliss commented 5 years ago

OK thanks. I briefly checked them out and think it would be better to stick with https://github.com/node-gradle/gradle-node-plugin for now as it seems the easiest to do. The liferay one does not seem that up to date and the other seems quite new and not very used yet, judging by the number of stars/forks. What do you think?

atomfrede commented 5 years ago

Yeah. Either use the fork (I don't know how it will be maintained) or stick with the original one for now and maybe check if we can build the needed functions into an own plugin later on.

pvliss commented 5 years ago

OK. Yes, I guess we could fix all these issues if we create a jhipster plugin. BTW, are you still interested in that?