-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 headernt.c & -output-obj Unicode support #1362
Conversation
b661514
to
2d242fa
Compare
Rebased on |
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.
LGTM. It's up to you if you want to wait for a review from @dra27, who is away until Monday.
Thanks! No problem waiting for David's review (only downside is that AppVeyor will remain broken until then). |
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. |
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.
|
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. |
Sorry - fully back from vacation tomorrow (though I may have a chance to give this and #1363 a review later today) |
@@ -487,7 +487,7 @@ let link_bytecode_as_c ppf tolink outfile = | |||
Symtable.output_primitive_table outchan; | |||
(* The entry point *) | |||
output_string outchan "\ | |||
\nvoid caml_startup(char ** argv)\ | |||
\nvoid caml_startup(charnat ** argv)\ |
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.
Should probably be renamed to char_os once the other PR is merged, right?
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.
That's right, will update as necessary.
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.
In the interests of getting at least one of our CI systems back up and running, can we merge this before #1363?
stdlib/headernt.c
Outdated
static char str[MAX_PATH]; | ||
|
||
if (GetConsoleMode(hOut, &consoleMode) != 0) { /* The output stream is a Console */ | ||
WriteConsoleW(hOut, wstr, wcslen(wstr), &numwritten, NULL); |
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.
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.
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 think you are right. Will fix.
stdlib/headernt.c
Outdated
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); |
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.
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?
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.
Sorry, I missed the fact that CP is indeed UTF-8 when OCaml is configured with Unicode support.
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.
Absolutely I think it makes good sense! I just did it this way to preserve existing behavior.
2d242fa
to
43553f9
Compare
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.
LGTM - cosmetic tweak to the exec_tests disabling only
testsuite/tests/win-unicode/Makefile
Outdated
@@ -9,7 +9,7 @@ C_FILES=mkfiles | |||
.PHONY: test | |||
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 && \ |
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.
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?
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.
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...)
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.
Fixed as suggested, thanks!
@@ -487,7 +487,7 @@ let link_bytecode_as_c ppf tolink outfile = | |||
Symtable.output_primitive_table outchan; | |||
(* The entry point *) | |||
output_string outchan "\ | |||
\nvoid caml_startup(char ** argv)\ | |||
\nvoid caml_startup(charnat ** argv)\ |
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.
In the interests of getting at least one of our CI systems back up and running, can we merge this before #1363?
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? |
I've just popped this branch into Inria CI's precheck |
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. |
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.
|
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.
|
@shindere wrote:
64-bit builds fail with dozens of errors like this one:
|
The errors all look similar to this one:
This is unrelated to the present PR, but it's a recent breakage. I'll send an email. |
Thanks, @nojb. Trying to reproduce this here.
|
Thanks! |
Cherry-picked to 4.06 in 36e0a3b |
This is a follow-up to #1200. It does three things:
read_runtime_path
ofheadernt.c
argv
argument in the C file generated by-output-obj
WriteConsole
inheadernt.c
to properly print Unicode pathnames in error messages.Together with #1357 this should fix the remaining Windows testsuite errors.
/cc @dra27