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 to Gradle 7 #1476

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Jocce-Nilsson
Copy link

Separated into 5 commits:

Preparation is made in first 3 commits, minor cleanup plus improving performance by delaying/avoiding task creation.
Main upgrade work is done in last 2 commits. Dependency changes were major but required, and builds still runs fine.

Having one settings.gradle in each subfolder
is not needed. Also rewriting the rootProject name
is not recommended.
Moving build script task creation
to configuration or execution will
improve performance and caching

findByName is an older method
replaced by named()
which also does not trigger
task creation during configuration.
Why:
Dependencies were using the older configuration names
'compile' and 'testCompile' which are deprecated.
Replacing them with 'implementaiton' caused a lot of
compilation errors which made it required to refactor
all dependencies.

How:
Adding dependency-analysis plugin and using the suggested
changes in dependencies.
This also included adding of all transitive dependencies
as direct dependencies wherever code usage existed.

Common dependencies were added as they were used in several
sub projects. To keep them in sync, a version variable was
added according to previous standard.
Also fix 2 deprecation warnings:

Update reports according to api,
replacing enabled with required

Adding explicit task dependency
when using generated sources as input.

Third deprecation warning is due to usage
inside the io.spring.dependency-management plugin
and cannot be fixed until plugin is updated.
Copy link

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

forgot to submit

@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-6.8.3-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6-bin.zip

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

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

done now

@@ -1 +0,0 @@
rootProject.name = 'acceptance-test'

Choose a reason for hiding this comment

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

why was this removed?

Copy link
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 about that change, but the code seemed strange. I removed all of the settings.gradle files since there was already one in the root, and the overall speed of the project made it redundant to have every sub project on it's own. Perhaps it is wrong. We'll see.

Choose a reason for hiding this comment

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

i agree that it should be removed unless these projects can be buillt independently of the root project.

@@ -1,3 +1,7 @@
plugins {

Choose a reason for hiding this comment

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

did you mean to intentionally keep this plugin here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I did, I thought it was a good idea to keep the project clean for other maintainers.
Perhaps I should have added a small readme about it?

Copy link
Author

Choose a reason for hiding this comment

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

Or I could remove it again

Choose a reason for hiding this comment

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

there are a large number of changes in this PR and it makes it hard to review. i would move this to a separate PR.

@@ -163,6 +174,11 @@ tasks.named("compileJava").configure {
source(generateAvro)
}

tasks.named("checkstyleMain").configure {

Choose a reason for hiding this comment

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

is this a new task? or was this moved from somewhere else?

Copy link
Author

Choose a reason for hiding this comment

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

new configuration of the task (added from root project) to fix deprecation warning
(warning: no dependency from checkstyleMain towards compileTestJava)

Choose a reason for hiding this comment

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

ah gotcha 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants