[
https://issues.apache.org/jira/browse/GROOVY-12382?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113283#comment-18113283
]
ASF GitHub Bot commented on GROOVY-12382:
-----------------------------------------
Copilot commented on code in PR #2904:
URL: https://github.com/apache/groovy/pull/2904#discussion_r3967743484
##########
src/main/java/org/codehaus/groovy/vmplugin/JavaFeatureVersion.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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 org.codehaus.groovy.vmplugin;
+
+/**
+ * The Java feature version of the running VM, e.g. {@code 17} (GROOVY-12382).
+ * <p>
+ * {@code Runtime.version()} is a Java 9 API that some runtimes built on the
+ * JDK class library do not provide (Android's ART, for one). Where it is
+ * missing, the {@code java.specification.version} property is parsed instead,
+ * and when that is unusable the lowest release Groovy supports is assumed.
+ * <p>
+ * Deliberately tiny and free of lambdas: it is consulted from static
+ * initialisers that can run while the system class loader is being
+ * instantiated ({@code
-Djava.system.class.loader=groovy.lang.GroovyClassLoader}),
+ * where an {@code invokedynamic} bootstrap is not yet permitted, and it must
+ * not trigger the creation of the VM plugin.
+ */
+public final class JavaFeatureVersion {
+
+ /** The lowest Java release Groovy runs on. */
+ public static final int MINIMUM = 17;
+
+ private JavaFeatureVersion() {
+ }
+
+ /**
+ * Returns the feature version of the running VM.
+ *
+ * @return the feature version, never below {@link #MINIMUM}
+ */
+ public static int current() {
+ try {
+ return Runtime.version().feature();
+ } catch (Throwable ignore) {
+ // NoSuchMethodError on a runtime without Runtime.version()
+ }
+ return parse(System.getProperty("java.specification.version"),
MINIMUM);
+ }
Review Comment:
Catching `Throwable` is overly broad here and can mask serious problems
(e.g., `OutOfMemoryError`, `StackOverflowError`). Since the intent is to
tolerate runtimes missing `Runtime.version()`, narrow the catch to
`NoSuchMethodError` (or possibly `LinkageError` if you want to cover related
linkage failures) and let other errors propagate.
##########
src/test/groovy/org/codehaus/groovy/vmplugin/VMPluginFactoryTest.groovy:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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 org.codehaus.groovy.vmplugin
+
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+import static org.junit.jupiter.api.Assertions.assertSame
+
+/**
+ * GROOVY-12382: the feature version must be available on runtimes without
+ * {@code Runtime.version()}, such as Android's ART, where the specification
+ * version property is the only clue and may itself be meaningless ("0.9").
+ */
+final class VMPluginFactoryTest {
+
+ @Test
+ void featureVersionMatchesTheRuntimeOnAJvm() {
+ assertEquals(Runtime.version().feature(),
VMPluginFactory.featureVersion())
+ assertEquals(Runtime.version().feature(), JavaFeatureVersion.current())
+ assertEquals(17, JavaFeatureVersion.MINIMUM)
+ assertEquals(Integer.toString(Runtime.version().feature()),
CompilerConfiguration.DEFAULT_TARGET_BYTECODE)
+ }
Review Comment:
This test hard-depends on `Runtime.version()` and also assumes
`CompilerConfiguration.DEFAULT_TARGET_BYTECODE` always equals the runtime
feature version. That assumption can break on newer Java releases if Groovy
clamps/normalizes to the latest supported bytecode in its internal map. To keep
the test aligned with the PR’s goal (tolerating runtimes without
`Runtime.version()`), avoid calling `Runtime.version()` in the test and assert
properties that are stable: e.g., that `VMPluginFactory.featureVersion()`
equals `JavaFeatureVersion.current()`, and that `DEFAULT_TARGET_BYTECODE`
parses as an integer >= `JavaFeatureVersion.MINIMUM` (or matches the expected
clamped behavior if that is the intended contract).
> VMPluginFactory: tolerate a runtime without Runtime.version()
> -------------------------------------------------------------
>
> Key: GROOVY-12382
> URL: https://issues.apache.org/jira/browse/GROOVY-12382
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Involves: a featureVersion() helper that tries Runtime.version(), then parses
> java.specification.version, then assumes the lowest supported release, which
> is the only plugin left anyway. Ten lines plus a unit test of the parsing.
> Also worth reusing in CompilerConfiguration.defaultTargetBytecode, which has
> the same call.
> Impact on normal usage: none. On every JVM the first branch succeeds as
> before.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
