[
https://issues.apache.org/jira/browse/GROOVY-11871?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18065228#comment-18065228
]
ASF GitHub Bot commented on GROOVY-11871:
-----------------------------------------
Copilot commented on code in PR #2140:
URL: https://github.com/apache/groovy/pull/2140#discussion_r2921432290
##########
subprojects/groovy-console/src/main/groovy/groovy/console/ui/Console.groovy:
##########
@@ -378,6 +378,13 @@ class Console implements CaretListener, HyperlinkListener,
ComponentListener, Fo
} catch (ClassNotFoundException ignore) {
}
+ // listen for Maven resolver events if the Maven console plugin is on
the classpath
+ try {
+ def mavenPluginClass =
Class.forName('groovy.console.ui.ConsoleMavenPlugin')
+ mavenPluginClass.getConstructor().newInstance().addListener(this)
+ } catch (ClassNotFoundException ignore) {
Review 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.
```suggestion
} catch (ReflectiveOperationException ignore) {
```
##########
versions.properties:
##########
@@ -48,6 +48,8 @@ junit5Platform=1.14.3
log4j=1.2.17
log4j2=2.25.3
logback=1.5.27
+mavenResolverProvider=4.0.0-rc-5
+mavenResolverSupplier=2.0.16
Review Comment:
The PR description says "Just a placeholder at the moment", but this PR
introduces a new Maven-based Grape engine subproject plus CLI/console/test
changes. Please update the PR description to reflect the implemented scope and
any remaining TODOs so reviewers know what's intended for this change set.
##########
src/main/java/groovy/grape/Grape.java:
##########
@@ -119,9 +121,12 @@ public static void setDisableChecksums(boolean
disableChecksums) {
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) {
}
Review 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.
##########
src/main/groovy/groovy/grape/GrapeUtil.groovy:
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.grape
+
+import groovy.transform.CompileStatic
+import org.apache.groovy.plugin.GroovyRunner
+import org.apache.groovy.plugin.GroovyRunnerRegistry
+import org.codehaus.groovy.reflection.CachedClass
+import org.codehaus.groovy.reflection.ClassInfo
+import org.codehaus.groovy.runtime.m12n.ExtensionModuleScanner
+import org.codehaus.groovy.runtime.metaclass.MetaClassRegistryImpl
+
+import java.util.jar.JarFile
+import java.util.zip.ZipEntry
+import java.util.zip.ZipException
+import java.util.zip.ZipFile
+
+/**
+ * Utility methods shared between GrapeIvy and GrapeMaven implementations.
+ */
+@CompileStatic
+class GrapeUtil {
+
+ private static final String METAINF_PREFIX = 'META-INF/services/'
+ private static final String RUNNER_PROVIDER_CONFIG = GroovyRunner.name
+
+ /**
+ * Adds a URI to a classloader's classpath via reflection.
+ */
+ 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())
+ }
Review 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`.
##########
src/test/groovy/org/codehaus/groovy/runtime/m12n/ExtensionModuleHelperForTests.groovy:
##########
@@ -53,7 +53,8 @@ final class ExtensionModuleHelperForTests {
def ant = new AntBuilder()
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]+\\]'
]
Review 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.
##########
subprojects/groovy-grape-maven/build.gradle:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'org.apache.groovy-library'
+}
+
+dependencies {
+ 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}"
Review 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.
```suggestion
testRuntimeOnly "org.slf4j:slf4j-simple:${versions.slf4j}"
```
> Support Maven Resolver based version of Grapes
> ----------------------------------------------
>
> Key: GROOVY-11871
> URL: https://issues.apache.org/jira/browse/GROOVY-11871
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
