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 Gradle to 6.9.2 #1664

Merged
merged 32 commits into from
Mar 23, 2022
Merged

Conversation

yhtMinceraft1010X
Copy link
Contributor

@yhtMinceraft1010X yhtMinceraft1010X commented Feb 13, 2022

Part of #1661. Upgrading to version 6.9.2 is meant to be a stopgap measure while resolving errors from third-party plugins.

Propsed Commit Message:

Gradle has been stuck at 5.2.1 for quite some time. This version is not
compatible with a future migration to Java 17. However, upgrading to
Gradle 7.4 cannot be done yet due to a need to resolve errors from
third-party plugins.

Let's upgrade Gradle to 6.9.2 in the meantime to close the gap between
the version we are using and the latest available version.

@chan-j-d
Copy link
Contributor

chan-j-d commented Feb 13, 2022

I've requested a second run of the checks. It seems the previous build failed on this
https://github.com/reposense/RepoSense/runs/5174576252?check_suite_focus=true#step:11:1465.
I've encountered it myself a few times and it's always somewhat random, perhaps we should raise a separate issue about it.

@yhtMinceraft1010X
Copy link
Contributor Author

I've requested a second run of the checks. It seems the previous build failed on this https://github.com/reposense/RepoSense/runs/5174576252?check_suite_focus=true#step:11:1465. I've encountered it myself a few times and it's always somewhat random, perhaps we should raise a separate issue about it.

Might be a concurrency issue similar to #1601. I think we should wait for #1625 to be merged.

@dcshzj dcshzj removed the request for review from a team February 14, 2022 00:30
A third-party plugin has a property that requires Java 9 to work
properly. It throws an error when Java 8 is used.

Let's downgrade it to a version before this property was
introduced.
A pre-compiled Java 8 jar file works on Java 17.

Let's reverse the ug change implying that Java 17 cannot work.
The register method defers configuration of tasks such that tasks
are only configured when needed.

Let's use the register method.
The .with methods in CSVFormat are marked for deprecation in v1.9.0
of Apache Commons CSV.

Let's switch to the builder methods.
Copy link
Contributor

@chan-j-d chan-j-d left a comment

Choose a reason for hiding this comment

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

LGTM! The current implementation seems to work well. Just some minor comments/clarifications.

Would like to confirm if rewriting the task definitions as tasks.register(...) is to support a future gradle 7 upgrade described here since it seems the gradle 6.8.2 documentation uses quite a different syntax.
Additionally, would it be possible to assign the tasks to variables to avoid referencing them by string? More details are described in the specific comment.

build.gradle Outdated

manifest {
attributes 'Implementation-Version': getRepoSenseVersion()
}
}

task installFrontend(type: com.liferay.gradle.plugins.node.tasks.ExecuteNpmTask) {
tasks.register('installFrontend', com.liferay.gradle.plugins.node.tasks.ExecutePackageManagerTask) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For those curious like me, it seems the task name itself was changed in this commit liferay/liferay-portal@e47fedf.

build.gradle Outdated
Comment on lines 114 to 120
tasks.register('installFrontend', com.liferay.gradle.plugins.node.tasks.ExecutePackageManagerTask) {
workingDir 'frontend/'
args = ['install']
}

task buildFrontend(dependsOn: ['installFrontend'], type: com.liferay.gradle.plugins.node.tasks.ExecuteNpmTask) {
tasks.register('buildFrontend', com.liferay.gradle.plugins.node.tasks.ExecutePackageManagerTask) {
dependsOn 'installFrontend'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to do an assignment like def installFrontend = tasks.register(...)? This way all further references to installFrontend like in line 120 can reference installFrontend without quotations directly. At least this seems possible when I tried it locally.

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 managed to change some of the assignments

@yhtMinceraft1010X
Copy link
Contributor Author

For tasks.register(...), it's used for task configuration avoidance. This prevents unnecessary tasks from being configured, which is best paired with #1631 - when only backend tests or backend checkstyle are run, configuration of frontend-related tasks like installFrontend is not carried out.

@chan-j-d chan-j-d requested a review from a team March 14, 2022 11:09
@gerhean
Copy link
Contributor

gerhean commented Mar 15, 2022

May I ask about the significance of including the Apache License block of text? It's a little weird that it Copyright 2015 the original author or authors. but nowhere states who these author or authors are.

@dcshzj
Copy link
Member

dcshzj commented Mar 15, 2022

May I ask about the significance of including the Apache License block of text? It's a little weird that it Copyright 2015 the original author or authors. but nowhere states who these author or authors are.

@gerhean I think the Apache License terms requires that the copyright notice is retained. I think for the "the original author or authors", we should leave it as it is, in case of licensing issues that may occur.

@gerhean
Copy link
Contributor

gerhean commented Mar 18, 2022

Just want to confirm that this has been tested to work on Windows? I assume it works on MacOs and Linux since the CI passed.

@chan-j-d
Copy link
Contributor

chan-j-d commented Mar 18, 2022

Just want to confirm that this has been tested to work on Windows? I assume it works on MacOs and Linux since the CI passed.

@gerhean Yep, I pulled the branch, clean build, frontendTest and ran it against a few repositories on Windows when I tested it.

Copy link
Contributor

@gerhean gerhean left a comment

Choose a reason for hiding this comment

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

LGTM

@gerhean gerhean requested a review from a team March 18, 2022 14:47
@dcshzj dcshzj merged commit af551f3 into reposense:master Mar 23, 2022
@github-actions
Copy link
Contributor

The following links are for previewing this pull request:

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

Successfully merging this pull request may close these issues.

None yet

5 participants