[dart:io][windows] Make unicode characters display correctly.

[dart:io][windows] Make unicode characters display correctly.

This change has two pieces.
- Set the console code page to UTF8.
  - This makes strings printed with print() display correctly
- Set the file translation mode to _O_WTEXT when writing to a
  stdout or stderr that is connected to a console.
  - This makes strings printed with e.g. stdout.writeln()
    display correctly.

fixes #28571

R=asiva@google.com, fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/bc079538666c5a9b1fff2f4848baf2ddef985932

[zra, 2017-02-17 16:36:25]

Description was changed from

==========
[dart:io] Allow translation mode to be set on a file

fixes #28571
==========

to

==========
[dart:io][windows] Make unicode characters display correctly.

This change has two pieces.
- Set the console code page to UTF8.
  - This makes strings printed with print() display correctly
- Set the file translation mode to _O_WTEXT when writing to a
  stdout or stderr that is connected to a console.
  - This makes strings printed with e.g. stdout.writeln()
    display correctly.

fixes #28571
==========

[zra, 2017-02-17 16:37:49]

zra@google.com changed reviewers:
+ asiva@google.com, fschneider@google.com

[Florian Schneider, 2017-02-21 20:34:51]

Thanks for fixing this!

Lgtm w/ comment.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:45: void set_binary(bool binary) {
SetBinary? - since it is not a simple setter

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/platform_w...
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/platform_w...
runtime/bin/platform_win.cc:134: SetConsoleOutputCP(old_output_cp);
Can there be multiple threads calling Platform::Exit? If yes, another thread
could have reset old_output_cp to -1 before this call here.

[zra, 2017-02-21 21:26:46]

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:45: void set_binary(bool binary) {
On 2017/02/21 20:34:50, Florian Schneider wrote:
> SetBinary? - since it is not a simple setter

Done.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/platform_w...
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/platform_w...
runtime/bin/platform_win.cc:134: SetConsoleOutputCP(old_output_cp);
On 2017/02/21 20:34:50, Florian Schneider wrote:
> Can there be multiple threads calling Platform::Exit? If yes, another thread
> could have reset old_output_cp to -1 before this call here.

Yes. This can happen if multiple isolates call Platform.exit() at the same time.
I've added a mutex to protect old_output_cp.

[Florian Schneider, 2017-02-21 21:42:25]

lgtm

[siva, 2017-02-22 00:58:45]

lgtm with comments.

Can a test be added to verify this?

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file.cc#ne...
runtime/bin/file.cc:83: DartUtils::GetIntptrValue(Dart_GetNativeArgument(args,
1));
Dart_GetNativeIntegerArgument might work here, just one transition into the VM.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_andro...
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_andro...
runtime/bin/file_android.cc:135: void File::SetTranslation(DartFileTranslation
translation) {}
Maybe add a comment here stating there is nothing to on the targets where it is
a NOP?

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:327: // racing with other Isolates' calls to _setmode()
and _write(). When the
Not sure I understand this comment, did we already have a bug with respect to
_write()

[zra, 2017-02-22 05:07:16]

The new behavior is only triggered when stdout and stderr are attached to a tty,
so we can't test from within our test harness, but I've added a test that passes
utf8 strings to print, stdout.writeln, and stdout.add as a sanity check since I
couldn't find an existing one.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file.cc#ne...
runtime/bin/file.cc:83: DartUtils::GetIntptrValue(Dart_GetNativeArgument(args,
1));
On 2017/02/22 00:58:45, siva wrote:
> Dart_GetNativeIntegerArgument might work here, just one transition into the
VM.

Done.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_andro...
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_andro...
runtime/bin/file_android.cc:135: void File::SetTranslation(DartFileTranslation
translation) {}
On 2017/02/22 00:58:45, siva wrote:
> Maybe add a comment here stating there is nothing to on the targets where it
is
> a NOP?

Done.

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/2698813002/diff/100001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:327: // racing with other Isolates' calls to _setmode()
and _write(). When the
On 2017/02/22 00:58:45, siva wrote:
> Not sure I understand this comment, did we already have a bug with respect to
> _write()

Tried to clarify. Without the _dup, calls to _setmode and _write on the same
file descriptor from different threads could be interleaved.

There was no race before because there was only the call to _write().

[zra, 2017-02-22 06:47:08]

Description was changed from

==========
[dart:io][windows] Make unicode characters display correctly.

This change has two pieces.
- Set the console code page to UTF8.
  - This makes strings printed with print() display correctly
- Set the file translation mode to _O_WTEXT when writing to a
  stdout or stderr that is connected to a console.
  - This makes strings printed with e.g. stdout.writeln()
    display correctly.

fixes #28571
==========

to

==========
[dart:io][windows] Make unicode characters display correctly.

This change has two pieces.
- Set the console code page to UTF8.
  - This makes strings printed with print() display correctly
- Set the file translation mode to _O_WTEXT when writing to a
  stdout or stderr that is connected to a console.
  - This makes strings printed with e.g. stdout.writeln()
    display correctly.

fixes #28571

R=asiva@google.com, fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bc079538666c5a9b1fff2f4848baf2ddef985932
==========

[zra, 2017-02-22 06:47:09]

Committed patchset #8 (id:140001) manually as
bc079538666c5a9b1fff2f4848baf2ddef985932 (presubmit successful).

[ahe, 2017-02-23 10:01:43]

On 2017/02/22 05:07:16, zra wrote:
> The new behavior is only triggered when stdout and stderr are attached to a
tty,
> so we can't test from within our test harness, but I've added a test that
passes
> utf8 strings to print, stdout.writeln, and stdout.add as a sanity check since
I
> couldn't find an existing one.

I don't think the test works correctly on Android, and breaks the test runner.

Also, running something attached to a tty on Windows must mean running it inside
cmd.exe. That's possible by writing a test that launches cmd.exe as an external
process and tells cmd.exe to run dart.exe given a script that triggers the
desired behavior.

More details are available here:
https://technet.microsoft.com/en-us/library/cc771320(v=ws.11).aspx

[ahe, 2017-02-23 10:08:36]

On 2017/02/23 10:01:43, ahe wrote:
> On 2017/02/22 05:07:16, zra wrote:
> > The new behavior is only triggered when stdout and stderr are attached to a
> tty,
> > so we can't test from within our test harness, but I've added a test that
> passes
> > utf8 strings to print, stdout.writeln, and stdout.add as a sanity check
since
> I
> > couldn't find an existing one.
> 
> I don't think the test works correctly on Android, and breaks the test runner.
> 
> Also, running something attached to a tty on Windows must mean running it
inside
> cmd.exe. That's possible by writing a test that launches cmd.exe as an
external
> process and tells cmd.exe to run dart.exe given a script that triggers the
> desired behavior.
> 
> More details are available here:
> https://technet.microsoft.com/en-us/library/cc771320(v=ws.11).aspx

When I run this:

./xcodebuild/ReleaseX64/dart tests/standalone/io/console_unicode_test.dart |
iconv -t utf8 -f utf8

I see this:

These are three black spades: ♠♠♠
German characters: aäbcdefghijklmnoöpqrsßtuüvwxyz
These are three black spades: ♠♠♠
German characters: aäbcdefghijklmnoöpqrsßtuüvwxyz
These are three black spades: ```
German characters: a
iconv: (stdin):6:20: cannot convert