Skip to content
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 Windows compilation failure ppx_base because of base and ocaml_intrinsics_kernel symbol clash #165

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Jun 5, 2024

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
Copy link
Contributor

hhugo commented Jun 9, 2024

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

@MisterDA
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@MisterDA MisterDA changed the title Fix Windows compilation failure of base and ocaml_intrinsic_kernel Fix Windows compilation failure ppx_base because of base and ocaml_intrinsics_kernel symbol clash Jun 10, 2024
@Chimrod
Copy link

Chimrod commented Jun 10, 2024

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
Copy link
Contributor Author

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

@Chimrod
Copy link

Chimrod commented Jun 11, 2024

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 MisterDA force-pushed the CAMLweakdef-Windows branch from aeb24fa to ea7d8e5 Compare June 13, 2024 21:35
@MisterDA
Copy link
Contributor Author

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

For Windows compatibility, or it conflicts with the symbol provided by
ocaml_intrinsics_kernel.

Signed-off-by: Antonin Décimo <antonin@tarides.com>
@MisterDA MisterDA force-pushed the CAMLweakdef-Windows branch from ea7d8e5 to 7901518 Compare June 17, 2024 09:18
@dkalinichenko-js dkalinichenko-js merged commit 670c0c6 into janestreet:master Jun 24, 2024
1 check passed
@dkalinichenko-js
Copy link
Contributor

Hi, thanks for your PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants