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.5236% -0.2335%
- Complexity 29856 29869 +13
==================================================
Files 1382 1386 +4
Lines 116130 116500 +370
Branches 20471 20601 +130
==================================================
- Hits 77525 77500 -25
- Misses 32275 32666 +391
- Partials 6330 6334 +4
🚀 New features to boost your workflow:
|
paulk-asert
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import org.eclipse.aether.graph.Dependency | ||
| import org.eclipse.aether.graph.DependencyFilter | ||
| import org.eclipse.aether.graph.DependencyNode | ||
| import org.eclipse.aether.internal.impl.scope.ScopeDependencySelector |
There was a problem hiding this comment.
ScopeDependencySelector is imported from org.eclipse.aether.internal.impl.scope, which is an internal/implementation API of Maven Resolver. Internal APIs are not covered by the same compatibility guarantees as public APIs and may change between versions. Consider using the public API alternatives for scope-based dependency selection, or document this as a known fragility so it can be updated if the Maven Resolver version is bumped.
| private static String detectOsArch() { | ||
| String osArch = System.getProperty('os.arch').toLowerCase() | ||
| if (osArch.matches('.*64.*')) { | ||
| if (osArch.contains('aarch64') || osArch.contains('arm64')) { | ||
| return 'aarch_64' | ||
| } | ||
| return 'x86_64' |
There was a problem hiding this comment.
Bug: The detectOsArch() method checks osArch.matches('.*64.*') first, which matches architectures like ppc64le and s390x (via potential '64' in the string). For ppc64 and ppc64le, this first branch will match and return x86_64 instead of falling through to the ppc64 check on line 601. The ppc64, s390, and sparc checks further down are effectively unreachable for 64-bit variants of those architectures.
The fix should be to check for aarch64/arm64/ppc64/s390x etc. before the generic .*64.* catch-all, or change the first check to be more specific (e.g., match x86_64 or amd64 explicitly).
| private boolean isIgnorableUnresolvedArtifact(RepositorySystem system, | ||
| RepositorySystemSession session, | ||
| Artifact rootArtifact, | ||
| Artifact missingArtifact) { | ||
| if (missingArtifact == null) return false | ||
| // Never ignore unresolved root artifact. | ||
| if (rootArtifact != null | ||
| && missingArtifact.groupId == rootArtifact.groupId | ||
| && missingArtifact.artifactId == rootArtifact.artifactId | ||
| && missingArtifact.version == rootArtifact.version) { | ||
| return false | ||
| } | ||
| // Ignore classifier/native variants that are often platform-specific/optional. | ||
| if (missingArtifact.classifier) return true | ||
| // Ignore POM-only coordinates requested as jars. | ||
| if (isMissingJarButPomExists(system, session, missingArtifact)) return true | ||
| // For transitive-only misses, keep Ivy-like lenient behavior and continue. | ||
| true | ||
| } |
There was a problem hiding this comment.
isIgnorableUnresolvedArtifact unconditionally returns true at line 648 as a fallback, meaning all unresolved transitive dependencies are silently ignored. This could mask real resolution failures (e.g., a genuine missing transitive dependency that the application needs at runtime), causing hard-to-diagnose ClassNotFoundExceptions later. Consider at minimum logging a warning when an unresolved transitive dependency is being ignored, or restricting silent ignoring to well-known patterns (like optional dependencies).
subprojects/groovy-console/src/main/groovy/groovy/console/ui/ConsoleIvyPlugin.groovy
Outdated
Show resolved
Hide resolved
subprojects/groovy-grape-maven/src/test/groovy/groovy/grape/maven/GrapeMavenTest.groovy
Show resolved
Hide resolved
paulk-asert
force-pushed
the
grapeMaven
branch
6 times, most recently
from
ac26560 to
202953e
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.
|
|
||
| assert writer.toString() == '<?xml version="1.0" ?><root attribute="1"><elem1>hello</elem1><elem2>world</elem2></root>' | ||
| def pretty= writer.toString() | ||
| .replaceAll(/<\?xml[^>]*>/, '') // remove XML declaration |
There was a problem hiding this comment.
writer.toString() previously included a space after the XML declaration (<?xml ... ?> <root...>). After .replaceAll(/<\?xml[^>]*>/, ''), that leading space will remain, so pretty is likely ' <root... and this assertion will fail depending on the StAX writer implementation. Consider removing the declaration and any adjacent whitespace (e.g., include \s* in the regex or trim the result) before comparing.
| .replaceAll(/<\?xml[^>]*>/, '') // remove XML declaration | |
| .replaceAll(/<\?xml[^>]*>\s*/, '') // remove XML declaration and any following whitespace |
| * } | ||
| * assert writer.toString() == '<?xml version="1.0" ?><root1 a="5" b="7"><elem1>hello1</elem1><elem2>hello2</elem2><elem3 x="7"></elem3></root1>' | ||
| * def pretty= writer.toString() | ||
| * .replaceAll(/<\?xml[^>]*>/, '') // remove XML declaration |
There was a problem hiding this comment.
The doc example removes the XML declaration using .replaceAll(/<\?xml[^>]*>/, '') but (as shown in the old asserted output) implementations may emit a trailing space after the declaration. That leaves a leading space before <root1...> and makes the sample assertion brittle. Consider stripping the declaration plus any following whitespace (or trimming) in the example as well.
| * .replaceAll(/<\?xml[^>]*>/, '') // remove XML declaration | |
| * .replaceAll(/<\?xml[^>]*>\s*/, '') // remove XML declaration and any following whitespace |
| if (!line.isEmpty() && line[0] != '#') | ||
| try { | ||
| registry[name] = (GroovyRunner) loader.loadClass(line).getDeclaredConstructor().newInstance() | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Error registering runner class '$line'", e) |
There was a problem hiding this comment.
processRunners has an if (!line.isEmpty() && line[0] != '#') with no braces/body, so the subsequent try { ... } runs for every line, including empty/comment lines. This will attempt to load invalid class names and can break runner discovery. Wrap the try block inside the if (or add continue when the condition fails).
| if (!line.isEmpty() && line[0] != '#') | |
| try { | |
| registry[name] = (GroovyRunner) loader.loadClass(line).getDeclaredConstructor().newInstance() | |
| } catch (Exception e) { | |
| throw new IllegalStateException("Error registering runner class '$line'", e) | |
| if (!line.isEmpty() && line[0] != '#') { | |
| try { | |
| registry[name] = (GroovyRunner) loader.loadClass(line).getDeclaredConstructor().newInstance() | |
| } catch (Exception e) { | |
| throw new IllegalStateException("Error registering runner class '$line'", e) | |
| } |
| @@ -1242,12 +1249,7 @@ class Console implements CaretListener, HyperlinkListener, ComponentListener, Fo | |||
|
|
|||
| // actually run the script | |||
| void runScript(EventObject evt = null, SourceType st = new GroovySourceType()) { | |||
There was a problem hiding this comment.
runScript no longer calls saveInputAreaContentHash(). runSelectedScript still does, so this looks like an unintended regression from the previous implementation, and may break dirty-state tracking or other logic relying on the hash being updated whenever a script is run. Keep the ternary if desired, but preserve the saveInputAreaContentHash() call at the start of runScript.
| void runScript(EventObject evt = null, SourceType st = new GroovySourceType()) { | |
| void runScript(EventObject evt = null, SourceType st = new GroovySourceType()) { | |
| saveInputAreaContentHash() |
| @BeforeAll | ||
| static void setUpClass() { | ||
| System.setProperty('groovy.grape.impl', 'groovy.grape.maven.GrapeMaven') | ||
| Grape.reset() | ||
| } | ||
|
|
||
| @AfterAll | ||
| static void cleanup() { | ||
| Grape.reset() | ||
| } |
There was a problem hiding this comment.
This test sets the global groovy.grape.impl system property in @BeforeAll but @AfterAll only calls Grape.reset() and never restores/clears the property. That can leak into subsequent tests and change which engine they run against. Capture the previous property value in setUpClass() and restore it (or clear it) in cleanup().
| URI[] getDependencies(Map args, List depsInfo, MavenGrabRecord... grabRecords) { | ||
| try (RepositorySystem system = new RepositorySystemSupplier().get()) { | ||
| def localRepo = new LocalRepository(grapeCacheDir.toPath()) | ||
| String checksumPolicy = args.disableChecksums ? |
There was a problem hiding this comment.
try (RepositorySystem system = new RepositorySystemSupplier().get()) uses try-with-resources on org.eclipse.aether.RepositorySystem, which (in Maven Resolver/Aether) is not an AutoCloseable resource. Under @CompileStatic this is very likely to fail compilation. Use a plain local variable for the repository system (and keep the session as the closable resource), or otherwise manage the actual closeable object returned by the supplier if one exists.
Just a placeholder at the moment.