Repository navigation
Convert newlines in text mode on Windows through the transcoder - #9707
Open
aminmansuri wants to merge 9 commits into
Open
aminmansuri wants to merge 9 commits into
aminmansuri wants to merge 9 commits into
Conversation
ruby/spec examples for CRLF on write and universal newline on read in text mode, JUnit coverage of the decorator selection with the platform passed in, and the Windows spec tags those examples replace. Red until the fix lands.
The JDK has no O_TEXT descriptors, so the default text mode MRI leaves to the C runtime did nothing here: text-mode reads now get the universal newline decorator, writes the CRLF one, and popen pipes the default text mode.
ARGF.binmode left the shared ARGF state in binmode for every later ARGF.class.new, which MRI does not do; on Windows that hid text mode from the ARGF specs. The File::BINARY flag check moves to the Windows tests.
The runner checks fixtures out with CRLF, so specs built on LF byte offsets and lengths fail there for MRI as well; the ARGF byte readers (each_byte, each_char, getc, readpartial) stay raw and are tagged too.
Its shared example is described as ARGF.getc, so the tag lives in the readchar tags file; the reader stays byte-level like ARGF.getc.
MRI's rb_w32_write only lets the C runtime insert CRs on a file or on stdout/stderr, so the default CRLF marker alone must not select the write converter on a pipe.
MRI's _write does not count the CRs it inserts.
System.console() is non-null on JDK 22-24 when the standard streams are redirected.
headius
requested changes
Oct 8, 2026
headius
left a comment
Member
There was a problem hiding this comment.
The logic looks good in a first review pass but I need some changes to fully approve it.
Along with the changes in review comments, rebase this on top of master and recent changes there (which includes a bunch of tag removals). We won't make this big of a change in the 10.0 cycle.
|
|
||
| // Poor man's isatty for stdio on Windows (jruby/jruby#3292). System.console() is non-null on JDK 22-24 | ||
| // even when the standard streams are redirected; Console#isTerminal (JDK 22+) tells the cases apart. | ||
| static boolean stdioIsConsole() { |
Member
There was a problem hiding this comment.
I believe we have similar code elsewhere for calling isTerminal. Unify.
|
|
||
| // MRI: part of prep_stdio | ||
| private static void prepStdioEcflags(OpenFile fptr, int fmode) { | ||
| // MRI: the newline decorators prep_stdio and pipe_open give an IO in the default text mode |
Member
There was a problem hiding this comment.
the "MRI: ..." comments are meant to connect this implementation with equivalent code in CRuby (see #9782 for a new Java annotation for this purpose). The additional comments added here should instead be part of javadoc for this method.
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.
Changes (Windows only):
Refs #9067 (the earlier approach) and #3742.