-
Notifications
You must be signed in to change notification settings - Fork 78
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 8.6, fix most warnings for Gradle 9 #4693
Conversation
Depends on #4692, in draft until that is merged. Also trying to add spotless magic to handle license headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only looked at R change
@@ -1,5 +1,3 @@ | |||
import nl.javadude.gradle.plugins.license.License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the license manager gets dropped here. What is going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old license plugin doesn't support newer gradle. We'll instead follow up with another set of changes to update spotless to do this for us, but spotless is very picky, and that also requiring updating basically all of our codegen (at least the codegen that we commit to git, rather than just build as-needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Please make certain the new infrastructure for licenses is in place before the next release.
license { | ||
header rootProject.file('license-header') | ||
include '**/*.h' | ||
include '**/*.cc' | ||
strictCheck true | ||
useDefaultMappings true | ||
ignoreFailures true | ||
} | ||
tasks.getByName('license').dependsOn(tasks.create("licensePy", com.hierynomus.gradle.license.tasks.LicenseCheck) { | ||
source = ['deephaven'] | ||
}) | ||
tasks.getByName('licenseFormat').dependsOn(tasks.create("licenseFormatPy", com.hierynomus.gradle.license.tasks.LicenseFormat) { | ||
source = ['deephaven'] | ||
}) | ||
|
||
project.tasks.getByName('quick').dependsOn project.tasks.getByName('license') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More license stuff is dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, by design, py/cpp/proto/etc will be managed by spotless once we re-enable that.
license { | ||
header rootProject.file('license-header') | ||
include '**/*.py' | ||
strictCheck true | ||
useDefaultMappings true | ||
ignoreFailures true | ||
} | ||
|
||
tasks.getByName('license').dependsOn(tasks.create("licensePy", com.hierynomus.gradle.license.tasks.LicenseCheck)) | ||
tasks.getByName('licenseFormat').dependsOn(tasks.create("licenseFormatPy", com.hierynomus.gradle.license.tasks.LicenseFormat)) | ||
afterEvaluate { | ||
tasks.named("licensePy") { task -> | ||
task.source = wheel.src() | ||
} | ||
tasks.named("licenseFormatPy") { task -> | ||
task.source = wheel.src() | ||
} | ||
} | ||
project.tasks.getByName('quick').dependsOn project.tasks.getByName('license') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More license things removed
// TODO(deephaven-core#1997): Remove license-gradle-plugin | ||
plugins { | ||
id 'license' | ||
} | ||
|
||
license { | ||
header rootProject.file('license-header') | ||
include '**/*.java' | ||
strictCheck true | ||
useDefaultMappings true | ||
ignoreFailures true | ||
} | ||
|
||
project.tasks.getByName('quick').dependsOn project.tasks.getByName('license') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More license stuff removed.
@@ -1,7 +1,6 @@ | |||
plugins { | |||
id 'com.bmuschko.docker-remote-api' | |||
id 'io.deephaven.project.register' | |||
id 'license' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More license stuff removed.
// TODO(deephaven-core#1997): Remove license-gradle-plugin | ||
implementation('gradle.plugin.com.hierynomus.gradle.plugins:license-gradle-plugin:0.16.1') { | ||
because('needed by plugin java-header-conventions') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More license stuff removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great; happy to merge asap. Tested locally, everything seemed to work. A few questions around the gradle wrapper that I didn't expect, let's resolve that before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this file still exists, but is empty. Do we have a ticket for license follow-up? Could drop a link to it in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I'll make one up and note it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to repurpose #1997
gradlew
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually verified this is the exact contents I see when creating a new gradle project.
gradlew.bat
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, there's a diff between this and what I see in a new gradle project:
$ diff gradlew.bat /tmp/test-project/gradlew.bat
46,50c46,50
< echo.
< echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
< echo.
< echo Please set the JAVA_HOME variable in your environment to match the
< echo location of your Java installation.
---
> echo. 1>&2
> echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. 1>&2
> echo. 1>&2
> echo Please set the JAVA_HOME variable in your environment to match the 1>&2
> echo location of your Java installation. 1>&2
60,64c60,64
< echo.
< echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME%
< echo.
< echo Please set the JAVA_HOME variable in your environment to match the
< echo location of your Java installation.
---
> echo. 1>&2
> echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME% 1>&2
> echo. 1>&2
> echo Please set the JAVA_HOME variable in your environment to match the 1>&2
> echo location of your Java installation. 1>&2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now get that as well (but didn't last week), will note more below.
gradle/wrapper/gradle-wrapper.jar
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a difference here I wouldn't expect. Maybe one of us did something wrong? Hmm...
$ diff -q gradle/wrapper/gradle-wrapper.jar /tmp/test-project/gradle/wrapper/gradle-wrapper.jar
Files gradle/wrapper/gradle-wrapper.jar and /tmp/test-project/gradle/wrapper/gradle-wrapper.jar differ
$ ls -la gradle/wrapper/gradle-wrapper.jar
-rw-r--r--. 1 devin devin 63721 Feb 21 10:55 gradle/wrapper/gradle-wrapper.jar
$ ls -la /tmp/test-project/gradle/wrapper/gradle-wrapper.jar
-rw-r--r--. 1 devin devin 43462 Feb 21 11:09 /tmp/test-project/gradle/wrapper/gradle-wrapper.jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if they continue to update these post-release? I'm specifically running ./gradlew wrapper --gradle-version 8.6
, and running that same command two weeks ago vs today is showing these very slightly different results.
The sha256sum did not quite match last time either, but it does now. Perhaps its a question of running the wrapper command using an older gradle version, which updates some aspects, then running it again while using the newer version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, it seems you have to run the above command twice, so that the gradle version changes from the first command, then the second command not only fetches the update (normally this would be your first ./gradlew assemble
or whatnot), but updates again and finishes actually replacing files? Seems surprising to me, but not worth spending more time on today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ wrapper changes
This patch cleans up most of our accumulated warnings, updates to Gradle 8.6, and deals with most incoming warnings in anticipating of Gradle 9.
Updates in anticipation of Gradle 8:
Updates in anticipation of Gradle 9:
ConfigureUtil.configureUsing
junit-platform-launcher
instead of implicitly using gradle's copyWarnings that are not fixed:
Fixes #3642