Make the plugin work #4
Conversation
Breaks task caching
Fixes task caching, but at what cost?
No more eager initialization, no sir
Requires manually setting jlink task dependencies, but that will be fixed later
Modules are determined by analyzing the libraries the project depends on, as well as running jdeps on build/classes directory to determine the modules used directly by the project Remove the modules and modulePath options, since they are no longer needed Add a sample JavaFX app project
Simplify the unit test logic by using the plugins block instead
Useful if the application or libraries it uses reference some class reflectively, since jdeps can't detect reflective references to modules
Also mention the extraModules option
OpenJDK 10 and 11 are still running
Launcher scripts will be generated if launcher options are set. No need for a separate task
README.md
Outdated
|
|
||
| jlink { | ||
| "configuration name" { | ||
| applicationJar = tasks.getByName<Jar>("jar").archivePath // No default value |
There was a problem hiding this comment.
This forces the configuration of the "jar" task, which is not good.
Additionally, if a configuration block is executed later that further configures all the Jar tasks, this won't update. We shouldn't do this.
| * ``` | ||
| */ | ||
| operator fun String.invoke(configurationAction: Action<in JLinkOptions>) = register(this, configurationAction) |
There was a problem hiding this comment.
We should be exposing the NamedDomainObjectContainer's API as the primary way for adding JLinkOptions. The NamedDomainObjectContainer is a standard API so it's better to just let users use that without providing helper methods for interacting with it. IMO.
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/JLinkLauncherOptions.kt
Show resolved
Hide resolved
| * The application JAR. This will be copied into the generated jlink image's `bin` directory, next to the | ||
| * `java` executable. | ||
| */ | ||
| var applicationJar: File? = null |
There was a problem hiding this comment.
This should be a RegularFileProperty.
There was a problem hiding this comment.
RegularFileProperty or just Provider<File>? RegularFileProperty is difficult to bind to normal File objects like those provided by the outputs of the Jar task
There was a problem hiding this comment.
You want RegularFileProperty. I've fixed the Jar task for Gradle 5.1 to support being able to produce a RegularFileProperty directly.
There was a problem hiding this comment.
I'd prefer not to have to depend on bleeding edge releases. It limits the usability of the plugin
There was a problem hiding this comment.
Regardless, we should still just use the RegularFileProperty.
There was a problem hiding this comment.
RegularFileProperty is impossible to use in 4.10 when setting it to a normal File object like Jar::archivePath or ShadowJar::archivePath. And as I said before, I don't want to depend on a bleeding edge release and limit the usability of the plugin
| @OutputDirectory | ||
| var jlinkDir: DirectoryProperty = newOutputDirectory() | ||
| @get:InputFile | ||
| val applicationJarLocation: RegularFileProperty = newInputFile() |
There was a problem hiding this comment.
Apparently, newInputFile is deprecated in Gradle 5.0.
There was a problem hiding this comment.
Welp. As long as it'll still work
| import org.gradle.api.tasks.InputFile | ||
| import org.gradle.api.tasks.OutputDirectory | ||
| import org.gradle.api.tasks.TaskAction | ||
| import org.gradle.api.tasks.* | ||
| import org.gradle.internal.jvm.Jvm |
There was a problem hiding this comment.
What alternatives do you propose?
There was a problem hiding this comment.
What's the use case or what do you need?
There was a problem hiding this comment.
To get the build JDK to determine the location of the jlink and jdeps tools. The java.home system property can't be used since Gradle may be running on a different JDK, like if -Dorg.gradle.java.home is set
There was a problem hiding this comment.
Can you open an issue with Gradle or ask about an alternative solution in the Gradle Slack channel?
| import org.gradle.internal.jvm.Jvm | ||
| import org.gradle.internal.os.OperatingSystem |
There was a problem hiding this comment.
Unfortunately, this is a very useful API with no real public alternative. I could copy the class into the plugin, but that seems hacky
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/LauncherGenerator.kt
Outdated
Show resolved
Hide resolved
plugin/src/test/kotlin/org/gradleweaver/plugins/jlink/LauncherGeneratorTest.kt
Outdated
Show resolved
Hide resolved
|
Can we make it so that it's possible to build a windows Jlink executable from a Mac and vice versa? |
| /** | ||
| * Use the endianness of the build system. | ||
| */ | ||
| SYSTEM_DEFAULT, |
There was a problem hiding this comment.
Shouldn't we just use the system default by default? Or is this because we can't determine the system default?
There was a problem hiding this comment.
This is equivalent to not passing the --endian flag. The jlink tool will use the byte order of the build system.
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/JLinkLauncherOptions.kt
Show resolved
Hide resolved
| * The application JAR. This will be copied into the generated jlink image's `bin` directory, next to the | ||
| * `java` executable. | ||
| */ | ||
| var applicationJar: File? = null |
There was a problem hiding this comment.
You want RegularFileProperty. I've fixed the Jar task for Gradle 5.1 to support being able to produce a RegularFileProperty directly.
| /** | ||
| * Options for the levels of compression available for generated runtime images. | ||
| */ | ||
| enum class CompressionLevel { |
There was a problem hiding this comment.
Can you link to documentation describing these each in more detail?
There was a problem hiding this comment.
There was a problem hiding this comment.
This is also linked in the README
There was a problem hiding this comment.
Can you link to the exact section?
https://docs.oracle.com/javase/9/tools/jlink.htm#GUID-CECAC52B-CFEE-46CB-8166-F17A8E9280E9__GUID-DF03D0FC-9390-4C17-BEEA-ECC2EA9B94F2
There was a problem hiding this comment.
The page doesn't support linking to the individual sections
| import org.gradle.api.tasks.InputFile | ||
| import org.gradle.api.tasks.OutputDirectory | ||
| import org.gradle.api.tasks.TaskAction | ||
| import org.gradle.api.tasks.* | ||
| import org.gradle.internal.jvm.Jvm |
There was a problem hiding this comment.
What's the use case or what do you need?
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/JLinkTask.kt
Outdated
Show resolved
Hide resolved
| `--disable-plugin pluginname` | ||
| `--limit-modules` | ||
| `--save-opts` | ||
| `@filename` |
There was a problem hiding this comment.
Elaborate on what this is supposed to mean.
There was a problem hiding this comment.
The assumption here is that people using a jlink plugin will have at least familiarized themselves with the jlink options from the reference page. We shouldn't have to explain standard tool options
There was a problem hiding this comment.
Erm... I don't think that's a valid assumption.
|
|
||
| fun getApplicationModuleName(): String = applicationModuleName.get() | ||
|
|
||
| fun getMainClassName(): String = mainClassName.get() |
There was a problem hiding this comment.
Any of these get*** methods could throw an exception if the value isn't present, just FYI.
|
|
||
| /** | ||
| * Strips debug symbols from the generated image. | ||
| */ | ||
| var stripDebug = false | ||
| val stripDebug = objectFactory.property(false) | ||
|
|
||
| /** | ||
| * Optimize `Class.forName` calls to constant class loads. | ||
| */ |
There was a problem hiding this comment.
Elaborate? I have no idea what this means from a practical sense.
There was a problem hiding this comment.
It bypasses class lookups
// Your code
Class<?> fooClass = Class.forName("com.example.Foo");
// Is optimized to
Class<?> fooClass = com.example.Foo.class;There was a problem hiding this comment.
Fascinating! 👍 Can you add a comment about that?
|
|
||
| /** | ||
| * Excludes man pages from the generated image. | ||
| */ | ||
| var excludeManPages = false | ||
| val excludeManPages = objectFactory.property(false) |
There was a problem hiding this comment.
How are man pages even generated??
There was a problem hiding this comment.
It's man pages for the Java executable(s) IIRC. Man pages are never generated by jlink
There was a problem hiding this comment.
How do you declare man pages? Is there a location that you need to declare them to get them bundled? If so, then we should document how a user can get man pages added, ie. what convention they need to follow.
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/JLinkPlugin.kt
Outdated
Show resolved
Hide resolved
plugin/src/main/kotlin/org/gradleweaver/plugins/jlink/JLinkPlugin.kt
Outdated
Show resolved
Hide resolved
samples/groovy-dsl/build.gradle
Outdated
|
|
||
| jlink { | ||
| register("release image") { | ||
| applicationJar.set(provider { tasks.withType(Jar).getByName("jar").archivePath }) |
There was a problem hiding this comment.
This doesn't convey the dependency information the way you want. You need to use the suggestion I provided above, (may require Gradle 5.0 to work).
There was a problem hiding this comment.
I'd prefer not to have to rely on high versions of Gradle, though it seems 5.0 is near to release
samples/javafx-app/build.gradle.kts
Outdated
| jlink { | ||
| "release image" { | ||
| useMinimalImage() | ||
| applicationJar.set(provider { jarTask.archivePath }) |
There was a problem hiding this comment.
Same issue here with not conveying the dependency information.
samples/javafx-app/build.gradle.kts
Outdated
| if (name == "jlinkGenerateReleaseImage") { | ||
| dependsOn("jar") | ||
| } | ||
| } |
There was a problem hiding this comment.
If we set up this plugin correctly, this shouldn't be necessary.
Co-Authored-By: SamCarlberg <sam.carlberg@gmail.com>
build.gradle.kts
Outdated
| dependsOn(gradle.includedBuild("simple-jlink").task(":check")) | ||
| dependsOn(findProject(":samples:javafx-app")!!.tasks.getByName("jlinkGenerateReleaseImage")) | ||
| dependsOn(findProject(":samples:simple-jar")!!.tasks.getByName("jlinkGenerateReleaseImage")) | ||
| dependsOn(findProject(":samples:groovy-dsl")!!.tasks.getByName("jlinkGenerateReleaseImage")) |
There was a problem hiding this comment.
Make each sub-project's check task depend upon that project's jlinkGenerateReleaseImage task, not the root check task.
There was a problem hiding this comment.
I'd prefer the samples to be normal projects that use the plugin.
There was a problem hiding this comment.
These goals aren't contradictory?
There was a problem hiding this comment.
Well, the way I see it is that the root project just ties the plugin project and the sample projects together. So having the root project's check task make sure the samples play well with the plugin makes more sense to me than having each sample project's standard check task not only check the normal project things (tests, static analysis, etc) but also check the generated jlink distributions
| * The name of the generated launcher script. If not set, the launcher name will be the name of the | ||
| * corresponding project. | ||
| */ | ||
| val launcherName = objectFactory.property(project.name) |
There was a problem hiding this comment.
Make the default lazily evaluate project.name.
There was a problem hiding this comment.
Does it matter? I thought the project name will always be defined since it's an immutable property
There was a problem hiding this comment.
Oh, if it's immutable, then it's fine.
Instead of hardcoding for the task names
For simplicity's sake, make JLinkTasks always depend on the project's jar task (for determining module info and linking/bundling). Support for custom jar files may be re-implemented in the future Make the JavaFX sample modular Add a simple modular project sample Make task caching work (which unfortunately requires manually implementing Serializable)
|
Merging this PR as-is. Future issues will be addressed in followup PRs. ATM the plugin is functional and usable, and I don't want to make this PR stay open for ages (it's already been a month!) while small improvements are made. |
No description provided.