Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sys.print duplicates CR on Windows #8379

Open
1 of 4 tasks
Aurel300 opened this issue Jun 5, 2019 · 9 comments
Open
1 of 4 tasks

Sys.print duplicates CR on Windows #8379

Aurel300 opened this issue Jun 5, 2019 · 9 comments
Assignees
Labels
bug platform-cpp Everything related to CPP / C++ platform-hl Everything related to HashLink
Milestone

Comments

@Aurel300
Copy link
Member

Aurel300 commented Jun 5, 2019

class Print {
  public static function main():Void
    Sys.print("foo\r\n");
}

On OS X:

$ haxe --run Print | xxd
0000000: 666f 6f0d 0a                             foo..

On Windows:

$ haxe --run Print | hexdump -C
00000000 66 6f 6f 0d 0d 0a                       |foo...|
00000006

Note the duplicate 0d byte.

@Aurel300 Aurel300 added bug platform-eval Everything related to the Haxe 4 eval macro interpreter labels Jun 5, 2019
@Aurel300 Aurel300 added this to the Release 4.0 milestone Jun 5, 2019
@Simn
Copy link
Member

Simn commented Jun 5, 2019

I think it must actually be Sys.print causing that.

@Aurel300 Aurel300 changed the title [eval] Bytes.ofString(...).toString() duplicates CR on Windows [eval] Sys.print duplicates CR on Windows Jun 5, 2019
@Aurel300
Copy link
Member Author

Aurel300 commented Jun 5, 2019

Whoops, you're right.

@Aurel300 Aurel300 changed the title [eval] Sys.print duplicates CR on Windows Sys.print duplicates CR on Windows Jun 6, 2019
@Aurel300 Aurel300 added platform-cpp Everything related to CPP / C++ platform-hl Everything related to HashLink and removed platform-eval Everything related to the Haxe 4 eval macro interpreter labels Jun 6, 2019
@Simn
Copy link
Member

Simn commented Jun 19, 2019

@Aurel300 Can you handle this or does someone else need to do something?

@hughsando
Copy link
Member

I'm not sure there is a problem here.
For starters, the slash-r in the example is a bit confusing - is it needed to illustrate the issue?
On windows if you do: printf("foo\n"); you get 5 characters. Crap as this might be, this is standard behaviour - ie text mode by default. And if it not supposed to be text mode, why put all the slash-rs back in? The should just be slash-n.

@Simn
Copy link
Member

Simn commented Jun 27, 2019

I don't find turning "\r\n" into "\r\r\n" when printing an acceptable default behavior.

@Aurel300
Copy link
Member Author

Aurel300 commented Jun 27, 2019

@Simn I think Hugh is suggesting just going with \n on Windows as well.

As much as I hate it, CRLF is a convention on Windows platforms and Sys.println should output a CRLF-style linebreak when outputting to stdout. The problem is in situations like in the OP, where the input could come from another program (that is how I spotted it in the Unicode tests in the first place). Most importantly, if we kept text mode, just "piping" binary data would not work as expected, since it would clobber any 0x0A character.

Oh, and of course – the fixed behaviour is already the case on the majority of sys platforms.

@hughsando
Copy link
Member

Well, it is really only, and always, turning "\n" into "\r\n". This is the whole difference between fopen(name,"r") and fopen(name,"rb") - 50 years of legacy catching up with us. We most certainly do not want to explicitly add the "\r", because this does not allow is to ever get rid of them. I think the solution you are after is to set the output mode to binary (on request).

@Aurel300
Copy link
Member Author

Aurel300 commented Jul 1, 2019

Ok, after some discussion with @hughsando:

Current system

Currently, stdout and stderr are open in text mode on Windows. This makes Sys.println("a\nb"); work "as expected", i.e. a<CR><LF>b is output. However, Sys.println("a\r\nb"); outputs a<CR><CR><LF>b, which is problematic when the string comes from a subprocess.

PR HaxeFoundation/hxcpp#820

In the PR I switched stdout and stderr to binary mode, then made Sys.println append "\r\n" on Windows. This fixes the issue with subprocesses, but breaks Sys.println("a\nb");.


We could solve this instead by adding Sys.setBinaryMode(binary:Bool); to stdlib. It is a bit annoying, since this will only have any effect on Windows, neither of the approaches above is good enough; \n inside Sys.println calls is presumably a common construct, which we don't want to break. On Windows, we will default to text mode to maintain backwards compatibility. Later we could think about auto-detecting stdout and setting the mode to something appropriate.

@Simn Simn modified the milestones: Release 4.0, Release 4.1 Jul 1, 2019
@RealyUniqueName
Copy link
Member

I'd say it doesn't break Sys.println("a\nb"); but fixes it. E.g. I always explicitly type \r\n when I want windows-style line endings.

@Simn Simn modified the milestones: Release 4.1, Release 4.2 Feb 19, 2020
@RealyUniqueName RealyUniqueName modified the milestones: Release 4.2, Bugs Dec 14, 2020
tobil4sk added a commit to tobil4sk/haxec that referenced this issue Feb 20, 2022
Simn added a commit that referenced this issue Apr 11, 2022
* [CI] Include `sqlite.hdll` in hl test build

This way it can be tested

Specify sqlite version for hl

* [CI] Remove bullet from hashlink build

* [CI] Enable more hashlink tests

* [CI] Enable a unicode test for hl and cpp

Test related to #8379

* [CI] Fix php sqlite and socket tests

Enable relevant packages in php config

* [CI] Enable more php tests

Enable php test for #5078

PHP's socket module is now enabled, so we can run socket tests

* [CI] Unspecify libsqlite3-dev version for hl build

Co-authored-by: Simon Krajewski <simon@haxe.org>
@Simn Simn modified the milestones: Bugs, Later Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug platform-cpp Everything related to CPP / C++ platform-hl Everything related to HashLink
Projects
None yet
Development

No branches or pull requests

4 participants