Fix building with Gradle's configuration cache#48
Open
melix wants to merge 2 commits intonsoft:masterfrom
Open
Fix building with Gradle's configuration cache#48melix wants to merge 2 commits intonsoft:masterfrom
melix wants to merge 2 commits intonsoft:masterfrom
Conversation
This commit fixes the `findResources` method, which failed to find resources when the resource name ended with a `/`, which is inconsistent with the Java classloader. This issue was discovered while evaluating Uno-Jar as a replacement for fatjar creation in Micronaut.
This commit fixes support for Gradle's configuration cache. Before this change, the plugin would break if `--configuraton-cache` was used. The changes are not trivial, but I think it's for the better. The first category of changes is on the build itself. It was quite difficult to work with, because it involved publishing to maven local, and it broke up-to-date checking. The build was using bad practices like reaching out to other project's state or using explicit `dependsOn`. The problem with these is that they were mostly incorrect and redundant. Therefore, this commit also reworks the build itself in order to be self contained, without having to rely on `mavenLocal()`. For example: - the examples jar is now properly built as part of testing the Gradle plugin. It is transparently added to the `core` test classpath, using an exported configuration - the `test-suite` tests are now executed as part of the build, using Gradle's TestKit - the JDK8 tests are executed as part of the Gradle plugin tests There is no need to publish artifacts to Maven Local, but there is the ability to publish all artifacts to a local Maven repository by running: `./gradlew publishAllPublicationsToBuildRepository` in which case the artifacts are published to the root `build/myRepo` directory. This makes it much easier to check if the artifacts are correctly published. This will include the Gradle plugin artifacts. In addition, the manifest changes are only included for release, or when a special flag is passed, in order to preserve up-to-date checks and build caching. I have tested this on a test Micronaut project with success.
Author
|
As a side note: I didn't change the fact that the build uses Java toolchains. However, I think it adds unnecessary complexity, in particular in testing. For example, the project builds with Java 8, but there's no Java 8 version publicly available with support anymore. Therefore, it would probably be better to use a single Java toolchain, say, Java 17, then configure the tasks to target Java 8 or 11 (via source compatibility or --release). Let me know what you think. This would also make it possible to add a GitHub workflow to test the builds. |
Owner
|
Sorry for the delay in response, was busy, then forgot about this. It's quite a large change so it's hard to find time to review it. I do want to respond to the suggestion about tool chains. I actually dropped support for 8 and then it was added back by a contribution and requests by multiple users in #21 and #24 ... I agree that it adds complexity, and I certainly expect to sunset that complexity as versions roll forward. The intention is documented here: https://github.com/nsoft/uno-jar/wiki/Java-Version-Support though it's quite obvious I've fallen behind. The primary blocker/delay in moving forward to java 21 is a need (in my upstream project JesterJ) for #44 and an additional need for ability to start with a custom system class loader. (which motivated this experimentation: 0f19955 ) Not that 21 actually blocks it, but these things should exist in a release that supports 17 first. As for build practices, I'd be the very first to say that the build is less than ideal, but there are difficulties in testing the functionality of classes running from archives built by new code. This also was ported from a ant build that was outdated in 2013, so we're definitely baking with hysterical raisins. I have a desire to do something more modern with JesterJ soon, so now I'm peeking down the rabbit hole at this library... which is how I rediscovered this contribution, and my shame at having not even responded (sorry about that). |
Owner
|
First Comment: I would like to separate migration to kotlin from groovy from any other changes. Next Comment: IntelliJ seems exceptionally cranky about everything in buildSrc The above doesn't seem to hinder gradle itself, but with the top level build target I get: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit fixes support for Gradle's configuration cache. Before this change,
the plugin would break if
--configuraton-cachewas used. The changes are nottrivial, but I think it's for the better.
The first category of changes is on the build itself. It was quite difficult
to work with, because it involved publishing to maven local, and it broke
up-to-date checking. The build was using bad practices like reaching out to
other project's state or using explicit
dependsOn. The problem with theseis that they were mostly incorrect and redundant.
Therefore, this commit also reworks the build itself in order to be self
contained, without having to rely on
mavenLocal(). For example:plugin. It is transparently added to the
coretest classpath, using anexported configuration
test-suitetests are now executed as part of the build, usingGradle's TestKit
There is no need to publish artifacts to Maven Local, but there is the
ability to publish all artifacts to a local Maven repository by running:
./gradlew publishAllPublicationsToBuildRepositoryin which case the artifacts are published to the root
build/myRepodirectory. This makes it much easier to check if the artifacts are
correctly published. This will include the Gradle plugin artifacts.
In addition, the manifest changes are only included for release,
or when a special flag is passed, in order to preserve up-to-date
checks and build caching.
I have tested this on a test Micronaut project with success.
This PR also includes changes from #47 , but it fixes the problem that I had manually generated the examples jar, which would be overwritten previously.