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

Upgrade from a very old gradle version #45

Merged
merged 2 commits into from Oct 7, 2021
Merged

Conversation

mirkoperillo
Copy link
Contributor

Upgrade to a newer Gradle version, upgraded:

  • wrapper version settings,
  • fix deprecated settings in build.gradle,
  • Dockerfile

@mirkoperillo mirkoperillo requested a review from a team October 4, 2021 17:51
@mirkoperillo mirkoperillo requested a review from a team as a code owner October 4, 2021 17:51
@iHiD
Copy link
Member

iHiD commented Oct 4, 2021

@mirkoperillo Any chance you could benchmark this to see what it does to performance before we merge it pls?

@iHiD
Copy link
Member

iHiD commented Oct 4, 2021

(Hopefully it's an improvement, but I've learnt not to trust my hopes for these!)

@mirkoperillo
Copy link
Contributor Author

@iHiD I can try. I have to think a good way to test this.
I have proposed this upgrade to try to solve this sort of timeout here https://github.com/exercism/java-analyzer/pull/44/checks.

@mirkoperillo mirkoperillo changed the title Upgrade from a very old gradle version WIP: Upgrade from a very old gradle version Oct 4, 2021
@mirkoperillo
Copy link
Contributor Author

Some current data I can see immediately:

@mirkoperillo
Copy link
Contributor Author

mirkoperillo commented Oct 4, 2021

Using --no-daemon option the execution time improves and is aligned to the previous one.
Test with Gradle ends in 15s and the complete task in 23s

@mirkoperillo
Copy link
Contributor Author

@iHiD using the github action as benchmark the last commit permits to have the same execution time of the previous Gradle version.

Looking the usage of Gradle in this application I can say:

  • it is used in Github action to build/validate the tests of application in a PR,
  • it is used to build the application in the docker image building.

It is not used in the running container so I don't think this upgrade will influence the execution workflow ( I don't think we would have the same problems we had in java-test-runner).

Before merge it @exercism/java other opinions ?

@mirkoperillo mirkoperillo changed the title WIP: Upgrade from a very old gradle version Upgrade from a very old gradle version Oct 5, 2021
Copy link

@ericbalawejder ericbalawejder left a comment

Choose a reason for hiding this comment

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

Would it be valuable to upgrade the whole project gradle version?
https://github.com/exercism/java/blob/main/gradle/wrapper/gradle-wrapper.properties

Should we wait for gradle to support Java 17 before we upgrade both?

@mirkoperillo
Copy link
Contributor Author

@ericbalawejder I think we can do an incremental upgrade of both, now to Gradle 7.2 and then to the version will support Java 17.

In this moment java-analyzer is based on Gradle 4.8 that is very old. The upgrade didn't introduce much work and the project currently is not used in production and it is under active development (so another step of active test before going live).

We know that exercism/java is already compatible with Gradle 7.x after exercism/java#1875, so we can think to align the wrapper to the new version (after a good session of tests)

@mirkoperillo mirkoperillo merged commit 1187b84 into main Oct 7, 2021
@mirkoperillo mirkoperillo deleted the gradle-upgrade branch October 7, 2021 19:02
@mirkoperillo mirkoperillo added the hacktoberfest-accepted Make this PR count for hacktoberfest label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Make this PR count for hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants