Fix headernt.c & -output-obj Unicode support by nojb · Pull Request #1362 · ocaml/ocaml

nojb · 2017-09-21T10:13:21Z

This is a follow-up to #1200. It does three things:

Fixes a bad rebase of read_runtime_path of headernt.c
Fixes the C signature of the argv argument in the C file generated by -output-obj
Uses WriteConsole in headernt.c to properly print Unicode pathnames in error messages.

Together with #1357 this should fix the remaining Windows testsuite errors.

/cc @dra27

nojb · 2017-09-21T15:01:40Z

Rebased on trunk.

damiendoligez

LGTM. It's up to you if you want to wait for a review from @dra27, who is away until Monday.

nojb · 2017-09-22T12:00:48Z

Thanks! No problem waiting for David's review (only downside is that AppVeyor will remain broken until then).

nojb · 2017-09-26T07:39:36Z

I was waiting for @dra27's review, but I think we could/should merge this now since it fixes two bugs and several testsuite failures. We can always revisit if necessary.

shindere · 2017-09-26T07:44:51Z

Nicolás Ojeda Bär (2017/09/26 07:39 +0000):

I was waiting for @dra27's review, but I think we could/should merge this now since it fixes two bugs and several testsuite failures. We can always revisit if necessary.

Agreed! I'm sure it can't make the situation worse it currently is so let's go aheand and fix later what may have to be fixed.

gasche · 2017-09-26T07:47:39Z

On the other hand, (1) this stuff seems tricky and (2) it's a pain to fix-after-the-fact when there is a release branch. Damien (or someone else) may want to merge it now, but I'll personally wait to merge unless @dra27 signals that he is too busy with other things.

dra27 · 2017-09-26T08:09:36Z

Sorry - fully back from vacation tomorrow (though I may have a chance to give this and #1363 a review later today)

alainfrisch · 2017-09-26T08:26:02Z

bytecomp/bytelink.ml

    (* The entry point *)
    output_string outchan "\
-\nvoid caml_startup(char ** argv)\
+\nvoid caml_startup(charnat ** argv)\


Should probably be renamed to char_os once the other PR is merged, right?

That's right, will update as necessary.

In the interests of getting at least one of our CI systems back up and running, can we merge this before #1363?

alainfrisch · 2017-09-26T08:27:51Z

stdlib/headernt.c

+  static char str[MAX_PATH];
+
+  if (GetConsoleMode(hOut, &consoleMode) != 0) { /* The output stream is a Console */
+    WriteConsoleW(hOut, wstr, wcslen(wstr), &numwritten, NULL);


Out of curiosity: do we actually need the 'W' suffix here? I thought we were now compiling in a mode where unprefixed functions are aliases for *W ones.

I think you are right. Will fix.

alainfrisch · 2017-09-26T08:33:51Z

stdlib/headernt.c

+  if (GetConsoleMode(hOut, &consoleMode) != 0) { /* The output stream is a Console */
+    WriteConsoleW(hOut, wstr, wcslen(wstr), &numwritten, NULL);
+  } else { /* The output stream is redirected */
+    len = WideCharToMultiByte(CP, 0, wstr, wcslen(wstr), str, sizeof(str), NULL, NULL);


It's very unimportant, but I wonder if in principle it wouldn't be better to translate to UTF-8 here (it could be that the displayed filename cannot be represented in the current 8-bit codepage). Or is the idea of an UTF-8 encoded text file crazy under Windows?

Sorry, I missed the fact that CP is indeed UTF-8 when OCaml is configured with Unicode support.

Absolutely I think it makes good sense! I just did it this way to preserve existing behavior.

nojb · 2017-09-26T10:45:53Z

Wohoo, all AppVeyor tests pass! @shindere @dra27

Travis failures seem unrelated.

shindere · 2017-09-26T10:54:15Z

Nicolás Ojeda Bär (2017/09/26 10:45 +0000):

Wohoo, all AppVeyor tests pass! @shindere @dra27

So nice! Hope Inria's CI will be as happy once this gets merged! Thank you so much for your hard work @nojb !

dra27

LGTM - cosmetic tweak to the exec_tests disabling only

dra27 · 2017-09-27T08:29:13Z

testsuite/tests/win-unicode/Makefile

 test:
 	@if echo 'let () = exit (if Config.windows_unicode then 0 else 1)' | $(OCAML) -I $(OTOPDIR)/utils config.cmo -stdin; then \
-	  $(MAKE) printargv.exe printenv.exe symlink_tests.precheck && \
+	  $(MAKE) printargv.exe printenv.exe symlink_tests.precheck exec_tests.precheck && \


This seems a little convoluted to me - why not just create exec_tests.precheck with exit 1 in it - the entry in .gitignore is then not necessary and lines 22-25 below can just be deleted?

The exec_tests.precheck file could also then include a comment as to why the tests are disabled (stability - which I will look into, as I have an idea what it is...)

Fixed as suggested, thanks!

dra27 · 2017-09-27T08:31:04Z

bytecomp/bytelink.ml

    (* The entry point *)
    output_string outchan "\
-\nvoid caml_startup(char ** argv)\
+\nvoid caml_startup(charnat ** argv)\


In the interests of getting at least one of our CI systems back up and running, can we merge this before #1363?

dra27 · 2017-09-27T08:33:41Z

Doesn't have to be done here - but are there tests we could add for the -output-obj failure and similarly for headernt producing garbage when passed Unicode?

dra27 · 2017-09-27T08:34:45Z

I've just popped this branch into Inria CI's precheck

dra27 · 2017-09-27T09:23:12Z

Lovely, thanks - let's let AppVeyor run through and then merge and cherry-pick. The Travis failures are identical to the current ones on trunk, so I agree that they don't appear to be related.

shindere · 2017-09-27T09:34:55Z

David Allsopp (2017/09/27 01:34 -0700):

I've just popped this branch into Inria CI's precheck

Thanks! Please let us now whether it works -- precheck was destroyed I don't know why and I had to re-create it from scratch.

shindere · 2017-09-27T09:37:39Z

David Allsopp (2017/09/27 09:23 +0000):

Lovely, thanks - let's let AppVeyor run through and then merge and cherry-pick. The Travis failures are identical to the current ones on trunk, so I agree that they don't appear to be related.

What's the problem exactly with Travis? Can't have a look at it, sorry.

nojb · 2017-09-27T09:39:42Z

@shindere wrote:

What's the problem exactly with Travis?

64-bit builds fail with dozens of errors like this one:

... testing 'fileio.ml': ocamlcFile "fileio.ml", line 1:
Error: Error on dynamically loaded library: /home/travis/build/ocaml/ocaml/testsuite/../otherlibs/unix/dllunix.so: /home/travis/build/ocaml/ocaml/testsuite/../otherlibs/unix/dllunix.so: undefined symbol: caml_atom_table

gasche · 2017-09-27T09:39:58Z

The errors all look similar to this one:

Running tests from 'tests/lib-bigarray' ...
... testing 'bigarrays.ml': ocamlcFile "bigarrays.ml", line 1:
Error: Error on dynamically loaded library: /home/travis/build/ocaml/ocaml/testsuite/../otherlibs/unix/dllunix.so: /home/travis/build/ocaml/ocaml/testsuite/../otherlibs/unix/dllunix.so: undefined symbol: caml_atom_table

This is unrelated to the present PR, but it's a recent breakage. I'll send an email.

shindere · 2017-09-27T09:45:35Z

Thanks, @nojb. Trying to reproduce this here.

nojb · 2017-09-27T09:50:31Z

Thanks!

dra27 · 2017-09-27T09:54:45Z

Cherry-picked to 4.06 in 36e0a3b

nojb changed the base branch from 4.06 to trunk September 21, 2017 14:53

nojb force-pushed the fix_headernt_unicode branch from b661514 to 2d242fa Compare September 21, 2017 14:59

damiendoligez approved these changes Sep 22, 2017

View reviewed changes

damiendoligez added this to the 4.06.0 milestone Sep 22, 2017

nojb mentioned this pull request Sep 23, 2017

Fix ocamltest / Windows / Unicode issues #1357

Merged

alainfrisch reviewed Sep 26, 2017

View reviewed changes

nojb added 3 commits September 26, 2017 10:33

headernt.c: fix read_runtime_path

5ab7643

Fix -output-obj C file argv signature

1f04d7b

headernt.c: use WriteConsole

43553f9

alainfrisch reviewed Sep 26, 2017

View reviewed changes

nojb force-pushed the fix_headernt_unicode branch from 2d242fa to 43553f9 Compare September 26, 2017 08:38

Disable win-unicode/exec_tests test

8411ed3

dra27 requested changes Sep 27, 2017

View reviewed changes

Simplify exec_tests.precheck

844ef51

dra27 approved these changes Sep 27, 2017

View reviewed changes

gasche mentioned this pull request Sep 27, 2017

Fix naming of shared Unicode stubs #1363

Merged

dra27 merged commit 99878e4 into ocaml:trunk Sep 27, 2017

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Add BeSport success story (ocaml#1362)

297d5f9

Conversation

nojb commented Sep 21, 2017

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

damiendoligez left a comment

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 22, 2017

Uh oh!

nojb commented Sep 26, 2017

Uh oh!

shindere commented Sep 26, 2017 via email

Uh oh!

gasche commented Sep 26, 2017

Uh oh!

dra27 commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 26, 2017

Uh oh!

shindere commented Sep 26, 2017 via email

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

shindere commented Sep 27, 2017 via email

Uh oh!

shindere commented Sep 27, 2017 via email

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

gasche commented Sep 27, 2017

Uh oh!

shindere commented Sep 27, 2017 via email

Uh oh!

nojb commented Sep 27, 2017

Uh oh!

dra27 commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants