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

[wip] Port to Dune #588

Closed
wants to merge 1 commit into from
Closed

[wip] Port to Dune #588

wants to merge 1 commit into from

Conversation

avsm
Copy link
Contributor

@avsm avsm commented Dec 31, 2018

This is a followon from #574 with the commit history cleaned up for easier intermediate examination of the approach. The current tree builds the libraries and examples with a single dune build, including all the libffi and other configuration logic. This is still a work-in-progress PR and will be rebased a few more times.

Motivation

With a port to dune, the ctypes library can be embedded in larger dune projects simply by including it in the directory tree of the bigger project. This in turn will allow MirageOS unikernels to use Ctypes seamlessly as part of embedded compilation, using the standard cross-compilation and library variants support built into Dune. The goal is to make Ctypes the default FFI interface to MirageOS, and have it work out of the box on all of the backends. All existing OS platforms should be supported as well before merge, including Windows.

Porting Approach

This PR still installs ocamlfind libraries in the same way as the previous Makefile infrastructure, so backwards compat is preserved. However, the findlib schema has been slightly modified to separate out the foreign library from the core ctypes library. We now install:

  • ctypes that contains ctypes.top and ctypes.stubs as before. There are aliases to ctypes.foreign that redirect existing uses of those.
  • ctypes-foreign contains the base, unthreaded and threaded libraries.

All of the configuration logic for libffi is now in src/ctypes-foreign-base, so deleting these directories will remove the ffi build logic without touching the core library.

Since dune by default has stricter warnings enabled (the default --profile=dev mode), the PR currently sprinkles files with [@@@warning tags. The warnings can be fixed as well if desired, but that would muddy this PR so not done yet.

Build Time

Some crude benchmarks on the clean build time show a modest improvement with dune:

$ cd ctypes.orig && time (make -j4 all; make -j4 examples)
real	0m16.243s
user	0m18.603s
sys	0m11.589s
$ cd ../ctypes.new && time dune build @all 
real	0m12.603s
user	0m19.178s
sys	0m11.091s

(The dune build isn't fully optimised yet -- in particular, the C symbol probing can be sped up with a single invocation to cc instead of 20).

TODO

@avsm avsm mentioned this pull request Dec 31, 2018
@avsm
Copy link
Contributor Author

avsm commented Jan 1, 2019

25ec72a switches over to using virtual_modules for ctypes-foreign, which is inline with the current Makefiles. It still doesnt do automatic selection of threaded/unthreaded, which is covered in ocaml/dune#1724

ctypes.opam Outdated Show resolved Hide resolved
.travis-ci.sh Outdated Show resolved Hide resolved
@NightBlues
Copy link

is the as-needed test for cc still needed?

Hi! I just want to notice that trouble with default ubuntu ld behavior described in
#49
still reproduces for Ubuntu 18.04, while MacOS Mojave still uses clang and does not support flag --no-as-needed, so dune config:

(library
    (name my_c_bindings)
  (libraries ctypes ctypes.foreign)
(c_library_flags (-Wl,--no-as-needed -l<some_c_lib>)))

works for ubuntu and fails for mac (removing -Wl,--no-as-needed gives inverse behavior)

@avsm
Copy link
Contributor Author

avsm commented May 18, 2019

I've worked around the ocaml/dune#1730 limitation via a configure pass in 78cad0b, which is working well for test-enums (d72e533) for example

@avsm
Copy link
Contributor Author

avsm commented May 22, 2019

All tests now ported; time to hook in CI

@avsm
Copy link
Contributor Author

avsm commented Jul 17, 2019

I'm working on rebasing this to a set of clean commits. @talex5 is helping with deploying a CI to run through the foreign architectures without needing qemu. We'll try that on this branch as a test.

@avsm
Copy link
Contributor Author

avsm commented Mar 18, 2020

The latest patchset on this branch improves by:

  • added github actions, to test on macos and windows. Windows now builds on mingw64
  • packaging fixes for ctypes vs ctypes-foreign, particularly for tests
  • port to dune 2.0 language, which allows for the old ctypes.foreign ocamlfind name to be deprecated by dune itself, rather than META file hacks

Comment on lines 87 to 90
let ls = String.lowercase_ascii s in
let s' =
if Filename.check_suffix ls ".so" ||
Filename.check_suffix ls ".dylib"
Copy link

Choose a reason for hiding this comment

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

Using lowercase_ascii breaks 4.02.3 compatibility. The fix is easy:

Suggested change
let ls = String.lowercase_ascii s in
let s' =
if Filename.check_suffix ls ".so" ||
Filename.check_suffix ls ".dylib"
let s' =
if Filename.check_suffix s ".so" ||
Filename.check_suffix s ".dylib"

Filename.check_suffix has done the correct thing since the dawn of time (ocaml/ocaml@4ef227d), even though it wasn't documented until the year before last (ocaml/ocaml@a7a76fd)

@yallop
Copy link
Owner

yallop commented Apr 23, 2020

It looks like this is using library_variants, which is currently under consideration for removal from dune. Is there a suitable alternative if the removal goes ahead?

@avsm
Copy link
Contributor Author

avsm commented Apr 23, 2020

We could simply drop the unthreaded variant. We could do this in stages: merge the existing port, and subsequently remove it (and the use of variants). We could also propose specific support in dune for supporting unthreaded libraries, since they’re a bit special in terms of compiler options.

@yallop
Copy link
Owner

yallop commented Jul 11, 2020

@avsm:

We could simply drop the unthreaded variant.

See #651.

@avsm
Copy link
Contributor Author

avsm commented Jul 26, 2020

Merged against #651 and activated ocaml-ci on my fork. I'm seeing some failures on arm32 I need to chase down:

https://ci.ocamllabs.io/github/avsm/ocaml-ctypes/commit/96700a9886dfea4551192b2cc75de9d1e845b859/variant/debian-10-ocaml-4.10_arm32

#18 7.805 test_returning_errno alias tests/test-returning-errno/runtest (exit 1)
#18 7.805 (cd _build/default/tests/test-returning-errno && ./test_returning_errno.exe)
#18 7.805 munmap_chunk(): invalid pointer
#18 7.805 E
#18 7.805 ==============================================================================
#18 7.805 Error: Errno tests:0:calling stat.
#18 7.805 
#18 7.805 File "/src/_build/default/tests/test-returning-errno/oUnit-Errno tests-buildkitsandbox#00.log", line 49, characters 1-1:
#18 7.805 Error: Errno tests:0:calling stat (in the log).
#18 7.805 
#18 7.805 Worker stops running: Killed by signal -1
#18 7.805 ------------------------------------------------------------------------------
#18 7.805 Ran: 1 tests in: 1.00 seconds.
#18 7.805 FAILED: Cases: 1 Tried: 1 Errors: 1 Failures: 0 Skip:  0 Todo: 0 Timeouts: 0.
#18 7.857 test_threads alias tests/test-threads/runtest
#18 7.857 .....
#18 7.857 Ran: 5 tests in: 1.05 seconds.
#18 7.857 OK
#18 8.093 test_returning_errno alias tests/test-returning-errno-lwt-preemptive/runtest (exit 1)
#18 8.093 (cd _build/default/tests/test-returning-errno-lwt-preemptive && ./test_returning_errno.exe)
#18 8.093 ...E
#18 8.093 ==============================================================================
#18 8.093 Error: Errno tests:0:calling stat.
#18 8.093 
#18 8.093 File "/src/_build/default/tests/test-returning-errno-lwt-preemptive/oUnit-Errno tests-buildkitsandbox#00.log", line 75, characters 1-1:
#18 8.093 Error: Errno tests:0:calling stat (in the log).
#18 8.093 
#18 8.093 Worker stops running: Killed by signal -10
#18 8.093 ------------------------------------------------------------------------------
#18 8.093 Ran: 4 tests in: 1.20 seconds.
#18 8.093 FAILED: Cases: 4 Tried: 4 Errors: 1 Failures: 0 Skip:  0 Todo: 0 Timeouts: 0.
#18 8.125 test_returning_errno alias tests/test-returning-errno-lwt-jobs/runtest (exit 1)
#18 8.125 (cd _build/default/tests/test-returning-errno-lwt-jobs && ./test_returning_errno.exe)
#18 8.125 double free or corruption (out)
#18 8.125 ...E

(I need to check if this also reproduces on the mainline ctypes, or if it's a bug specific to the build rules in this PR)

And oddly, a bug with a multiply defined symbols in Fedora 32 worked around by 96700a9. This doesn't happen in the mainline branch, so it might be some over-eager linkage in the test_functions/clib area in this PR.

@avsm
Copy link
Contributor Author

avsm commented Jul 26, 2020

To broaden that, all the Lwt-based tests seem to fail in the PR at the moment, but only on Linux but not macOS.

@yallop
Copy link
Owner

yallop commented Aug 5, 2020

There's another build change just gone into master: #656.

@olafhering
Copy link

The dune-port branch breaks consumers of ctypes because the ctypes.cmxa lacks the -I $(ocamlc -where)/integers include path for C stubs. See ocamlobjinfo ctypes.cmxa|head from a dune build and from a make build. dune may just lack the required knobs to add the required info.

This breaks at least luv.

@yallop
Copy link
Owner

yallop commented Jun 3, 2023

Thank you for for continuing to work on this, @emillon. I like the goal of avoiding upper-bounds in opam-repository. I'd also keep the ability of installing ctypes without ctypes-foreign, and it looks like your new package arrangement does that, too.

@emillon
Copy link
Contributor

emillon commented Jun 6, 2023

Some updates. I made the following changes:

  • 5.0.0 compatibility fixes: remove bytes from (libraries), rename kprintf to ksprintf
  • fixed some missing or imprecise opam dependencies
  • I'm looking for a good workaround for enabled_if doesn't work in (test) ocaml/dune#6938 to have enabled_if supported in examples. But I think I'll have to bite the bullet and implement it in dune and disable the examples in the meantime.

@emillon
Copy link
Contributor

emillon commented Jun 6, 2023

And the revdeps results are pretty satisfactory, but several packages have missing dependencies on ctypes.foreign or ctypes.stubs, including tsdl, mariadb, hacl-star.

@emillon
Copy link
Contributor

emillon commented Jun 7, 2023

Regarding tests on windows. The crux of the problem is that there's no way at the moment with Dune to attach tests to a package and restrict them to a particular platform (ocaml/dune#6938).
In the current state of affairs, this means that examples/fts would need to be disabled ((enabled_if false) with a comment explaining the situation).

To improve this, I implemented a new feature in ocaml/dune#7899: (build_if) extends (enabled_if) so that it does not even attempt to build the test executable when the clause evaluates to false.
I pushed a branch that uses it, and this gives us a green CI on all versions and platforms.

I'm happy with this solution but we might not want wait for this feature to be released (and to bump the dune dependency to >= 3.9), just to check examples.
My recommendation would be to first release the dune port with this example disabled, then add it back using the new dune feature once 3.9 is reasonably not bleeding edge.

Next step for me is to do a proper differential testing of revdeps to have a definitive list of regressions.

@avsm
Copy link
Contributor Author

avsm commented Jun 7, 2023

That's an extremely useful feature in dune, also handy for eio! (/cc @talex5)

You could put the examples in a separate opam package for now (ctypes-examples) which may make things easier. Also, feel free to push your branch to my fork if useful, as then it'll show up against this PR (and preserve comments/etc).

@emillon
Copy link
Contributor

emillon commented Jun 8, 2023

You could put the examples in a separate opam package for now (ctypes-examples) which may make things easier.

Actually I reused the workaround in eio, which consists in using OCaml syntax to evaluate the enabled_if. This means we don't have to disable the tests nor use a separate package.

Also, feel free to push your branch to my fork if useful, as then it'll show up against this PR (and preserve comments/etc).

I don't have the rights to do that but I'm keeping the branch updated.

Next: revdeps!

@avsm
Copy link
Contributor Author

avsm commented Jun 8, 2023

Actually I reused the workaround in eio, which consists in using OCaml syntax to evaluate the enabled_if. This means we don't have to disable the tests nor use a separate package.

That workaround breaks embedding the repository into a monorepo as a subdirectory.

@emillon
Copy link
Contributor

emillon commented Jun 8, 2023

Thanks, I pushed my branch to yours.

That workaround breaks embedding the repository into a monorepo as a subdirectory.

Can you tell me more about that? Specifically I'm referring to this. I tried to build ctypes in a subdirectory and it succeeded, but maybe more is needed to demonstrate the issue.

@emillon
Copy link
Contributor

emillon commented Jun 13, 2023

I ran some differential testing of 0.20.2 vs 0.20.2+dune3 (as tagged on my fork; 16db268 here).

Considering the packages that failed on 0.20.2+dune3 but didn't on 0.20.2, there are:

All the fixes I've tried are fairly easy: replace ctypes with ctypes.stubs or ctypes.foreign in the build system. So it's likely that these are accepted upstream. It is also possible to patch the existing packages, though that would be up to their maintainers and opam-repository maintainers. I've included some patches in ocaml/opam-repository#23866 to test that they apply and fix the issue.

The testing plan seems pretty extensive:

  • the github action in this repo covers macos and windows
  • this opam-repository run covers many distributions and ocaml versions (I should note that a test fails on mac on opam-repo-ci and not in the github action, but it's not a regression related to this port. 0.20.2 fails too)
  • I tried to use this in a monorepo context with success

In terms of next steps, I'll leave that up to you but I would suggest:

  • (*) squash the existing work
  • cut a release
  • (*) add required upper bounds on opam-repository in the release PR
  • (*) submit patches to broken revdeps
  • (*) on the dune side, release a version with add: build_if in test stanza ocaml/dune#7899
  • (*) open a PR on ctypes that uses this feature
  • merge it once it's acceptable to depend on 3.9+

Steps with (*) are where I can help.

Let me know what you think.

@yallop
Copy link
Owner

yallop commented Jun 13, 2023

The plan sounds good to me. I can make a release once you let me know that these changes are ready to merge.

@emillon
Copy link
Contributor

emillon commented Jun 21, 2023

To conclude the testing saga:

  • @TheLortex & I tested this in the context of a mirage unikernel. this worked fine, with the caveat that it was used in the host context. But it doesn't seem that mirage would support ctypes in its cross-compilation context due to missing headers. But that issue also affects the existing version of ctypes in use by mirage, so that's not related to this port.
  • I also tested this against the dune test suite for the experimental (ctypes) support and this works (dune links to both ctypes and ctypes.stubs)

So it's time for me to squash this and write a very long commit message.

@emillon
Copy link
Contributor

emillon commented Jul 3, 2023

I squashed the work with an extensive commit message and pushed.

Motivation
==========

With a port to dune, the ctypes library can be embedded in larger dune
projects simply by including it in the directory tree of the bigger
project. This in turn will allow MirageOS unikernels to use Ctypes
seamlessly as part of embedded compilation, using the standard
cross-compilation and library variants support built into Dune. The goal
is to make Ctypes the default FFI interface to MirageOS, and have it
work out of the box on all of the backends. All existing OS platforms
should be supported as well before merge, including Windows.

Porting Approach
================

This PR still installs ocamlfind libraries in the same way as the
previous Makefile infrastructure, so backwards compat is preserved.
However, the findlib schema has been slightly modified to separate out
the foreign library from the core ctypes library. We now install:

- ctypes that contains ctypes.top and ctypes.stubs as before. There are
  aliases to ctypes.foreign that redirect existing uses of those.
- ctypes-foreign contains the foreign library.
  All of the configuration logic for libffi is now in
  src/ctypes-foreign-base, so deleting these directories will remove the
  ffi build logic without touching the core library.

Since dune by default has stricter warnings enabled (the default
--profile=dev mode), the PR currently sprinkles files with [@@@warning
tags. The warnings can be fixed as well if desired, but that would muddy
this PR so not done yet.

Layout
======

The previous packaging would install ctypes and ctypes.stubs in the same
library. Depending on the build system, it means that it's possible to
specify a dependency on ctypes and use ctypes.stubs succesfully. This is
not the case with Dune since it installs these in different directories.
This means that it is necessary to patch these reverse dependencies,
though it can be argued that they were relying on a bug.

Followup work
=============

A test uses OCaml syntax to skip test on windows. Once it's acceptable
to use Dune 3.9, it should be updated to use build_if.

Test and dynamic libraries
==========================

Due to a different linking model, there are some changes in the test
suite regarding dynamic library loading.

Dynamic symbols in ocaml <4.06
------------------------------

The setup for tests is that each test executable tests some properties
using both "stubs" and "foreign" strategies to refer to some symbols
define in `tests/clib/` (the `test_functions` library).

The symbols are accessed through both strategies:
- as a linked symbol (through an `external` via stubs)
- through the dynamic loader (through `dlopen` via foreign)

Dune links the stubs statically, so by default the symbols would not be
visible to the dynamic loader. OCaml sets `-Wl,-E` in `LDFLAGS` to make
these symbols visible at runtime, but until
ocaml/ocaml@edbba02
(between 4.05 and 4.06), this was ignored when linking executables.

So this commit adds these flags when building tests for older versions.
An alternative would be to use `(link_flag)` in an `(env)` stanza but
this requires dune 3.

This issue does not affect the original make-based build because it
assembles the test executables a bit differently: the `test_functions`
library is linked dynamically to the test executable. So the symbols are
already visible to the dynamic loader, even when `-Wl,-E` is not set.

clib loading
------------

This change is specific to windows (but does not change the behavior on
Linux)

It modifies the test setup a bit so that the contents of `clib` are
dynamically loaded at the beginning of each test. This ensures that
later foreign calls (that use the default handle) will succeed.

It was not necessary before because `clib` was linked dynamically. Now
that it is linked statically, dynamic symbols do not have access to the
symbols in the main executable. This is a problem on windows which does
not have the concept of `-Wl,--export-dynamic`. Also, on windows the
stubs DLL can not be loaded directly so we recompile the library using
plain `%{cc}` instead of going through ocamlopt and flexlink.

Instrumentation
===============

This uses dune instrumentation for coverage The instructions are now:

    opam install bisect_ppx
    dune runtest --instrument-with bisect_ppx --force
    bisect-ppx-report html

See <https://github.com/aantron/bisect_ppx#Dune>

Depext handling
===============

This port uses conf-libffi instead of hardcoding depexts in
ctypes-foreign.
@yallop
Copy link
Owner

yallop commented Jul 7, 2023

I've merged the squashed commit to master (0f8a92a), but left this PR open, since there's more to do. There's a release in preparation (#745), which I'll finish in a few days, unless we discover any issues in the meantime.

@yallop
Copy link
Owner

yallop commented Jul 12, 2023

There's a draft release here: ocaml/opam-repository#24101

@emillon, is that everything you need for the next step of your plan ("add required upper bounds on opam-repository in the release PR")?

@emillon
Copy link
Contributor

emillon commented Jul 13, 2023

Great! Yes, I should be able to push to your branch and make the necessary changes on the PR directly. This is going to take some time to run and I'm off tomorrow so I'll probably have to finish that on Monday.

@emillon
Copy link
Contributor

emillon commented Jul 17, 2023

@emillon
Copy link
Contributor

emillon commented Jul 17, 2023

I sent PRs to the various projects, links are above.
For llvm, actually the problem was with ctypes itself: there was no version information in the release tarball, so the configuration script could not determine the version of ctypes.
The idiomatic way to do that with dune is to add (version 0.21.0) in dune-project, in the release tarball only. When present, the META files get the correct version number (which in turn can be queried with ocamlfind query), and some features like dune-build-info will report the correct version.
In turn, the recommended way to set this (version) info is by using dune-release which does that automatically.

@yallop
Copy link
Owner

yallop commented Jul 17, 2023

How do you recommend handling the missing version information in the 0.21.0 release? For example, we could

  • release 0.21.1, and make 0.21.0 unavailable in opam-repository
  • add a patch that injects the version to the metadata in opam-repository [Edit: I see you've added the patch already.]

@emillon
Copy link
Contributor

emillon commented Jul 17, 2023

Ah I should have mentioned that: I added a patch in the release PR that edits the dune-project file.

@avsm
Copy link
Contributor Author

avsm commented Jul 17, 2023

ocaml/opam-repository#24101 hasn't been merged yet. It's not too late to respin the 0.21.0 release to add the version patch, or replace the package in that PR with a 0.21.1 (if you prefer not to slip the tag). Where possible, we prefer not to carry patches in the opam-repository and have them upstream.

yallop added a commit that referenced this pull request Jul 20, 2023
@avsm
Copy link
Contributor Author

avsm commented Jul 24, 2023

And announced on Discuss, so closing this PR as completed. Thank you for all the help pushing this over the finish line, @emillon and @yallop!

https://discuss.ocaml.org/t/ann-ctypes-0-21-1-now-with-dune-support/12675

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.