You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/09/09 06:58:23 UTC
[PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
paulk-asert opened a new pull request, #2908:
URL: https://github.com/apache/groovy/pull/2908
…initialisers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2908:
URL: https://github.com/apache/groovy/pull/2908#issuecomment-5600678178
## ✅ All tests passed ✅
🏷️ Commit: 12b85e080ff76ed6b2af6339f6d046c2039c4bc2
▶️ Tests: 115116 executed
⚪️ Checks: 33/33 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2908:
URL: https://github.com/apache/groovy/pull/2908#issuecomment-5598805801
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2908?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `33.33333%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.1862%. Comparing base ([`17b99a8`](https://app.codecov.io/gh/apache/groovy/commit/17b99a8c6c942b207257090e7ce795b34f58d8b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`12b85e0`](https://app.codecov.io/gh/apache/groovy/commit/12b85e080ff76ed6b2af6339f6d046c2039c4bc2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2908?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...va/org/apache/groovy/json/internal/Exceptions.java](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&filepath=subprojects%2Fgroovy-json%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgroovy%2Fjson%2Finternal%2FExceptions.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWpzb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dyb292eS9qc29uL2ludGVybmFsL0V4Y2VwdGlvbnMuamF2YQ==) | 33.3333% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...main/java/org/apache/groovy/json/internal/Sys.java](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&filepath=subprojects%2Fgroovy-json%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgroovy%2Fjson%2Finternal%2FSys.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWpzb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dyb292eS9qc29uL2ludGVybmFsL1N5cy5qYXZh) | 33.3333% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2908 +/- ##
==================================================
- Coverage 71.1943% 71.1862% -0.0082%
+ Complexity 37632 37626 -6
==================================================
Files 1581 1581
Lines 136251 136261 +10
Branches 25311 25313 +2
==================================================
- Hits 97003 96999 -4
- Misses 30511 30520 +9
- Partials 8737 8742 +5
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2908?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...main/java/org/apache/groovy/json/internal/Sys.java](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&filepath=subprojects%2Fgroovy-json%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgroovy%2Fjson%2Finternal%2FSys.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWpzb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dyb292eS9qc29uL2ludGVybmFsL1N5cy5qYXZh) | `35.2941% <33.3333%> (-1.3726%)` | :arrow_down: |
| [...va/org/apache/groovy/json/internal/Exceptions.java](https://app.codecov.io/gh/apache/groovy/pull/2908?src=pr&el=tree&filepath=subprojects%2Fgroovy-json%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgroovy%2Fjson%2Finternal%2FExceptions.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWpzb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dyb292eS9qc29uL2ludGVybmFsL0V4Y2VwdGlvbnMuamF2YQ==) | `88.7324% <33.3333%> (-2.0368%)` | :arrow_down: |
... and [7 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2908/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2908:
URL: https://github.com/apache/groovy/pull/2908#discussion_r3967752455
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Exceptions.java:
##########
@@ -31,7 +31,23 @@
*/
public class Exceptions {
- private static final System.Logger LOGGER = System.getLogger(Exceptions.class.getName());
+ /**
+ * Holds the logger so that loading this class does not resolve
+ * {@code System.Logger}, a Java 9 API absent from some runtimes built on
+ * the JDK class library (Android's ART); where it is unavailable the
+ * exception prints its stack trace the ordinary way (GROOVY-12386).
+ */
+ private static final class Log {
+ static final System.Logger LOGGER = create();
+
+ private static System.Logger create() {
+ try {
+ return System.getLogger(Exceptions.class.getName());
+ } catch (Throwable t) {
+ return null;
+ }
+ }
+ }
Review Comment:
This change aims to avoid requiring the Java 9 logging API on runtimes like Android, but the nested `Log` class still has hard bytecode references to `System.Logger` (and likely `System.Logger.Level` via `ERROR`). On runtimes where `System.Logger` is absent, loading `Exceptions$Log` will fail with a linkage error before `create()` can run and before `catch (Throwable)` can return `null`, so the fallback path in `printStackTrace()` won’t be reached. To make the fallback effective, avoid direct type references by using reflection (store the logger as `Object` and invoke `log` reflectively, including resolving `java.lang.System$Logger$Level` by name), or isolate the System.Logger-based implementation into a Java 9+ specific artifact (e.g., MR-JAR / multi-release split) and keep the base runtime free of `System.Logger` symbols.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Exceptions.java:
##########
@@ -234,10 +250,14 @@ public void printStackTrace(PrintWriter s) {
*/
@Override
public void printStackTrace() {
- LOGGER.log(ERROR, this.getMessage());
+ if (Log.LOGGER == null) {
+ super.printStackTrace();
+ return;
+ }
+ Log.LOGGER.log(ERROR, this.getMessage());
if (getCause() != null) {
- LOGGER.log(ERROR, "This Exception was wrapped, the original exception stack trace is:", getCause());
+ Log.LOGGER.log(ERROR, "This Exception was wrapped, the original exception stack trace is:", getCause());
Review Comment:
This change aims to avoid requiring the Java 9 logging API on runtimes like Android, but the nested `Log` class still has hard bytecode references to `System.Logger` (and likely `System.Logger.Level` via `ERROR`). On runtimes where `System.Logger` is absent, loading `Exceptions$Log` will fail with a linkage error before `create()` can run and before `catch (Throwable)` can return `null`, so the fallback path in `printStackTrace()` won’t be reached. To make the fallback effective, avoid direct type references by using reflection (store the logger as `Object` and invoke `log` reflectively, including resolving `java.lang.System$Logger$Level` by name), or isolate the System.Logger-based implementation into a Java 9+ specific artifact (e.g., MR-JAR / multi-release split) and keep the base runtime free of `System.Logger` symbols.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Sys.java:
##########
@@ -29,7 +29,23 @@
*/
class Sys {
- private static final System.Logger LOGGER = System.getLogger(Sys.class.getName());
+ /**
+ * Holds the logger so that loading this class does not resolve
+ * {@code System.Logger}, a Java 9 API absent from some runtimes built on
+ * the JDK class library (Android's ART); the logger is only needed on a
+ * failure path (GROOVY-12386).
+ */
+ private static final class Log {
+ static final System.Logger LOGGER = create();
+
+ private static System.Logger create() {
+ try {
+ return System.getLogger(Sys.class.getName());
+ } catch (Throwable t) {
+ return null;
+ }
+ }
+ }
Review Comment:
Catching `Throwable` here will also swallow fatal VM conditions (e.g., `OutOfMemoryError`, `StackOverflowError`) and turn them into a silent `null` logger. If you keep the try/catch approach, narrow the catch to the expected linkage failures (e.g., `LinkageError`/`NoSuchMethodError`) and optionally `SecurityException`, or rethrow `VirtualMachineError` (and possibly `ThreadDeath`) to avoid masking unrecoverable failures.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Sys.java:
##########
@@ -55,7 +71,9 @@ class Sys {
v = new BigDecimal("1.9");
}
} catch (Exception ex) {
- LOGGER.log(WARNING, "Unable to determine build number or version", ex);
+ if (Log.LOGGER != null) {
+ Log.LOGGER.log(WARNING, "Unable to determine build number or version", ex);
+ }
Review Comment:
Catching `Throwable` here will also swallow fatal VM conditions (e.g., `OutOfMemoryError`, `StackOverflowError`) and turn them into a silent `null` logger. If you keep the try/catch approach, narrow the catch to the expected linkage failures (e.g., `LinkageError`/`NoSuchMethodError`) and optionally `SecurityException`, or rethrow `VirtualMachineError` (and possibly `ThreadDeath`) to avoid masking unrecoverable failures.
##########
subprojects/groovy-json/src/test/groovy/org/apache/groovy/json/internal/LazyLoggerTest.groovy:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.apache.groovy.json.internal
+
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertNotNull
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12386: the internal classes create their {@code System.Logger} lazily,
+ * in a nested holder, so that loading them does not require the Java 9 logging
+ * API, which Android's runtime lacks.
+ */
+class LazyLoggerTest {
+
+ @Test
+ void loggersLiveInNestedHoldersNotInTheClassesThemselves() {
+ [Sys, Exceptions].each { owner ->
+ assertTrue(owner.declaredFields.every { it.type != System.Logger }, owner.name)
+ def holder = owner.declaredClasses.find { it.simpleName == 'Log' }
+ assertNotNull(holder, owner.name)
+ def field = holder.getDeclaredField('LOGGER')
+ field.accessible = true
+ assertNotNull(field.get(null), 'the logger resolves on a JVM')
Review Comment:
Using `field.accessible = true` can fail under stronger access checks (e.g., `InaccessibleObjectException` in some Java 9+ module configurations). Consider using `trySetAccessible()` (when available) and asserting it succeeded (or skipping the assertion with an assumption) to avoid brittle test failures in more restricted runtimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2908:
URL: https://github.com/apache/groovy/pull/2908#issuecomment-5609046314
## ✅ All tests passed ✅
🏷️ Commit: 4443e2d4500629914ff42622a5812c815e889579
▶️ Tests: 117685 executed
⚪️ Checks: 33/33 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12386: groovy-json: keep System.Logger out of internal static … [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2908:
URL: https://github.com/apache/groovy/pull/2908
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org