[Gradle JDK automanagment] GradleWrapperMain patching #341
Conversation
crogoz
changed the title
| // Set the daemon Java Home | ||
| String osName = CurrentOs.get().uiName(); | ||
| String archName = CurrentArch.get().uiName(); | ||
| Path daemonKdkMajorVersionPath = projectHome.resolve("gradle/gradle-daemon-jdk-version"); |
| } | ||
| for (File file : files) { | ||
| if (file.isDirectory()) { | ||
| paths.addAll(getAllInstallationsPaths(file, gradleJdksInstallationDir, localPathPattern)); |
There was a problem hiding this comment.
It might be better to use Files.walk or Files.visitFileTree here rather than manually doing the recursive file exploration.
There was a problem hiding this comment.
In fact, why is this searching the file tree - shouldn't the installation path be at a fixed known location inside $projectRoot/gradle/jdks/....? Why do the file searching?
| .getCodeSource() | ||
| .getLocation() | ||
| .toURI(); | ||
| } catch (URISyntaxException var3) { |
There was a problem hiding this comment.
Probably give this a better name than var3, even if it's e (was this copied from a decompiler?)
| } else { | ||
| try { | ||
| return Paths.get(location); | ||
| } catch (NoClassDefFoundError var2) { |
gradle-jdks/build.gradle
Outdated
| @@ -14,13 +14,16 @@ dependencies { | |||
| api project(':gradle-jdks-distributions') | |||
| api project(':gradle-jdks-json') | |||
| api project(':gradle-jdks-setup') | |||
| api project(':gradle-jdks-setup-common') | |||
There was a problem hiding this comment.
I think because the files that were moved to gradle-jdks-setup-common were originally in gradle-jdks-distributions, we should make it an api dep from gradle-jdks-distributions not here?
| @@ -66,4 +69,5 @@ tasks.withType(JavaCompile).configureEach { | |||
|
|
|||
| tasks.test { | |||
| environment("PROJECT_VERSION", project.version) | |||
| dependsOn ':gradle-jdks-setup:fatJar' | |||
There was a problem hiding this comment.
GradleJdkInstallationSetupIntegrationTest is inside the integrationTest source set, so I think this dependsOn needs to go on under tasks.named('integrationTest').configure {
There was a problem hiding this comment.
Hmm, that's actually in gradle-jdks-setup
gradle-jdks-setup/build.gradle
Outdated
| @@ -21,10 +22,23 @@ dependencies { | |||
| integrationTestImplementation 'org.assertj:assertj-core' | |||
| } | |||
|
|
|||
| task fatJar(type: Jar) { | |||
There was a problem hiding this comment.
Might as well use tasks.register('fatJar', Jar) { for Gradle best practices.
| tasks.withType(JavaCompile) { | ||
| options.errorprone.disable 'PreferSafeLoggableExceptions' | ||
| } | ||
|
|
||
| tasks.integrationTest { | ||
| environment("PROJECT_VERSION", project.version) | ||
| } | ||
|
|
||
| tasks.build.dependsOn tasks.fatJar |
There was a problem hiding this comment.
How are we going to ship the fat jar to people? I guess we don't have the code in the plugin to do this yet (?), but I imagine we're either going to include the fat jar as a resource or download it?
There was a problem hiding this comment.
Correct, we don't have yet the code in the plugin to do this yet.
I think it needs to be distributed alongside "./gradle-jdks.setup.sh" and the "gradle/" files. I was thinking of downloading it (similar how the wrapper.jar gets added to a project). I'll do this in the next PR.
|
Released 0.38.0 |
Before this PR
After this PR
Part of: #333
Workflow changes:
Modifies GradleWrapperMain when running
./gradlew wrapper:gradle-wrapper.jarcontent into a new directoryGradleWrapperMainintoOriginalGradleWrapperMainusing ASMGradleWrapperMainclass that delegates the installation of the jdks (see explanation below) to the./gradle/gradle-jdks-setup.shscriptgradle-wrapper.jarfileModifies
./gradle/gradle-jdks-setup.shscript:gradle/jdksconfiguration directoryrunning.from.gradlew) to theGradlwWrapperMainargs; this ensures that if GradleWrapperMain was already called through./gradlewwe won't need to do any extra setup.Modifies the
./gradlewpatch to include therunning.from.gradlewpropertyImplementation changes:
gradle-jdks-setup-commonthat groups the classes (CurrentOs/CurrentArch/Os/Arch/CommandRunner etc.). It will be added in the fatJargradle-jdks-setup-all*.jar==COMMIT_MSG==
[Gradle JDK automanagment] GradleWrapperMain patching logic
==COMMIT_MSG==
Possible downsides?