-
Notifications
You must be signed in to change notification settings - Fork 389
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
Use wrapper distribution when gradle-wrapper.properties exists. #3012
Conversation
cd17696
to
9dea5e8
Compare
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.
Checking the gradle-wrapper.properties
file is the correct approach to enable the gradle wrapper importer. This file tells the tooling api where to get the gradle distribution from the distributionUrl. The tooling api will download the distribution using its own download utility. It doesn't depend on the local gradle-wrapper.jar for downloading, so it's ok to move the checksum validator to a separate job.
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.
Meanwhile, this PR moves the gradle wrapper jar checksum validation into a non-blocking job, because tooling api will not use that jar to import the project. So, it's not necessary to do it during import synchronized.
The gradle tooling API doesn't use gradle-wrapper.jar - https://github.com/gradle/gradle/blob/dc132c9b9b14ab88545cdceac90259ecd1ae1198/platforms/core-runtime/wrapper-shared/src/main/java/org/gradle/wrapper/BootstrapMainStarter.java#L45
We could remove the gradle wrapper jar checksum validation.
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.
Feel free to merge when ready. Placing the code into a Job would normally open it up to progress reporting, but since no monitor is used, it should be silent, which is fine.
Update: It does report, but it's nothing major.
[Trace - 17:48:53] Received notification '$/progress'.
Params: {
"token": "35218318-2d27-46a6-9d55-bf30bd6d7f63",
"value": {
"kind": "begin",
"title": "Validating Gradle wrapper checksum...",
"message": "Validating Gradle wrapper checksum..."
}
}
[Trace - 17:49:09] Received notification '$/progress'.
Params: {
"token": "35218318-2d27-46a6-9d55-bf30bd6d7f63",
"value": {
"kind": "end",
"message": "Validating Gradle wrapper checksum..."
}
}
...eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/GradleProjectImporter.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sheng Chen <sheche@microsoft.com>
9dea5e8
to
00200fe
Compare
This requires to remove |
In
GradleProjectImporter
, the previously implementation will useGradleDistribution.fromBuild()
when thegradlew
exists.While checking the source of Gradle, the Gradle tooling api will check
gradle/wrapper/gradle-wrapper.properties
instead of thegradlew
.See:
This PR changes the logic to use wrapper distribution (
GradleDistribution.fromBuild()
) whengradle/wrapper/gradle-wrapper.properties
exists.Meanwhile, this PR moves the gradle wrapper jar checksum validation into a non-blocking job, because tooling api will not use that jar to import the project. So, it's not necessary to do it during import synchronized.