Fix ocamltest / Windows / Unicode issues by nojb · Pull Request #1357 · ocaml/ocaml

nojb · 2017-09-19T14:34:43Z

This is a first PR to fix some of the CI errors under Windows following the merge of #1200 and #681. ~~It is not yet ready for merging.~~

The first commit makes ocamltest a Unicode Windows application.
The second commit fixes a bug in Unicode support for the Windows runtime: Let's do it! #1200. (I think due to a bad rebase; ~~there is an outstanding FIXME that I am working on right now.~~)

Also, some "new" tests are still failing in the compare-native-programs step. I am investigating.

/cc @shindere @dra27

nojb · 2017-09-19T16:55:14Z

This PR seems to fix all tests under Windows, except for the compare-native-programs tests of ocamltest. After some detective work with the help of @alainfrisch it seems that the reason is that the PE/COFF executables embed timestamps (in seconds). This makes the test fail sometimes.

One possibility would be to try to skip the portion of the file that contains the timestamp when comparing them, but it may not be so easy because the timestamp appears in more than one place in the executables.

gasche · 2017-09-19T17:25:16Z

ocamltest already hardcodes the fact that the 512 first bytes of the native programs are ignored under windows: https://github.com/ocaml/ocaml/blob/635b5ed/ocamltest/builtin_actions.ml#L707-L710 . A very conservative approach would be to simply disable this test/action under the same OS conditions.

nojb · 2017-09-20T11:12:52Z

Another possibility is to count the number of different bytes (for example, with cmp -l FILE1 FILE2 | wc -l) and succeed as long as this number is not too large.

gasche · 2017-09-20T16:44:16Z

(Apparently there exists a reasonably portable tool, Ducible, that knows how to strip non-reproducible information from PE executables. I'm pointing it out, but not suggesting to use it from ocamltest, because it adds a lot to the dependency chain (it's 10K lines of C++, and depends on Python 3 for configuration/building).

nojb · 2017-09-21T07:04:52Z

Is there a way to trigger AppVeyor on this pull request ?

gasche · 2017-09-21T07:25:22Z

I will try to close and reopen.

A few remarks by having just a very quick look at the patch:

some changes are not ocamltest-related at all (you say it's a Unicode support for the Windows runtime: Let's do it! #1200 merge issue) and I find it a bit confusing to have both kinds of changes in the same PR. Could you send the non-ocamltest-related changes in a separate PR? (In particular, those should not need Sébastien's attention/review).
Apologies for the very naive comment (I know nothing of your Windows-Unicode work), but I find it a bit strange that ocamltest/run_stubs.c needs to be changed to use ..._utf16 function, as I assume that this file (unlike run_win32.c) is not Windows-specific. Does that mean that arbitrary OCaml C code will have to use ..._utf16 functions for portability now? Is there a guide on porting applications somewhere? Or is that ocamltest-specific and why?
the implementation of disabling native tests on Windows is morally wrong, as you just overloaded the conditional that shows "tests are skipped under flambda" (so after your patch this line will show under non-flambda Windows builds); you should use a new conditional branch with its own meaningful message.

gasche · 2017-09-21T07:27:56Z

(Apparently close-reopen didn't work, I'm not sure why AppVeyor does not work here. Did we recently break the appveyor metadata files? (But they were last touched by #1200 and AppVeyor kept working after we merged that). Did AppVeyor automatically shut off after too many failed build? cc: @avsm)

nojb · 2017-09-21T07:33:22Z

Thanks @gasche for the comments.

OK, will open a separate PR for the Unicode support for the Windows runtime: Let's do it! #1200 fix.
Indeed, C code that is shared between Unix and Windows needs to use the _utf16 functions. In the Unix case these are just defined to be the usual functions, see here. Do you think it is worth introducing a new name for this three functions to be used in C code shared between Unix and Windows ?
Indeed, I forgot to fix the message.

gasche · 2017-09-21T07:37:31Z

Personally I find it fairly confusing to have to use _utf16 functions when my own system doesn't use utf16 anywhere. I would rather expect caml_stat_strdup to do the right thing on systems I don't know about, rather than me learning about the function names they need. But maybe that is too naive and cannot work. Has this been discussed before? Otherwise I would recommend starting a discussion with the development team on an appropriate place -- maybe in the comments for #1200? Or in a Discourse thread?

gasche · 2017-09-21T07:38:14Z

(I think it's better to have two separate tests with two precise error messages, one for flambda and one for windows, rather than a message that enumerates various possible skipping causes.)

nojb · 2017-09-21T07:49:53Z

OK, amended the ocamltest message as suggested and removed unrelated commits, except for one tiny fix to tests/letrec test to avoid a warning when running the testsuite (let me know if I should remove that one as well).

shindere · 2017-09-21T07:52:48Z

Hi, sorry for the late response, will address all issues ASAP. @nojb I don't know what makes AppVeyor restart its builds. In my experience it does so each time a branch on which a PR has been opened is updated. If you don't already have an AppVeyor account you may try to create one. Or perhaps this behaviour is because I'm part of the developers for the ocaml project, I have no idea.

alainfrisch · 2017-09-21T07:53:23Z

Personally I find it fairly confusing to have to use _utf16 functions when my own system doesn't use _utf16 anywhere.

I tend to agree. Perhaps just changing the suffix from _utf16 to _sys (or something else) would make it clearer that this is about converting between OCaml strings and the ones expected/returned by the OS (the identity and same types on both sides under *nix, the proper conversion and char* vs wchar* under Windows).

nojb · 2017-09-21T07:54:21Z

OK, I will open a new PR with this change and we can continue the discussion there.

shindere · 2017-09-21T13:44:32Z

Hi, NIcolas, Many thanks for all your work on Unicode support for Windows in general and ocamltest in particular. Am I correct that the intention is that #1363 gets merged first and then the present PR gets rebased on it so that run_stubs does not need to mention utf16 any longer? Also, with your PR I noticed that run_main.c leaked in although i didn't intend to publish it. There is also a comment I'd like to get rido of in the function that handles string arrays. Shall I do a PR for these two things and let you rebase yours on mine? Or shall I push to your branch directly? Or shall this PR get merged first and then I do a PR to update the other things?

shindere · 2017-09-21T13:49:29Z

Nicolás Ojeda Bär (2017/09/19 16:55 +0000):

This PR seems to fix all tests under Windows, except for the `compare-native-programs` tests of `ocamltest`. After some detective work with the help of @alainfrisch it seems that the reason is that the PE/COFF executables embed timestamps (in seconds). This makes the test fail **sometimes**.

Thanks! Actually, I was aware of the timestamp issue, it is something we discussed with @xavierleroy. So I started by going through the COFF documentation. My hope was to be able to figure out exactly where this timestamp is to be able to skip the precise number of bytes. Since I failed to do that in a proper way, I hard-coded a skip of 512 bytes which seemed to be enough when I tested last year but now seems not appropriate any longer.

One possibility would be to try to skip the portion of the file that contains the timestamp when comparing them, but it may not be so easy because the timestamp appears in more than one place in the executables.

Ah I didn't know it appears at several places. But I did attempt to do what you suggest, as noticed by @gasche.

shindere · 2017-09-21T13:54:08Z

FWIW, in the days just before #681 was merged, I had to disable the comparison of native binaries in the case where flambda is enabled. Indeed, on one test file (basic-more/morematch.ml IIRC) the binaries produced by ocamlopt.byte and ocamlopt.opt (under Linux) with flambda enabled were different. I don't know if this can be fixed, but at the very least this is something I'd like to understand better. Any idea @chambart? The file mentionned above did also produce different binaries when compiled with ocamlopt.byte and ocamlopt.opt when the level of inlining was >=5.

nojb · 2017-09-21T13:56:47Z

Hi Sébastien,

Either way is fine for me. I can easily rebase the PRs if needed. I propose thus

you make a PR with your two little fixes
I rebase this PR on that
we try to merge this PR without waiting for Fix naming of shared Unicode stubs #1363, since this PR is necessary to fix the Windows tests and Fix naming of shared Unicode stubs #1363 is "cosmetic".

nojb · 2017-09-21T14:04:28Z

@shindere Out of curiosity did you try investigating the binary diff of morematch under both compilers ?

shindere · 2017-09-21T14:07:06Z

Nicolás Ojeda Bär (2017/09/20 04:12 -0700):

Another possibility is to count the number of different bytes (for example, with `cmp -l FILE1 FILE2 | wc -l`) and succeed as long as this number is not too large.

That's an interesting idea, thanks. Well to be honest I really like the idea of comparing the native binaries and I would love to be able to do that in a proper way. Partly for aesthetic reasons, but also because I think the more properly it is done, the more robust and sustainable it will be. Skipping the beginning of the files or making sure the number of differences is not too big sounds less satisfactory, although I fully realise it may be the best one can reasonably expect. Disabling the comparison for Windows native binaries completely is also an option, of course. I don't know what's our best choice here. Any opinion welcome.

shindere · 2017-09-21T14:10:00Z

Many thanks for your careful review, @gasche !

nojb · 2017-09-21T14:14:35Z

Well, another possibility is to teach ocamltest a teeny bit of COFF so that it can step over the timestamps. It sounds like a fun exercise.

shindere · 2017-09-21T14:29:29Z

Nicolás Ojeda Bär (2017/09/21 07:04 -0700):

@shindere Out of curiosity did you try investigating the binary diff of `morematch` under both compilers ?

No, due to a lack of time. But I hope I'll be able to investigate now that ocamltest has finally been merged.

shindere · 2017-09-21T14:32:58Z

Nicolás Ojeda Bär (2017/09/21 14:14 +0000):

Well, another possibility is to teach `ocamltest` a teeny bit of COFF so that it can step over the timestamps. It sounds like a fun exercise.

Yeah I started to do it. Look for the comparison_start_address function in ocamltest/builtin_actions.ml. It's just that it didn't work, most likely because I misread the spec.

shindere · 2017-09-21T14:43:16Z

Nicolás Ojeda Bär (2017/09/21 06:57 -0700):

Hi Sébastien, Either way is fine for me. I can easily rebase the PRs if needed. I propose thus 1) you make a PR with your two little fixes 2) I rebase this PR on that 3) we try to merge this PR without waiting for #1363, since this PR is necessary to fix the Windows tests and #1363 is "cosmetic".

Thanks! See #1364.

shindere · 2017-09-25T08:34:57Z

It seems that, at least for some tests, it is CreateProcess which fails. I can see only one letter for the error message, "T". Can't investigate more right now but perhaps this will give someody a hint?

nojb · 2017-09-25T08:38:49Z

Only one letter being printed - seems like a WCHAR * is being passed to a function expecting a char *. Will look into it.

shindere · 2017-09-25T08:44:48Z

It's also ocamltest which does a too poor job at checking exactly what happens when CreateProcess fails. Will improve this ASAP, hopefully tonight, and hopefully I can give more useful feedback.

shindere · 2017-09-25T08:48:44Z

@nojb well actually the support for error messages has been implemented in ocamltest but perhaps it is not done well enough regarding unicdoe support. Could you please have a look to the report_error function in run_win32.c and see whether it looks good to you? Thanks!

nojb · 2017-09-25T09:20:07Z

I cannot reproduce the native test failures in my machine. In any case, I added a commit that may fix the error message. The bytecode failures should be fixed by #1362.

If someone could post the _ocamltest logs (or, even better, a .zip with the whole directory) produced by doing make new in testsuite/ in Inria's CI that may help debugging.

nojb · 2017-09-25T09:24:10Z

One guess about the native failures in Inria CI: an issue with flexdll.

Are you using the binary 0.36 release ? or the bootstrapped version ? I remember running into some issues when I tried to use the bootstrapped version.

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

@shindere I think report_error is essentially correct except for the use of %S to print wide strings which is pretty broken on Windows (we are looking into this with @dra27). Instead, in the last commit I explicitly convert the wide string into UTF-8 which should make it work.

xavierleroy

One serious error spotted. The rest looks reasonable to me but could use a second reading.

xavierleroy · 2017-09-25T11:40:10Z

ocamltest/run_stubs.c

  settings.logger = logToChannel;
  settings.loggerData = Channel(Field(caml_settings, 7));
  res = run_command(&settings);
+  caml_stat_free((charnat *)settings.program);


Why this cast (and several other below) to charnat * ? caml_stat_free takes void * arguments.

Yes, this is confusing: the right fix is not to define the struct fields (see https://github.com/ocaml/ocaml/blob/trunk/ocamltest/run.h#L32-L34) as being const char * in the original code. Will fix.

xavierleroy · 2017-09-25T11:40:45Z

ocamltest/run_win32.c

    error_with_location(file, line,
-      settings, "%s: %s", message, error_message);
+      settings, "%s: %s", message, error_message_c);
+  caml_stat_free(error_message);


AAAARGH! You're freeing the wrong error_message!!

Thanks, how embarrassing! (there should be a warning somewhere about writing C with less than 5 hours of sleep...)

xavierleroy · 2017-09-25T11:44:26Z

One guess about the native failures in Inria CI: an issue with flexdll.

That was the case until recently, but now the whole system builds and runs the test suite, it's just the ocamltest-administered tests that report errors.

I'm confident that the changes from the present PR will address this issue;. Let's just have them reviewed and finished as quickly as possible so that we can merge and try again.

nojb · 2017-09-25T11:55:27Z

Fixed and rebased.

damiendoligez

I've reviewed the diff and I think it's good.
@xavierleroy you'll need to change your review status...

shindere · 2017-09-26T07:26:17Z

Could not go very far with the tests but I think it would be great to
have this PR merged and perhaps also #1363. Indeed, the interaction between
ocamltest (#681) and Unicode support for Windows (#1200) could not be tested
very well and I think several of the failures that are actually encountered
on Inria's CI come from that. For instance, even the names of the files that
are created to store compiler outputs look weird.

So, my suggestion would be to merge these PRs if they are okay and I expect
the number of failures on Inria's CI to decrease. Then we can see what
errors are left and not due to this interaction.

gasche · 2017-09-26T07:31:22Z

Merged.

gasche · 2017-09-26T07:33:54Z

Note that #1363 conflicts with the present PR, if I understand correctly (the present one uses the pre-1363 names). I'll ask @nojb to rebase taking this code into account.

nojb · 2017-09-26T07:34:27Z

Thanks! Will rebase right away!

Fix ocamltest / Windows / Unicode issues

gasche · 2017-09-26T07:38:56Z

Cherry-picked in 4.06 [ 9aebb96 ]. (Cherry-pick squashed so history is lost, but (1) it was not that clean anyway and (2) this is just in the release branch, trunk has full history.)

* Add changelog for dune 3.9.0 * Minor improvements * Update data/changelog/dune/2023-06-30-dune-3.9.0.md Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> --------- Co-authored-by: Thibaut Mattio <thibaut.mattio@gmail.com> Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>

gasche closed this Sep 21, 2017

gasche reopened this Sep 21, 2017

nojb force-pushed the make_ocamltest_unicode_aware branch from 01c6956 to 56231aa Compare September 21, 2017 07:45

This was referenced Sep 21, 2017

Fix headernt.c & -output-obj Unicode support #1362

Merged

Fix naming of shared Unicode stubs #1363

Merged

nojb force-pushed the make_ocamltest_unicode_aware branch 2 times, most recently from 7890250 to c66fe99 Compare September 25, 2017 09:10

xavierleroy requested changes Sep 25, 2017

View reviewed changes

nojb added 5 commits September 25, 2017 13:54

Make ocamltest on Windows Unicode-aware

4e1aa8b

ocamltest: disable comparison of native programs under Windows

69b721c

Fix tests/letrec warning

e77cee8

Typo

a220588

Avoid '%S' when printing wide strings

a750eb4

nojb force-pushed the make_ocamltest_unicode_aware branch from b6dd129 to a750eb4 Compare September 25, 2017 11:54

damiendoligez approved these changes Sep 25, 2017

View reviewed changes

damiendoligez added this to the 4.06.0 milestone Sep 25, 2017

xavierleroy approved these changes Sep 25, 2017

View reviewed changes

gasche merged commit 7cc4e6d into ocaml:trunk Sep 26, 2017

gasche added a commit that referenced this pull request Sep 26, 2017

Merge pull request #1357 from nojb/make_ocamltest_unicode_aware

9aebb96

Fix ocamltest / Windows / Unicode issues

gasche mentioned this pull request Sep 29, 2017

ocamltest #681

Merged

Conversation

nojb commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Sep 19, 2017

Uh oh!

gasche commented Sep 19, 2017

Uh oh!

nojb commented Sep 20, 2017

Uh oh!

gasche commented Sep 20, 2017

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

gasche commented Sep 21, 2017

Uh oh!

gasche commented Sep 21, 2017

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

gasche commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Sep 21, 2017

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

alainfrisch commented Sep 21, 2017 • edited by damiendoligez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

nojb commented Sep 21, 2017

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 21, 2017 via email

Uh oh!

shindere commented Sep 25, 2017 via email

Uh oh!

nojb commented Sep 25, 2017

Uh oh!

shindere commented Sep 25, 2017 via email

Uh oh!

shindere commented Sep 25, 2017 via email

Uh oh!

nojb commented Sep 25, 2017

Uh oh!

nojb commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Sep 25, 2017

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

xavierleroy Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

nojb Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 19, 2017 •

edited

Loading

gasche commented Sep 21, 2017 •

edited

Loading

alainfrisch commented Sep 21, 2017 •

edited by damiendoligez

Loading

nojb commented Sep 25, 2017 •

edited

Loading

xavierleroy commented Sep 25, 2017 •

edited

Loading