janestreet / base

Standard library for OCaml
MIT License
848 stars 124 forks source link

Fix Windows compilation failure `ppx_base` because of `base` and `ocaml_intrinsics_kernel` symbol clash #165

Closed MisterDA closed 2 months ago

MisterDA commented 3 months ago

Put CAMLweakdef function in its own translation unit for Windows compatibility, or it conflicts with other non-weakdef symbols with the same name from other libraries. There's a conflicting symbol in ocaml_intrinsics_kernel at: https://github.com/janestreet/ocaml_intrinsics_kernel/blob/1047d3186d0c6b498f1baa96024901cc3cbc2a19/src/conditional_stubs.c#L23-L26.

$ dune build
File "bin/dune", line 3, characters 8-12:
3 |  (names main)
            ^^^^
/usr/lib/gcc/x86_64-w64-mingw32/11/../../../../x86_64-w64-mingw32/bin/ld: C:\Users\Antonin\AppData\Local\opam\default\lib\ocaml_intrinsics_kernel\libocaml_intrinsics_kernel_stubs.a(conditional_stubs.o):/cygdrive/c/Users/Antonin/AppData/Local/opam/default/.opam-switch/build/ocaml_intrinsics_kernel.v0.17.0/_build/default/src/conditional_stubs.c:16: multiple definition of `caml_csel_value'; C:\Users\Antonin\AppData\Local\opam\default\lib\base\libbase_stubs.a(int_math_stubs.o):/cygdrive/c/Users/Antonin/AppData/Local/opam/default/.opam-switch/build/base.v0.17.0/_build/default/src/int_math_stubs.c:200: first defined here
collect2: error: ld returned 1 exit status
** Fatal error: Error during linking

Also note that the caml_ prefix for C symbols is reserved by the OCaml runtime. I suggest JS libraries pick another prefix.

hhugo commented 3 months ago

Why is moving the stub in another file enough to fix the issue ?

MisterDA commented 3 months ago

This was my initial question:

I've just noticed that the new version of JS base doesn't build anymore on Windows: it's defining a caml_csel_value function with CAMLweakdef, but their ocaml_intrinsic_kernel package also defines that same function, without CAMLweakdef. I understand there's no easy definition of weak linking on Windows?

This PR is based on @dra27's explanations:

Sadly, this is true - there's a very strange selectany, but its semantics are really odd The best way - which is used in libcamlrun already - is to put the "weak" function in its own .o file Because it is well defined that if you have already linked a symbol, then a .o file doesn't get pulled in when linking a .a file (we use this with main.o in libcamlrun.a)

MisterDA commented 3 months ago

Hmm, it appears I had only tested ocaml_intrinsics_kernel and base, but installing ppx_base still fails (as someone reported, then removed their comment). The catch is that the same patch needs to be applied to ocaml_intrinsics_kernel.

Chimrod commented 3 months ago

Yes, I’ve tested the branch and got the same error. After my comment, I’ve been in doubt if I set up the branch properly when I compiled the package and I’ve removed the comment. (after checking I confirm I was right, but I didn’t had the opportunity to connect here again :)

MisterDA commented 3 months ago

Could you confirm that with both patches in base and ocaml_intrinsics_kernel, the compilation of ppx_base succeeds?

Chimrod commented 3 months ago

I just tried this morning and with the two branches I’ve been able to build the package properly. (I did not test any code using it but the compilation went fine)

MisterDA commented 3 months ago

I've removed the symbol from base as it is now provided by ocaml_intrinsics_kernel. This also fixes the problem.

dkalinichenko-js commented 2 months ago

Hi, thanks for your PR!