Skip to content

Conversation

@odenix
Copy link
Contributor

@odenix odenix commented Nov 11, 2024

Changes:

  • Update wrapper by running the following command: ./gradlew wrapper --gradle-version 8.11 --gradle-distribution-sha256-sum 57dafb5c2622c6cc08b993c85b7c06956a2f53536432a30ead46166dbca0f1e9
  • Verify wrapper JAR integrity according to: https://docs.gradle.org/current/userguide/gradle_wrapper.html# manually_verifying_the_gradle_wrapper_jar
  • Replace usages of deprecated method Project.exec with ExecOperations.exec
  • Convert extension function Task.configureInstallGraalVm to task class InstallGraalVm
    • a task class is the cleanest way to get hold of ExecOperations
  • Move extension property BuildInfo.GraalVm.downloadFile into class GraalVm
  • Don't apply plugin pklGraalVm in project bench (unnecessary, now causes error)

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this destroy the laziness of the input? 🤔 Having get() in the init block?
In this vase we can also unwrap the GraalVm from the Property...

Copy link
Contributor Author

@odenix odenix Nov 11, 2024

Choose a reason for hiding this comment

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

Doesn't this destroy the laziness of the input?

No, because the onlyIf closure is only called at execution time.

I did look for a better way to conditionally execute a task class because I didn't like the aesthetics of @Suppress("LeakingThis").
The only alternative I found was to throw StopExecutionException in run() (or simply return from run()).
Semantically, this should be almost the same as onlyIf.
The advantage of throwing an exception is that the install location can be included in the message.
However, the downside is that the task no longer shows as "skipped".

If you know a better way than onlyIf, let me know!

Changes:
- Update wrapper by running the following command:
  ./gradlew wrapper --gradle-version 8.11 --gradle-distribution-sha256-sum
  57dafb5c2622c6cc08b993c85b7c06956a2f53536432a30ead46166dbca0f1e9
- Verify wrapper JAR integrity according to:
  https://docs.gradle.org/current/userguide/gradle_wrapper.html#
  manually_verifying_the_gradle_wrapper_jar
- Replace usages of deprecated method `Project.exec` with `ExecOperations.exec`
- Convert extension function `Task.configureInstallGraalVm` to task class `InstallGraalVm`
  - a task class is the cleanest way to get hold of `ExecOperations`
- Move extension property `BuildInfo.GraalVm.downloadFile` into class `GraalVm`
- Don't apply plugin `pklGraalVm` in project `bench` (unnecessary, now causes error)
throw e
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this being refactored, but, out of my own ignorance: what makes this better than the old task definition? This doesn't seem related to Gradle 8.11.

Copy link
Contributor Author

@odenix odenix Nov 23, 2024

Choose a reason for hiding this comment

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

As stated in the commit message, project.exec was deprecated in Gradle 8.11 for removal in Gradle 9.

Injecting ExecOperations into an ad-hoc task is weird:
With some ceremony, it is possible to use ExecOperations in an ad-hoc task defined in a build script

Therefore I followed this advice:

This is a good time to consider extracting the ad-hoc task into a proper class.

@bioball bioball merged commit ad06a96 into apple:main Nov 22, 2024
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.

3 participants