se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 415 forks source link

Upgrade Gradle to 7.4.2 #134

Closed yhtMinceraft1010X closed 2 years ago

yhtMinceraft1010X commented 2 years ago

Fixes #123

Properties marked as deprecated and to be removed in a future Gradle 8.0 are also changed.

canihasreview[bot] commented 2 years ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

canihasreview[bot] commented 2 years ago

v1

@yhtMinceraft1010X submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/134/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
fans2619 commented 2 years ago

@damithc Hi prof, there seems to be some PRs going on here for upgrading different tools. Some seems to be pre-requisites of upgrading Java (e.g., this upgrade of gradle to 7.4.2). Do you want to merge them?

By the way, the app seems able to run when I simply change to Java 17. 😅

damithc commented 2 years ago

Thanks for the PR @yhtMinceraft1010X and thanks for the reminder @fans2619

@yhtMinceraft1010X I assume all the changes in this commit are interrelated and cannot be done as multiple independent steps (and hence, multiple commits)?

@se-edu/tech-team-level1 for your review please

kouyk commented 2 years ago

I performed the upgrade process separately, following instructions in https://docs.gradle.org/current/userguide/upgrading_version_5.html and https://docs.gradle.org/current/userguide/upgrading_version_6.html to perform the necessary checks. The properties updated in this PR are the only ones I found necessary as well.

Not sure how @yhtMinceraft1010X performed the upgrades, for myself I ran gradle wrapper --gradle-version 6.9.2 and gradle wrapper --gradle-version 7.4.2. Several gradle wrapper scripts and jars have been updated as well, which were not included in this PR. See my branch for the details.

I can confirm that it is necessary to bump Gradle Shadow together with Gradle, due to deprecations and some incompatibilities I have outlined in my commits.

I assume all the changes in this commit are interrelated and cannot be done as multiple independent steps (and hence, multiple commits)?

The only question is whether to bump Gradle directly to v7 or perform the upgrade step by step and commit them separately.

damithc commented 2 years ago

@yhtMinceraft1010X and others, what do you guys think? Do the 'full' upgrade in two steps as @kouyk mentioned or do a minimal one-step upgrade as done in this PR? What are the pros and cons?

yhtMinceraft1010X commented 2 years ago

I tried running gradlew wrapper --gradle-version 7.4.2 again and there were some additional changes with respect to the wrapper scripts and the jar file.

I've squashed them into the same commit.

canihasreview[bot] commented 2 years ago

v2

@yhtMinceraft1010X submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/134/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
vimuthm commented 2 years ago

Do the 'full' upgrade in two steps as @kouyk mentioned or do a minimal one-step upgrade as done in this PR?

The gradle guides suggest running the upgrades separately and following that, I think it makes sense to commit the changes separately as well. One pro of this would be might be easier to track any regressions down the line, though it will be unlikely.

canihasreview[bot] commented 2 years ago

v3

@yhtMinceraft1010X submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/134/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 2 years ago

Thanks for the reviews @kouyk @VimuthM @yhtMinceraft1010X can you do the small change @kouyk suggested. After that, we can merge this.

canihasreview[bot] commented 2 years ago

v4

@yhtMinceraft1010X submitted v4 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/134/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
yhtMinceraft1010X commented 2 years ago

These are the corresponding PRs for the same change in Duke:

damithc commented 2 years ago

Thanks for your good work @yhtMinceraft1010X