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 build on 5.3.0+trunk #9733

Merged
merged 2 commits into from Jan 15, 2024
Merged

Fix build on 5.3.0+trunk #9733

merged 2 commits into from Jan 15, 2024

Conversation

jmid
Copy link
Contributor

@jmid jmid commented Jan 15, 2024

After a recent change to the internal compiler headers, dune no longer compiles on trunk (named 5.3.0) on macOS.
I spotted it since it is breaking out multicoretests CI run:
https://github.com/ocaml-multicore/multicoretests/actions/runs/7502879005/job/20426416884?pr=431#step:5:128

  #=== ERROR while compiling dune.3.12.2 ========================================#
  # context     2.1.5 | macos/x86_64 | ocaml-variants.5.3.0+trunk | git+https://github.com/ocaml/opam-repository.git
  # path        ~/work/multicoretests/multicoretests/_opam/.opam-switch/build/dune.3.12.2
  # command     ~/.opam/opam-init/hooks/sandbox.sh build ocaml boot/bootstrap.ml -j 4
  # exit-code   2
  # env-file    ~/.opam/log/dune-5096-92e0aa.env
  # output-file ~/.opam/log/dune-5096-92e0aa.out
  ### output ###
  # ocamlc -output-complete-exe -w -24 -g -o .duneboot.exe -I boot -I +unix unix.cma boot/libs.ml boot/duneboot.ml
  # ./.duneboot.exe -j 4
  # cd _boot && /Users/runner/work/multicoretests/multicoretests/_opam/bin/ocamlopt.opt -c -g -I +unix -I +threads dune_digest_stubs.c
  # In file included from src/dune_digest/dune_digest_stubs.c:10:
  # In file included from /Users/runner/work/multicoretests/multicoretests/_opam/lib/ocaml/caml/md5.h:24:
  # In file included from /Users/runner/work/multicoretests/multicoretests/_opam/lib/ocaml/caml/io.h:26:
  # /Users/runner/work/multicoretests/multicoretests/_opam/lib/ocaml/caml/platform.h:79:17: error: implicit declaration of function 'atomic_load_acquire' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  #     uintnat v = atomic_load_acquire(p);
  #                 ^
  # 1 error generated.
  # 

The fix is to #define CAML_INTERNALS before loading any ocaml headers as documented in the OCaml manual:
https://v2.ocaml.org/manual/intfc.html#ss:c-internals

"Since OCaml 4.04, it is possible to get access to every part of the internal runtime API by defining the CAML_INTERNALS macro before loading caml header files."

CC to @dra27 who co-authored the PR

Signed-off-by: Jan Midtgaard <mail@janmidtgaard.dk>
@@ -1,3 +1,4 @@
#define CAML_INTERNALS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth commenting here that it's needed to access the functions in md5.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I added a comment in 818a6c3

Signed-off-by: Jan Midtgaard <mail@janmidtgaard.dk>
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. #undef'ing CAML_INTERNALS does not work, anyway.

@emillon
Copy link
Collaborator

emillon commented Jan 15, 2024

In terms of metadata, that probably means we'll have some versions to restrict in opam-repository but they don't claim compat with 5.3 anyway.

@emillon
Copy link
Collaborator

emillon commented Jan 15, 2024

Thanks!

@emillon emillon merged commit b619366 into ocaml:main Jan 15, 2024
25 checks passed
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.

None yet

3 participants