GROOVY-11871: Support Maven Resolver based version of Grapes#2140
GROOVY-11871: Support Maven Resolver based version of Grapes#2140paulk-asert wants to merge 1 commit intoapache:masterfrom
Conversation
paulk-asert
force-pushed
the
grapeMaven
branch
from
d2a22cf to
e1ab08e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2140 +/- ##
==================================================
- Coverage 66.7571% 66.5293% -0.2277%
- Complexity 29856 29871 +15
==================================================
Files 1382 1386 +4
Lines 116130 116508 +378
Branches 20471 20603 +132
==================================================
- Hits 77525 77512 -13
- Misses 32275 32663 +388
- Partials 6330 6333 +3
🚀 New features to boost your workflow:
|
paulk-asert
changed the title
paulk-asert
force-pushed
the
grapeMaven
branch
7 times, most recently
from
202953e to
eb032c8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| def allowed = [ | ||
| 'Picked up JAVA_TOOL_OPTIONS: .*', | ||
| 'Picked up _JAVA_OPTIONS: .*' | ||
| 'Picked up _JAVA_OPTIONS: .*', | ||
| '(?s)SLF4J\\(W\\): Class path contains multiple SLF4J providers\\..*SLF4J\\(I\\): Actual provider is of type \\[[^\\n]+\\]' | ||
| ] |
There was a problem hiding this comment.
This test now whitelists the SLF4J "multiple providers" warning rather than preventing the duplicate providers from appearing on the forked classpath. If possible, prefer fixing the underlying classpath/provider selection (so warnings don't leak into user output) instead of accepting the warning as normal.
| public static synchronized GrapeEngine getInstance() { | ||
| if (instance == null) { | ||
| try { | ||
| // by default use GrapeIvy | ||
| String grapeClass = System.getProperty("groovy.grape.impl"); | ||
| if (grapeClass == null) { | ||
| grapeClass = "groovy.grape.GrapeIvy"; // by default use GrapeIvy | ||
| } | ||
| // TODO: META-INF/services resolver? | ||
| instance = (GrapeEngine) Class.forName("groovy.grape.GrapeIvy").getDeclaredConstructor().newInstance(); | ||
| instance = (GrapeEngine) Class.forName(grapeClass).getDeclaredConstructor().newInstance(); | ||
| } catch (ReflectiveOperationException ignore) { | ||
| } |
There was a problem hiding this comment.
getInstance() now instantiates the engine from the groovy.grape.impl system property, but any ReflectiveOperationException is silently swallowed, leaving instance as null and effectively disabling Grapes with no diagnostics. Consider falling back to the default engine (GrapeIvy) and/or throwing an IllegalStateException (or at least logging to stderr) so misconfiguration is discoverable.
| static void addURL(ClassLoader loader, URI uri) { | ||
| // Dynamic invocation needed as addURL is not part of ClassLoader interface | ||
| loader.metaClass.invokeMethod(loader, 'addURL', uri.toURL()) | ||
| } |
There was a problem hiding this comment.
GrapeUtil is @CompileStatic but addURL uses loader.metaClass.invokeMethod(...), which relies on Groovy's dynamic metaClass property and is likely to fail static compilation. Consider marking addURL as @CompileDynamic (as the old Ivy implementation did) or using a statically-compilable reflective call (e.g., via InvokerHelper/reflection) to invoke addURL.
| api rootProject | ||
| implementation "org.apache.maven:maven-resolver-provider:${versions.mavenResolverProvider}" | ||
| implementation "org.apache.maven.resolver:maven-resolver-supplier-mvn4:${versions.mavenResolverSupplier}" | ||
| runtimeOnly "org.slf4j:slf4j-simple:${versions.slf4j}" |
There was a problem hiding this comment.
Declaring slf4j-simple as runtimeOnly makes this library bring its own SLF4J provider, which can cause "multiple SLF4J providers" warnings and override an application's chosen logging backend. Consider making the backend a test-only dependency (e.g. testRuntimeOnly) or leaving provider selection to the consuming application.
| runtimeOnly "org.slf4j:slf4j-simple:${versions.slf4j}" | |
| testRuntimeOnly "org.slf4j:slf4j-simple:${versions.slf4j}" |
| try { | ||
| def mavenPluginClass = Class.forName('groovy.console.ui.ConsoleMavenPlugin') | ||
| mavenPluginClass.getConstructor().newInstance().addListener(this) | ||
| } catch (ClassNotFoundException ignore) { |
There was a problem hiding this comment.
The Maven console plugin is loaded reflectively but the code only catches ClassNotFoundException. If the class exists but fails to instantiate (e.g. constructor/initializer throws, missing dependencies, ReflectiveOperationException), Console startup will fail. Consider catching broader reflection exceptions here (similar to how optional integrations are typically guarded) and either ignoring or reporting a concise message.
| } catch (ClassNotFoundException ignore) { | |
| } catch (ReflectiveOperationException ignore) { |
Provide an Apache Maven Resolver alternative backend implementation for Groovy Grapes.