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
Fix ocamltest / Windows / Unicode issues #1357
Conversation
This PR seems to fix all tests under Windows, except for the 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. |
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. |
Another possibility is to count the number of different bytes (for example, with |
(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). |
Is there a way to trigger AppVeyor on this pull request ? |
I will try to close and reopen. A few remarks by having just a very quick look at the patch:
|
Thanks @gasche for the comments.
|
Personally I find it fairly confusing to have to use |
(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.) |
01c6956
to
56231aa
Compare
OK, amended the |
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.
|
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). |
OK, I will open a new PR with this change and we can continue the discussion there. |
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?
|
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.
|
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.
|
Hi Sébastien, Either way is fine for me. I can easily rebase the PRs if needed. I propose thus
|
@shindere Out of curiosity did you try investigating the binary diff of |
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.
|
Many thanks for your careful review, @gasche !
|
Well, another possibility is to teach |
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.
|
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.
|
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.
|
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?
|
Only one letter being printed - seems like a |
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.
|
@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!
|
7890250
to
c66fe99
Compare
I cannot reproduce the If someone could post the |
One guess about the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One serious error spotted. The rest looks reasonable to me but could use a second reading.
ocamltest/run_stubs.c
Outdated
settings.append = Bool_val(Field(caml_settings, 5)); | ||
settings.timeout = Int_val(Field(caml_settings, 6)); | ||
settings.logger = logToChannel; | ||
settings.loggerData = Channel(Field(caml_settings, 7)); | ||
res = run_command(&settings); | ||
caml_stat_free((charnat *)settings.program); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this cast (and several other below) to charnat *
? caml_stat_free
takes void *
arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ocamltest/run_win32.c
Outdated
else | ||
error_with_location(file, line, | ||
settings, "%s: %s", message, error_message); | ||
settings, "%s: %s", message, error_message_c); | ||
caml_stat_free(error_message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AAAARGH! You're freeing the wrong error_message!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, how embarrassing! (there should be a warning somewhere about writing C with less than 5 hours of sleep...)
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. |
b6dd129
to
a750eb4
Compare
Fixed and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the diff and I think it's good.
@xavierleroy you'll need to change your review status...
Could not go very far with the tests but I think it would be great to So, my suggestion would be to merge these PRs if they are okay and I expect |
Merged. |
Thanks! Will rebase right away! |
Fix ocamltest / Windows / Unicode issues
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>
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.ocamltest
a Unicode Windows application.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