fix setting Lwt_process env on Windows

[mroch]

env is an "environment block": a null-terminated block of null-terminated strings. using caml_stat_strdup_to_os on it doesn't work, it only copies the first string.

instead, we need to use an explicit length (caml_string_length(env)), but there's no public *_to_os function for this so we have to use the internal caml_win32_multi_byte_to_wide_char directly.

Fixes #966

[raphael-proust]

Thanks a lot for the MR. I don't have a windows machine to test this on so I can't reproduce locally. Nevertheless, I'll be reviewing this MR soon.

[mroch]

i dunno if you can get CI to run on the first diff (1d199cd) but it'll show the bug. for example: https://github.com/mroch/lwt/actions/runs/3284275234/jobs/5410034604

I believe the EINVAL comes from passing just a single null terminated string, without the additional \0 to terminate the block (and it's only passing the first of many envs, but I think it's the malformed block that causes the EINVAL specifically)

[raphael-proust]

i dunno if you can get CI to run on the first diff

I don't know either. I don't understand github's UI very well. ❓ ❗

But that's ok. When I do the review I might push a branch with just the first commit to force the CI.

[MisterDA]

Thanks for finding this and fixing the bug. The code is semantically identical to OCaml's Unix library, so I think this is good to go.
https://github.com/ocaml/ocaml/blob/c6d207656de19d97b5edf5c2f9028412aa218571/otherlibs/unix/createprocess.c#L120-L131

[raphael-proust]

At first I was pleased that the code was so similar.
Then I thought it'd be even better if the code was even more similar.
And then I started thinking about licenses and now I don't know what to do about this PR.

I'll ask if it's ok. Basically, the added code is calling the functions provided by the ocaml project in the appropriate sequence. I think the alternative to using the same code is making a pull-request on the ocaml project to provide this sequence of calls as a function itself to be called. That might not be desirable upstream anyway…

Anyway, I'll ask.

[raphael-proust]

A similar situation happened on the eio bugtracker:
ocaml-multicore/eio#352 (comment)

It's not exactly the same (not the same code being copied, not the same size of code) but it's also copying parts of the C code.

[raphael-proust]

I've made raphael-proust/ocaml@bdbddd2 which aims to expose the code that we actually need from the OCaml source. Please leave comments on there, I don't know that I did anything correctly.

[raphael-proust]

As per ocaml/ocaml#12492 (comment) it turns out licensing is not an issue. We can go ahead on this PR.

[MisterDA]

This is long overdue ;)

[raphael-proust]

Do we need to #include an additional file for windows on 5.2? The CI has the following error for that specific combination:

#=== ERROR while compiling lwt.dev ============================================#
# context     2.2.0 | win32/x86_64 | ocaml-base-compiler.5.2.0 | pinned(git+file://D:/a/lwt/lwt#HEAD#9d9efab4e28699c2f9ffd8397f19141dac199ead)
# path        D:\a\lwt\lwt\_opam\.opam-switch\build\lwt.dev
# command     D:\a\lwt\lwt\_opam\bin\dune.exe build -p lwt -j 3 @install
# exit-code   1
# env-file    D:\.opam\log\lwt-5000-9b088c.env
# output-file D:\.opam\log\lwt-5000-9b088c.out
### output ###
# File "_build/.dune/default/src/unix/dune", line 49, characters 2-19:
# 49 |   lwt_process_stubs
#        ^^^^^^^^^^^^^^^^^
# (cd _build/default/src/unix && D:\a\lwt\lwt\_opam\bin\x86_64-w64-mingw32-gcc.exe -O2 -fno-strict-aliasing -fwrapv -mms-bitfields -I. -DUNICODE -D_UNICODE -g -I D:/a/lwt/lwt/_opam/lib/ocaml -I D:/a/lwt/lwt/_opam/lib/ocaml\threads -I D:/a/lwt/lwt/_opam/lib/ocaml\unix -I D:\a\lwt\lwt\_opam\lib\bytes -I D:\a\lwt\lwt\_opam\lib\ocplib-endian -I D:\a\lwt\lwt\_opam\lib\ocplib-endian\bigstring -I ../core -o lwt_process_stubs.o -c lwt_process_stubs.c)
# In file included from D:/a/lwt/lwt/_opam/lib/ocaml/caml/memory.h:27,
#                  from lwt_process_stubs.c:21:
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h: In function 'caml_check_gc_interrupt':
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h:43:3: error: 'CAMLalloc_point_here' undeclared (first use in this function)
#    43 |   CAMLalloc_point_here;
#       |   ^~~~~~~~~~~~~~~~~~~~
# D:/a/lwt/lwt/_opam/lib/ocaml/caml/domain.h:43:3: note: each undeclared identifier is reported only once for each function it appears in

[dra27]

Hmm, apparently I accidentally allowed ocaml/ocaml#11449 to be closed by the stale bot. The issue is that CAML_INTERNALS is causing more things to happen in the headers than are wanted. I wonder if this might be the best plan until an "official" API function is added: as before, only define CAML_INTERNALS for OCaml < 4.13, but instead duplicate the extern declaration from osdeps.h for caml_win32_multi_byte_to_wide_char?

It might be possible to work around it by explicitly including caml/misc.h, but I think it's better to have it that CAML_INTERNALS is only being defined for old versions of OCaml (i.e. that it's not still required with current versions)?

[raphael-proust]

I made a few attempts but it's a pain to work on without access to a windows machine.

I'll make a release which won't be available for 5.2 on windows (sorry) and I can make a point release later to fix this.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)

• src/unix/lwt_process_stubs.c: Language not supported
• test/unix/dummy.ml: Language not supported
• test/unix/test_lwt_process.ml: Language not supported