fix(windows): use unicode version of CreateProcess #10212

emillon · 2024-03-05T12:26:37Z

This updates our spawn vendored version to include janestreet/spawn#58.

Fixes #10180

emillon · 2024-03-05T12:29:03Z

@kit-ty-kate can you test this? thanks

kit-ty-kate · 2024-03-05T12:32:44Z

Thanks for the ping, i'll do that in a minute

emillon · 2024-03-05T12:34:31Z

still some issues with the vendoring, I'll ping you when it works, sorry

boot/libs.ml

emillon · 2024-03-05T14:22:20Z

~~(TODO: update this PR with upstream dune file which restricts flags to win32)~~

emillon · 2024-03-05T15:21:31Z

@kit-ty-kate this is now ready if you want to test it.

kit-ty-kate · 2024-03-05T15:58:24Z

I can confirm this fixes the issue. I tried twice with two different unicode characters which was reliably causing problems before and this PR fixes the issue.

Thanks a lot!

emillon · 2024-03-05T16:10:52Z

I've pushed the bootstrap changes to #10217 to make this a bit simpler.

emillon · 2024-03-06T08:51:20Z

This seems to work, but TBH I'm not completely sold on passing -DUNICODE as cflags compared to using #define in the stubs. I get that it's a more principled way of doing it, but this brings a bit of complexity in the build instructions and in the bootstrap mechanism.

nojb

LGTM (modulo the questions below)

vendor/spawn/src/dune

nojb · 2024-03-08T08:31:31Z

boot/libs.ml

+let build_flags =
+  [ ("win32", [ "-ccopt"; "-D_UNICODE"; "-ccopt"; "-DUNICODE" ])
+  ; ("win64", [ "-ccopt"; "-D_UNICODE"; "-ccopt"; "-DUNICODE" ])
+  ; ("mingw", [ "-ccopt"; "-D_UNICODE"; "-ccopt"; "-DUNICODE" ])
+  ; ("mingw64", [ "-ccopt"; "-D_UNICODE"; "-ccopt"; "-DUNICODE" ])
+  ]


I don't see where this is used?

This is used in the bootstrap process. It doesn't read the dune files, so the build flags are read from boot/libs.ml.

emillon · 2024-03-11T09:03:15Z

@dra27 @nojb I have some questions regarding how consistent we need to be with these C flags. Concretely, we have some C stubs in fswatch_win_stubs that pull windows.h. They're built into a library that's linked into dune, next to spawn. Is it an issue if one library has -DUNICODE and the other one doesn't?

nojb · 2024-03-11T09:07:36Z

how consistent we need to be with these C flags.

You can have Unicode-compatible and ANSI-compatible code in the same executable, so there is no requirement perse from that perspective. On the other hand, we very much want to be sure that all our Windows C code is Unicode-compatible. The stubs of the "fswatch" library use "W" (= Unicode-compatible) functions explicitly, which is why we don't need to define UNICODE and _UNICODE when building it, but it would be harmless to do it.

emillon · 2024-03-11T10:08:37Z

Thanks for the confirmation @nojb. Given the previous tests and reviews, I'm merging this.

This does several things: - update our spawn vendored version to include janestreet/spawn#58 - pick the dune file - add new build flags for bootstrapping Fixes ocaml#10180 Signed-off-by: Etienne Millon <[email protected]>

dra27 · 2024-03-11T11:02:56Z

Yes, indeed - pretty much everything should use Unicode versions now (most especially when dealing with the file system). The exception is if you have non-ASCII data (say in a database) which was interpreted using the various ANSI legacy encodings.

Defining _UNICODE and UNICODE is good because it catches accidental use of ANSI functions and can also influence header behaviour (I think that part affects the CRT much more than the Windows headers, given that we don't use TCHAR)

This does several things: - update our spawn vendored version to include janestreet/spawn#58 - pick the dune file - add new build flags for bootstrapping Fixes #10180 Signed-off-by: Etienne Millon <[email protected]> Co-authored-by: Etienne Millon <[email protected]>

@emillon

CHANGES: ### Fixed - When a directory is changed to a file, correctly remove it in subsequent `dune build` runs. (ocaml/dune#9327, fix ocaml/dune#6575, @emillon) - Fix a problem with the doc-new target where transitive dependencies were missed during compile. This leads to missing expansions in the output docs. (ocaml/dune#9955, @jonludlam) - coq: fix performance regression in coqdep unescaping (ocaml/dune#10115, fixes ocaml/dune#10088, @ejgallego, thanks to Dan Christensen for the report) - coq: memoize coqdep parsing, this will reduce build times for Coq users, in particular for those with many .v files (ocaml/dune#10116, @ejgallego, see also ocaml/dune#10088) - on Windows, use an unicode-aware version of `CreateProcess` to avoid crashes when paths contains non-ascii characters. (ocaml/dune#10212, fixes ocaml/dune#10180, @emillon)

anmonteiro · 2024-03-11T22:03:41Z

This change seems to break compiling dune on 4.14+musl+static. Here's the error message:

dune-x86_64-unknown-linux-musl> In file included from vendor/spawn/src/spawn_stubs.c:19:
dune-x86_64-unknown-linux-musl> /nix/store/bxid93hl5p406haravq2bc189y93c6gp-ocaml+flambda-x86_64-unknown-linux-musl-4.14.1/lib/ocaml/caml/signals.h:97:48: error: unknown type name 'sigset_t'
dune-x86_64-unknown-linux-musl>    97 | CAMLextern int (*caml_sigmask_hook)(int, const sigset_t *, sigset_t *);
dune-x86_64-unknown-linux-musl>       |                                                ^~~~~~~~
dune-x86_64-unknown-linux-musl> /nix/store/bxid93hl5p406haravq2bc189y93c6gp-ocaml+flambda-x86_64-unknown-linux-musl-4.14.1/lib/ocaml/caml/signals.h:97:60: error: unknown type name 'sigset_t'; did you mean 'size_t'?
dune-x86_64-unknown-linux-musl>    97 | CAMLextern int (*caml_sigmask_hook)(int, const sigset_t *, sigset_t *);
dune-x86_64-unknown-linux-musl>       |                                                            ^~~~~~~~
dune-x86_64-unknown-linux-musl>       |                                                            size_t
dune-x86_64-unknown-linux-musl> make: *** [Makefile:52: _boot/dune.exe] Error 2

and the link to a failing job

anmonteiro · 2024-03-11T22:05:46Z

Ah, it looks like that has been discovered in opam-ci too: ocaml/opam-repository#25456

emillon force-pushed the update-spawn-windows branch from c7179fd to 3c6ee60 Compare March 5, 2024 12:28

emillon mentioned this pull request Mar 5, 2024

3.14.1 release tracking #10203

Closed

15 tasks

emillon force-pushed the update-spawn-windows branch from 560afa6 to 93b1be5 Compare March 5, 2024 13:08

emillon commented Mar 5, 2024

View reviewed changes

boot/libs.ml Outdated Show resolved Hide resolved

emillon force-pushed the update-spawn-windows branch from 05f5040 to 655029f Compare March 5, 2024 14:13

emillon force-pushed the update-spawn-windows branch from 655029f to 7ea41f6 Compare March 5, 2024 15:03

kit-ty-kate approved these changes Mar 5, 2024

View reviewed changes

emillon mentioned this pull request Mar 6, 2024

feat(boot): specify platform-specific cflags #10217

Closed

kit-ty-kate mentioned this pull request Mar 6, 2024

Convert Windows stubs to Unicode janestreet/spawn#58

Merged

rgrinberg requested a review from nojb March 8, 2024 08:20

rgrinberg added the windows label Mar 8, 2024

nojb approved these changes Mar 8, 2024

View reviewed changes

emillon force-pushed the update-spawn-windows branch 2 times, most recently from 9faef00 to 7fb6e28 Compare March 11, 2024 09:23

fix(windows): use unicode version of CreateProcess

09c6e8e

This does several things: - update our spawn vendored version to include janestreet/spawn#58 - pick the dune file - add new build flags for bootstrapping Fixes ocaml#10180 Signed-off-by: Etienne Millon <[email protected]>

emillon force-pushed the update-spawn-windows branch from 7fb6e28 to 09c6e8e Compare March 11, 2024 10:09

emillon merged commit 6c40cb1 into ocaml:main Mar 11, 2024
25 of 27 checks passed

emillon deleted the update-spawn-windows branch March 11, 2024 10:13

Leonidas-from-XIV mentioned this pull request Mar 11, 2024

[3.14] Backport #10212 #10246

Merged

Leonidas-from-XIV mentioned this pull request Mar 11, 2024

[new release] dune (15 packages) (3.14.1) ocaml/opam-repository#25456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(windows): use unicode version of CreateProcess #10212

fix(windows): use unicode version of CreateProcess #10212

emillon commented Mar 5, 2024

emillon commented Mar 5, 2024

kit-ty-kate commented Mar 5, 2024

emillon commented Mar 5, 2024

emillon commented Mar 5, 2024 •

edited

Loading

emillon commented Mar 5, 2024

kit-ty-kate commented Mar 5, 2024

emillon commented Mar 5, 2024

emillon commented Mar 6, 2024

nojb left a comment

nojb Mar 8, 2024

emillon Mar 11, 2024

emillon commented Mar 11, 2024

nojb commented Mar 11, 2024

emillon commented Mar 11, 2024

dra27 commented Mar 11, 2024

anmonteiro commented Mar 11, 2024 •

edited

Loading

anmonteiro commented Mar 11, 2024

fix(windows): use unicode version of CreateProcess #10212

fix(windows): use unicode version of CreateProcess #10212

Conversation

emillon commented Mar 5, 2024

emillon commented Mar 5, 2024

kit-ty-kate commented Mar 5, 2024

emillon commented Mar 5, 2024

emillon commented Mar 5, 2024 • edited Loading

emillon commented Mar 5, 2024

kit-ty-kate commented Mar 5, 2024

emillon commented Mar 5, 2024

emillon commented Mar 6, 2024

nojb left a comment

Choose a reason for hiding this comment

nojb Mar 8, 2024

Choose a reason for hiding this comment

emillon Mar 11, 2024

Choose a reason for hiding this comment

emillon commented Mar 11, 2024

nojb commented Mar 11, 2024

emillon commented Mar 11, 2024

dra27 commented Mar 11, 2024

anmonteiro commented Mar 11, 2024 • edited Loading

anmonteiro commented Mar 11, 2024

emillon commented Mar 5, 2024 •

edited

Loading

anmonteiro commented Mar 11, 2024 •

edited

Loading