-
Notifications
You must be signed in to change notification settings - Fork 93
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
5.0.x fix graalvm resolution #370
base: 5.0.x
Are you sure you want to change the base?
5.0.x fix graalvm resolution #370
Conversation
Previous behaviorruntimeOnly "com.bertramlabs.plugins:asset-pipeline-grails:$assetPipeline"
assets "com.bertramlabs.plugins:less-asset-pipeline:$assetPipeline" results in:
|
@codeconsole I have many comments about this project, but for this PR:
compileOnly 'org.grails:grails-web-taglib' // For taglib compilation I think these are the only js-related dependencies needed in // Library consumers should be provide engines if they need them, to not bloat other consumers
// This could possibly be refactored to use javax.script.ScriptEngine API (JSR-223), and/or pluggable implementations
compileOnly "org.graalvm.polyglot:polyglot:$graalvmVersion"
compileOnly 'com.google.javascript:closure-compiler-unshaded:v20240317' And these in runtimeOnly "org.graalvm.js:js-community:$graalvmVersion"
runtimeOnly 'com.google.javascript:closure-compiler-unshaded:v20240317' Now, using |
@matrei other than fixing the spacing on the gradle files, this pull request is only to address the dependency resolution issue in bootRun. This PR is not for Rather than using startsWith and adding jars individually by text search, it leverages gradle dependency management and only adds the jars exposed by the
The end result of this PR is it stops resolving jsr305 and jspeciify from being added to the bootRun classpath and it resolves the missing transitive dependencies that were previously needed. In versions of this plugin < 5.0, there were never api dependencies on asset-pipeline-core. |
@codeconsole Sorry for not being explicit, I think the change to But in addition, my suggestions are focused on cleaning up the dependencies. Also, without the addition of the |
asset-pipeline-gradle/src/main/groovy/asset/pipeline/gradle/AssetPipelinePlugin.groovy
Outdated
Show resolved
Hide resolved
@matrei I don't have the time to burn on this. I just wanted to get the plugin working correctly. We can clean it up in a separate PR. I didn't add any dependencies, I just made graalvm version a property. |
@matrei I added you as a collaborator to my repository. Feel free to add to my PR if you want. |
@codeconsole OK, I'll give it a swing. |
Minor cleanup of unused imports and changing `@Commons` to `@Slf4j`.
… Groovy idioms - Adopt more up-to-date Gradle API usage for better compatibility and maintainability. - Refactor to leverage Groovy language features for cleaner and more idiomatic code.
Update project dependencies and extract versions to `gradle.properties`.
@codeconsole OK, I'm done! |
Sweet, thanks @matrei ! |
@matrei I am impressed, you really cleaned this up! |
This addresses
This exception occurs because
org.graalvm.polyglot:polyglot
is not present in the classpath, but this is expected because that dependency is not part of graalvm 22.0.0.2. So if an application is using a later version of graalvm, they will get an exception asset-pipeline-gradle uses 22.0.0.2 and the plugin does not automatically look for an include the polygot dependency.Currently, dependencies are added to the bootRun task if the dependency starts with a particular string, but this is actually resulting in unwanted dependencies being added such as findbugs jsr305 and jspecify which aren't even needed simply because
file.name.startsWith('js')
.asset-pipeline/asset-pipeline-gradle/src/main/groovy/asset/pipeline/gradle/AssetPipelinePlugin.groovy
Lines 167 to 169 in 905f399
All these things are done in the background and there is no way to know that they are being resolved in the final bootRun classpath.
This PR simplifies everything, makes it more maintainable, and provides better exposure to understand what exactly is being added to the bootRun classpath.
24.1.2
8.12.1
Pros
Simple
Makes class path resolution understood
Simplifies code
Imports dependencies as expected
Allows maintaining of dependencies via
build.gradle
Cons
Adds a single gradle plugin
asset-pipeline-gradle
jar to the bootRun classpath, but this has the added advantage of not needing to hard code graalvm versions inside the plugin code.