refactor: drop the java.logging module dependency and use System.Logger instead#1934
Merged
Conversation
…er instead Logging through java.util.logging forces the consumers to depend on the java.logging module. Switch all internal logging to System.Logger (in java.base) and drop "requires java.logging" from every module-info. Level mapping: FINEST -> TRACE, FINE -> DEBUG, INFO and WARNING stay the same, SEVERE -> ERROR. Runtime behavior is unchanged. When java.logging is present, System.Logger routes to the "org.jline" JUL logger as before, so existing logging.properties configuration keeps working. DumbTerminalWarningTest still uses java.util.logging to capture and assert on log records. The unused console-handler setup in the reader tests was removed.
Contributor
📝 WalkthroughWalkthroughJLine migrates its entire logging infrastructure from ChangesSystem.Logger API Migration
Sequence Diagram(s)This PR does not contain control-flow changes or new component interactions that would benefit from sequence diagram visualization. The changes are primarily API-level migrations where logging statements route through a different backend without altering the execution order or business logic. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/docs/troubleshooting.md`:
- Around line 815-817: Update the documentation mapping so JLine's `trace` level
maps to JUL `FINEST` (not `FINER`) to match the migration contract; change the
mapping line that currently reads "trace → FINER" to "trace → FINEST" and update
the recommendation text to suggest using `org.jline.level=FINEST` (or
`FINER`/`FINE` as alternatives) when describing how to enable trace-level output
for JLine-to-JUL configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54534b41-dadb-41f0-af1d-8060d2e572ca
📒 Files selected for processing (32)
builtins/src/main/java/module-info.javaconsole-ui/src/main/java/module-info.javaconsole/src/main/java/module-info.javajansi-core/src/main/java/module-info.javanative/src/main/java/module-info.javanative/src/main/java/org/jline/nativ/JLineNativeLoader.javanative/src/main/java/org/jline/nativ/OSInfo.javaprompt/src/main/java/module-info.javareader/src/main/java/module-info.javareader/src/test/java/org/jline/keymap/BindingReaderTest.javareader/src/test/java/org/jline/keymap/KeyMapTest.javareader/src/test/java/org/jline/reader/impl/ReaderTestSupport.javaremote-ssh/src/main/java/org/jline/builtins/ssh/ShellCommand.javaremote-telnet/src/main/java/module-info.javaremote-telnet/src/main/java/org/jline/builtins/telnet/Connection.javaremote-telnet/src/main/java/org/jline/builtins/telnet/ConnectionManager.javaremote-telnet/src/main/java/org/jline/builtins/telnet/PortListener.javaremote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.javastyle/src/main/java/module-info.javastyle/src/main/java/org/jline/style/MemoryStyleSource.javastyle/src/main/java/org/jline/style/StyleBundleInvocationHandler.javastyle/src/main/java/org/jline/style/Styler.javaterminal-ffm/src/main/java/module-info.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/CLibrary.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.javaterminal/pom.xmlterminal/src/main/java/module-info.javaterminal/src/main/java/org/jline/utils/Log.javaterminal/src/main/java/org/jline/utils/NonBlockingInputStream.javaterminal/src/main/java/org/jline/utils/NonBlockingReader.javaterminal/src/main/java/org/jline/utils/StyleResolver.javawebsite/docs/troubleshooting.md
💤 Files with no reviewable changes (14)
- remote-telnet/src/main/java/module-info.java
- style/src/main/java/module-info.java
- jansi-core/src/main/java/module-info.java
- console/src/main/java/module-info.java
- terminal/src/main/java/module-info.java
- prompt/src/main/java/module-info.java
- reader/src/test/java/org/jline/keymap/BindingReaderTest.java
- console-ui/src/main/java/module-info.java
- builtins/src/main/java/module-info.java
- terminal-ffm/src/main/java/module-info.java
- reader/src/main/java/module-info.java
- reader/src/test/java/org/jline/reader/impl/ReaderTestSupport.java
- native/src/main/java/module-info.java
- reader/src/test/java/org/jline/keymap/KeyMapTest.java
gnodet
reviewed
Jun 14, 2026
gnodet
left a comment
Member
There was a problem hiding this comment.
Clean, well-executed migration. Dropping the java.logging module dependency is a meaningful improvement for consumers building trimmed jlink images — System.Logger (part of java.base since Java 9) is the right abstraction here.
What I verified:
- Java version:
System.Loggerhas been injava.basesince Java 9. JLine targets Java 11+, so no compatibility concern. - Public API:
Log.java's public interface (trace,debug,info,warn,error,isDebugEnabled) is unchanged. No downstream breakage. - Level mapping correctness:
FINEST→TRACE,FINE→DEBUG,SEVERE→ERRORacross all 32 files — verified consistent. - Module-info: All 11
requires java.loggingdeclarations removed. Tests correctly add--add-modules java.logging --add-readsfor test compilation. - Message format: Files using
{0},{1}JUL-style placeholders (e.g.,FfmSignalHandler.java) continue to work becauseSystem.Logger's default JUL backend delegates toMessageFormat. If a user switches to a non-JULLoggerFinder, these placeholders would need updating, but that's inherent toSystem.Loggerand not a concern for this PR. - Documentation: Troubleshooting guide updated with level mapping table and
jlinknote — good.
Two minor observations:
-
Log.javamessage rendering: TheByteArrayOutputStream/PrintStreamallocation inlog(Level, Object...)happens on every call that passes the level check. This was the same behavior before, so no regression — but for hot paths,Supplier<String>variants should be preferred by callers. -
Logger naming: The core
Log.javauses"org.jline"while module-specific loggers (telnet, style, FFM) use class-name-based logger names (e.g.,Connection.class.getName()). This is fine — it's the existing pattern — but worth noting that log configuration needs to account for both naming styles.
No blocking issues. Well done.
Review by Claude Code on behalf of Guillaume Nodet
|
|
||
| <properties> | ||
| <automatic.module.name>org.jline.terminal</automatic.module.name> | ||
| <surefire.argLine>--add-opens java.base/java.io=org.jline.nativ --add-opens java.base/java.io=org.jline.terminal --add-modules java.logging --add-reads org.jline.terminal=java.logging</surefire.argLine> |
Member
There was a problem hiding this comment.
Good: test compilation still gets java.logging via --add-modules/--add-reads, keeping the test infra working while the main code is free of the dependency.
Review by Claude Code on behalf of Guillaume Nodet
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Logging through java.util.logging forces the consumers to depend on the java.logging module. Switch all internal logging to System.Logger (in java.base) and drop "requires java.logging" from every module-info.
Level mapping: FINEST -> TRACE, FINE -> DEBUG, INFO and WARNING stay the same, SEVERE -> ERROR.
Runtime behavior is unchanged. When java.logging is present, System.Logger routes to the "org.jline" JUL logger as before, so existing logging.properties configuration keeps working.
DumbTerminalWarningTest still uses java.util.logging to capture and assert on log records. The unused
console-handler setup in the reader tests was removed.
Summary by CodeRabbit
Release Notes
Chores
Documentation