feature: use posix_spawn on macos

[rgrinberg]

Signed-off-by: Rudi Grinberg me@rgrinberg.com

[rgrinberg]

@anmonteiro @jchavarri can you check if you can reproduce the bug on this PR?

[anmonteiro]

I can't reproduce the hang on macOS anymore with this PR.

[anmonteiro]

I'm running it through the full set of packages too: nix-ocaml/nix-overlays#926

[anmonteiro]

This might not be that risky, the CI run succeeded: https://github.com/nix-ocaml/nix-overlays/actions/runs/5466251548/jobs/9950895919

[jchavarri]

Can't repro the original issue with this PR either.

[rgrinberg]

This PR has some advantages from a performance standpoint. So it might be good to report if you notice anything. I know @anmonteiro did some benchmarks on melange.

[jchavarri]

I can see some significant improvements yes. From 8% to 25% faster with this branch, it seems to affect particularly Melange builds (that's where I see the 25% improvement). Note that the mac CI nodes we use have 12 cores, I imagine that the more capacity there is to parallelize, the larger the impact on build times?

[anmonteiro]

@rgrinberg what's missing to get this one over the line?

[rgrinberg]

If you could read over and familiarize yourself with the C to give a formal review that would be a great start.

[anmonteiro]

It's probably worth it to copy the upstream concern about resetting close-on-exec, and think about whether we'd want to solve it here:

https://github.com/janestreet/spawn/pull/47/files#r930889893

[rgrinberg]

Yeah, we'll do it upstream. Note that I've talked to the maintainers and we agreed that this isn't an issue for dune because we don't allow anywhere else in the code to use fork.

[rgrinberg]

@anmonteiro could you give this another test please? I re-enabled threads on macos since they longer should be a problem and should give us an additional perf benefit.

[anmonteiro]

Running this through all the packages here: nix-ocaml/nix-overlays#1000

[anmonteiro]

LGTM.

All macOS packages build on nix-overlays. They were previously broken with fork

[jchavarri]

Just reporting some build times i measured after this PR was merged in a quite large Melange build on our codebase, using my macbook pro (16 cores).

Using an older version of dune (e1f3e19):

time dune build @fe.all
dune build @fe.all  2539.49s user 1807.54s system 581% cpu 12:27.02 total

Using latest dune (758e370):

time dune build @fe.all
dune build @fe.all  2518.63s user 1663.82s system 889% cpu 7:49.98 total

Thanks @rgrinberg !

[ixzzd]

Here are the results of compiling the same large Melange codebase on the M2 MacBook Air on the latest dune (df4399b)

$ time opam exec -- dune build @fe.all

________________________________________________________
Executed in  226.01 secs    fish           external
   usr time  705.37 secs    0.26 millis  705.37 secs
   sys time  412.18 secs    1.31 millis  412.18 secs

$ opam exec -- dune --version
3.10.0-259-gdf4399b

vs older version of dune (e1f3e19)

$ time opam exec -- dune build @fe.all

________________________________________________________
Executed in  296.71 secs    fish           external
   usr time  675.71 secs    0.27 millis  675.71 secs
   sys time  498.06 secs    1.32 millis  498.06 secs

296.71 secs vs 226.01 secs

@rgrinberg 👏 👏 👏

[barracuda156]

@rgrinberg This does not seem right. Let me try fixing this and see if it builds then.

[barracuda156]

@rgrinberg What this macro is supposed to do specifically?

[rgrinberg]

Yeah, this is wrong. The check should be for 10.5 because we need pthread_chdir

[barracuda156]

I guess this is what fails for me on 10.6 ppc, which will be similar to 10.5 in this regard.

[barracuda156]

There are two more issues:

1. environ define is wrong for macOS.
2. There seems to be incoherency in the code, at first vfork fork is defined generally, then below the same only for macOS > 12000.

[barracuda156]

Let me see if with these fixes it works. Will update in few min.

[rgrinberg]

Okay, thanks. Note that we can't use vfork on macos. So it must be a leftover from the old code when we did.

[barracuda156]

@rgrinberg After these 39703e0 I got here:

Error: execve(../../install/default/bin/dune): Exec format error
-> required by _build/default/doc/dune-build.1
-> required by _build/install/default/man/man1/dune-build.1
-> required by _build/default/dune.install
-> required by alias install
make: *** [release] Error 1

How to get a proper log to see what fails exactly?

[rgrinberg]

What kind of log are you looking for? There is no more information for this error.

Try running this executable maunally ../../install/default/bin/dune and see if it works.

[barracuda156]

What kind of log are you looking for? There is no more information for this error.

Well, just in case.

Try running this executable maunally ../../install/default/bin/dune and see if it works.

This is literally a symlink to an empty file. So for w/e reason it is not written correctly, and build system is unaware of that.

P. S. Could you please take a look at my changes and say what should be done re fork? I know pretty well how macOS-specific macros work and which should be used, but I have no idea whether that fork define should or should not be used, and if yes, then conditional on what.

[rgrinberg]

This is literally a symlink to an empty file. So for w/e reason it is not written correctly, and build system is unaware of that.

Okay, that's the source of the error then. What command were you using to build dune and then to generate this error?

Could you please take a look at my changes and say what should be done re fork? I know pretty well how macOS-specific macros work and which should be used, but I have no idea whether that fork define should or should not be used, and if yes, then conditional on what.

Sure, where are your changes?

Regarding posix_spawn vs fork vs vfork:

1. We should only use vfork on Linux
2. We should use posix_spawn if it's available and has all the options we need (MacOS in conjunction with pthread_chdir)
3. If none of the above are available, we fallback to fork.

So on macos, we should only use fork if our posix_spawn is good enough and pthread_chdir is available (I believe anything above 10.5, but I could be wrong).

[barracuda156]

@rgrinberg Changes are here: 39703e0
environ part should be correct.
All PowerPC should use same fallbacks as 10.5 (therefore additional condition for __ppc__, since it exists on 10.6, but does not work exactly like 10.6 Intel).

And fork part is wrong, I guess :) How to fix it?

Okay, that's the source of the error then. What command were you using to build dune and then to generate this error?

I build from the port: https://github.com/macports/macports-ports/blob/master/ocaml/ocaml-dune/Portfile
(Both 3.11.0 and 3.11.1 fail, initially on this #8941 and after I added a suggested fix from that thread I bumped into spawn failure. Then I added my patch.)

[barracuda156]

@rgrinberg Changing the code to:

#if defined(__APPLE__)
# include <AvailabilityMacros.h>

# if (MAC_OS_X_VERSION_MAX_ALLOWED >= 1060) && !defined(__ppc__)
#  define USE_POSIX_SPAWN
# endif

# define vfork fork

#include <fcntl.h>
#include <sys/socket.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>

I still get Error: execve(../../install/default/bin/dune): Exec format error.

P. S. This new error does not need to be spawn-related. The last version I can say built and worked for sure was 3.7.0 and master branch as of 2022.11.18. Everything in between that and 3.11.0 has not been tried, at least yet.

UPD. I am pretty sure now that Error: execve(../../install/default/bin/dune): Exec format error has nothing to do with spawn. Something got broken elsewhere. I do not see any relevant changes in Macports environment, so presumably it is either in dune or its dependencies. No idea so far.

[Alizter]

@barracuda156 Could you try 3.10 to make sure it is this PR that is causing problems.

[barracuda156]

@Alizter It looks like we have fixed spawn issue in main...barracuda156:dune:darwin

Error: execve(../../install/default/bin/dune): Exec format error is not related to spawn though. In fact, I get it with 3.8.0, 3.9.0 too. Something has broken elsewhere.

[rgrinberg]

Try disabling fast copying completely with DUNE__COPY_FILE=portable. Does that help?

[barracuda156]

Try disabling fast copying completely with DUNE__COPY_FILE=portable. Does that help?

Sorry for a silly question, where do I pass this? Configure does not seem to take this as an arg.

[rgrinberg]

It's an environment variable, so you just need it to set when running dune. E.g. DUNE__COPY_FILE=portable make dev.

[barracuda156]

Unfortunately, no effect:

--->  Building ocaml-dune
Executing:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune/work/dune-3.11.1" && DUNE__COPY_FILE=portable /usr/bin/make -j6 release 
ocamlc -output-complete-exe -w -24 -g -o .duneboot.exe -I boot unix.cma boot/libs.ml boot/duneboot.ml
./.duneboot.exe
Error: execve(../../install/default/bin/dune): Exec format error
-> required by _build/default/doc/dune-build.1
-> required by _build/install/default/man/man1/dune-build.1
-> required by _build/default/dune.install
-> required by alias install
make: *** [release] Error 1
Command failed:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune/work/dune-3.11.1" && DUNE__COPY_FILE=portable /usr/bin/make -j6 release 
Exit code: 2

But perhaps we patched out both copy_file and posix_spawn issues. This is something else, it seems.

[Alizter]

@barracuda156 I think that's a typo, it should be DUNE_CONFIG__COPY_FILE.

[barracuda156]

@Alizter @rgrinberg So yes, DUNE_CONFIG__COPY_FILE worked, dune 3.11.1 has built now!

What are the conclusions? Could we fix it properly rather than just disabling? dune is an important port, for OCaml ecosystem very much so, it is desirable to have it working optimally, as long as it is feasible. I can test any suggestions on my end.

UPD. I still need to verify if resulting dune binary is functional though. So far only build is confirmed, FWIW.

[barracuda156]

I gonna return to this in detail tomorrow, need to leave now, but preliminary results are problematic:

Whether built with my patch to spawn or without it, dune 3.11.1 cannot now compile ocaml-compiler-libs, which has no dependencies besides OCaml and Dune, and which I built successfully with dune 3.7.0 few days ago. Now I get this:

--->  Configuring ocaml-compiler-libs
--->  Building ocaml-compiler-libs
Executing:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-compiler-libs/ocaml-compiler-libs/work/ocaml-compiler-libs-0.12.4" && /opt/local/bin/dune build -j 6 --root=/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-compiler-libs/ocaml-compiler-libs/work/ocaml-compiler-libs-0.12.4 --profile=release --ignore-promoted-rules --no-config --only-packages=ocaml-compiler-libs --default-target=@install @install 
File "src/shadow/dune", line 10, characters 0-147:
10 | (rule
11 |  (targets ocaml_shadow.ml)
12 |  (deps
13 |   (:first_dep gen/gen.exe))
14 |  (action
15 |   (run %{first_dep} -dir %{ocaml_where}/compiler-libs -o %{targets})))
Error: Rule failed to generate the following targets:
- src/shadow/ocaml_shadow.ml
File "src/ocaml_common/dune", line 8, characters 0-185:
 8 | (rule
 9 |  (targets ocaml_common.ml)
10 |  (deps
11 |   (:first_dep ../gen/gen.exe))
12 |  (action
13 |   (run
14 |    %{first_dep}
15 |    -archive
16 |    %{ocaml_where}/compiler-libs/ocamlcommon.cma
17 |    -o
18 |    %{targets})))
Error: Rule failed to generate the following targets:
- src/ocaml_common/ocaml_common.ml
File "src/ocaml_bytecomp/dune", line 8, characters 0-189:
 8 | (rule
 9 |  (targets ocaml_bytecomp.ml)
10 |  (deps
11 |   (:first_dep ../gen/gen.exe))
12 |  (action
13 |   (run
14 |    %{first_dep}
15 |    -archive
16 |    %{ocaml_where}/compiler-libs/ocamlbytecomp.cma
17 |    -o
18 |    %{targets})))
Error: Rule failed to generate the following targets:
- src/ocaml_bytecomp/ocaml_bytecomp.ml
File "src/ocaml_toplevel/dune", line 8, characters 0-189:
 8 | (rule
 9 |  (targets ocaml_toplevel.ml)
10 |  (deps
11 |   (:first_dep ../gen/gen.exe))
12 |  (action
13 |   (run
14 |    %{first_dep}
15 |    -archive
16 |    %{ocaml_where}/compiler-libs/ocamltoplevel.cma
17 |    -o
18 |    %{targets})))
Error: Rule failed to generate the following targets:
- src/ocaml_toplevel/ocaml_toplevel.ml
Command failed:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-compiler-libs/ocaml-compiler-libs/work/ocaml-compiler-libs-0.12.4" && /opt/local/bin/dune build -j 6 --root=/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-compiler-libs/ocaml-compiler-libs/work/ocaml-compiler-libs-0.12.4 --profile=release --ignore-promoted-rules --no-config --only-packages=ocaml-compiler-libs --default-target=@install @install 
Exit code: 1

(This patch https://github.com/ocaml/dune/pull/8942/files is required, without it dune does not build.)

dune-configurator also fails to build now:

--->  Building ocaml-dune-configurator
Executing:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune-configurator/work/dune-3.11.1" && /opt/local/bin/dune build -j 6 --root=/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune-configurator/work/dune-3.11.1 --profile=release --ignore-promoted-rules --no-config --only-packages=dune-configurator --default-target=@install @install 
File "otherlibs/configurator/src/extract_obj.mll", line 1, character 0: syntax error.
File "_build/default/otherlibs/configurator/src/flags/flags.sexp", line 1, characters 0-0:
Error: no s-expression found in input
Command failed:  cd "/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune-configurator/work/dune-3.11.1" && /opt/local/bin/dune build -j 6 --root=/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_ocaml_ocaml-dune/ocaml-dune-configurator/work/dune-3.11.1 --profile=release --ignore-promoted-rules --no-config --only-packages=dune-configurator --default-target=@install @install 
Exit code: 1

Apparently while dune itself builds with DUNE_CONFIG__COPY_FILE=portable, it does not work as supposed at the moment.

[Alizter]

I think that disabling DUNE_CONFIG__COPY_FILE is not particularly a difficult thing to do. It should be straightforward enough to detect MacOS on powerpc by detecting it in stdune/src/platform_stubs.c, adding another platform to Stdune.Platform.OS (perhaps by refining the Darwin variant and then choosing copy_file in dune_config.ml appropriately depending on ppc or not for darwin.

I should say however that this isn't something we will probably do any time soon since we currently have other priorities in Dune and not that many developers. However if you feel like creating a patch yourself, we can try to review it when we have the time. Since OCaml isn't looking to officially support powerpc we have even less of a reason to. c.f. ocaml/ocaml#11216

You can also subsume my patch for the copyfile C stubs and add an appropriate comment detailing why its needed.

• fix: define COPYFILE_CLONE if not available. #8942

[barracuda156]

@Alizter OCaml does support all 32-bit platforms including powerpc in bytecode compiler (this applies to OCaml 5). That was the stance of upstream at least rather recently. Native was broken anyway, so nothing changes.

The thread you refer to was about native compiler, which was broken and remained so, and then in OCaml 5 support for i386 and arm was removed too. So no point trying fix it.
However, bytecode works on 32-bit (including ppc), and is supported: ocaml/ocaml@eb04c8b#commitcomment-116722903