Switch to Dune

[snowleopard]

As we discussed previously by email, in this PR we switch the library to building with Dune.

I updated the README with new instructions, but here is a quick summary:

• Build: dune build.
• Test: dune exec test/test.exe, dune exec test/prngtest.exe or dune exec test/speedtest.exe.
• Install: dune install.
• Build docs: dune build @doc, see _build/default/_doc/_html/cryptokit/Cryptokit/index.html.

There is a special file src/compute_flags.ml, invoked during the build, which takes care of computing various flags that depend on the system and architecture. This can be improved by allowing the user to override computed values, or by integrating with dune-configurator which is a configure-like system for checking if a certain library exists, a certain flag is supported, etc. Please let us know your thoughts on this.

This PR currently deletes all OASIS-related files, because maintaining two build systems is painful. We could keep OASIS for some time, just in case, but we don't recommend this: it would be better to fix any issues with Dune instead (we're happy to help).

/cc @jeremiedimino

[xavierleroy]

Thanks for the code and apologies for the delay in reviewing.

Overall, this looks very good to me. I'm a bit disappointed by the auto-configuration part, which is both a lot of code and not very subtle.

As an example of "subtility" we would like to add -maes if we're on x86 and the C compiler supports this flag. The Oasis code could not express that, so we went for a not-subtle criterion "x86 and not Windows" which Oasis expresses nicely and concisely.

If we were to "do the right thing" with -maes, how would we go about it? Use dune-configurator? Use autoconf? Something else?

[snowleopard]

@xavierleroy Thanks! I'm happy to make the auto-configuration part a bit smarter. Indeed, dune-configurator should help.

we would like to add -maes if we're on x86 and the C compiler supports this flag

Unpacking the "C compiler supports this flag" part: is there a simple test we can use for checking that a C compiler supports this flag? Do we expect that the compilation will just fail if we pass this flag when compiling a dummy C file, producing an error like "unrecognized command line option -maes"? If yes, that should be easy to automate.

[xavierleroy]

Unpacking the "C compiler supports this flag" part: is there a simple test we can use for checking that a C compiler supports this flag? Do we expect that the compilation will just fail if we pass this flag when compiling a dummy C file, producing an error like "unrecognized command line option -maes"?

Yes, pretty much. The only difficulty I've encountered is that, when faced with an unknown option, some C compilers stop with a nonzero exit code, and other C compilers print a warning and go on, exiting with 0 code.

Here is how we do it in CompCert to check for availability of a link-time option (-no-pie):
https://github.com/AbsInt/CompCert/blob/master/configure#L441-L461

For a compile-time option such as -maes, it's probably enough to compile an empty .c file (instead of a complete C program).

Is a test like this appropriate for dune-configurator? On the one hand it would be very useful, and not just for Cryptokit. On the other hand, it's a bit of a hack, owing to the grepping for keywords in standard error. Your call.

[snowleopard]

@xavierleroy I've added a basic test using dune-configurator, please have a look. This seems to work well on my machine but it would be great to test it in other typical environments.

The current implementation doesn't correctly handle the situation where a compiler happily prints a warning about an ignored flag instead of failing with an error. It makes sense to add support for this to dune-configurator but that will take some time.

Is the current solution sufficient or would you prefer to make the logic more robust, similar to the CompCert check you linked? It's not too difficult (so I'm happy to do it) but the resulting logic would be a bit more ad-hoc.

[snowleopard]

@xavierleroy Just a reminder: I think this PR is now complete and can be merged if you're happy with the implementation.

I've also tested that this version of cryptokit can be built by Dune as part of the Jane Street universe.

[xavierleroy]

Thanks for the ping and sorry for the delays.

To be entirely honest, I'm still not happy with the configuration part. In the current Oasis-based system, a Windows user can configure with --enable-zlib to force Zlib support even though Cryptokit defaults to no Zlib on Windows. I'm not seeing a similar mechanism in the current code, just the computation of the default settings for Zlib.

I'm very new to Dune, so I may miss the obvious, but how can users pass configuration options to a Dune build? is there an equivalent to ./configure <options> that produces a configuration that users can review before building?

[snowleopard]

@xavierleroy At the moment Dune doesn't provide support for the configuration step. @jeremiedimino and I have just discussed this and we agree that a sub-command like dune configure would be very useful in many situations. We hope to add it in future.

For now, we decided to implement a custom configure script that can be used as follows:

$ ./configure --help
Usage: ./configure [OPTIONS]
  --enable-zlib              Enable ZLib
  --disable-zlib             Disable ZLib
  --enable-hardwaresupport   Enable hardware support for AES and GCM (needs GCC or Clang)
  --disable-hardwaresupport  Disable hardware support for AES and GCM (needs GCC or Clang)
  -help                      Display this list of options
  --help                     Display this list of options

# Run configure with default settings
$ ./configure 
       flags src/flags.sexp,src/library_flags.sexp
ZLib: ............................... enabled
Hardware support for AES and GCM: ... enabled

# Override one of the settings
$ ./configure --disable-zlib
       flags src/flags.sexp,src/library_flags.sexp
ZLib: ............................... disabled
Hardware support for AES and GCM: ... enabled

Calling the configure script before dune build is optional: if you skip it, Dune will just use the default settings. If you override some of the settings, dune build will pick up the changes.

Hope this works for you, although we admit that we needed to write more custom code for this than we would have liked.

[xavierleroy]

Thanks a lot for your patience! The "configure" script seems to work pretty well, and the same approach could apply to other libraries. So, I'm happy.

The one thing that still worries me is that dune build says:

File "_unknown_", line 1, characters 0-0:
Error: File unavailable: prngtest.native
File "_unknown_", line 1, characters 0-0:
Error: File unavailable: speedtest.native
File "_unknown_", line 1, characters 0-0:
Error: File unavailable: test.native
       flags src/flags.sexp,src/library_flags.sexp
ZLib: ............................... enabled
Hardware support for AES and GCM: ... enabled

I would expect dune to either build the tests successfully, or explain why it cannot.

[xavierleroy]

Post-scriptum: I would be perfectly happy if running ./configure was mandatory, and dune build doesn't attempt to run it by itself, if that's what causes the problem.

[snowleopard]

Glad to hear you like how the configure script works!

I didn't realise that tests can't be built in some configurations. I'll think about possible solutions.

Strangely, I can't reproduce the poor error messages from your example. If I first run ./configure --disable-zlib and then do dune build, it fails but does prints some error messages to explain why:

Click to expand!

File "_none_", line 1: 
Error: Error on dynamically loaded library: ./src/dllcryptokit_stubs.so: ./src/dllcryptokit_stubs.so: undefined symbol: mlraise
    ocamlopt test/prngtest.exe (exit 2)
(cd _build/default && /usr/local/home/amokhov/.opam/default/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -o test/prngtest.exe /usr/local/home/amokhov/.opam/default/lib/ocaml/unix.cmxa -I /usr/local/home/amokhov/.opam/default/lib/ocaml /usr/local/home/amokhov/.opam/default/lib/zarith/zarith.cmxa -I /usr/local/home/amokhov/.opam/default/lib/zarith src/cryptokit.cmxa -I src test/.prngtest.eobjs/native/dune__exe__Prngtest.cmx)
src/libcryptokit_stubs.a(stubs-zlib.o): In function `caml_zlib_not_supported':
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:191: undefined reference to `alloc_small'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:189: undefined reference to `invalid_argument'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:194: undefined reference to `mlraise'
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking
    ocamlopt test/speedtest.exe (exit 2)
(cd _build/default && /usr/local/home/amokhov/.opam/default/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -w -35 -g -o test/speedtest.exe /usr/local/home/amokhov/.opam/default/lib/ocaml/unix.cmxa -I /usr/local/home/amokhov/.opam/default/lib/ocaml /usr/local/home/amokhov/.opam/default/lib/zarith/zarith.cmxa -I /usr/local/home/amokhov/.opam/default/lib/zarith src/cryptokit.cmxa -I src test/.speedtest.eobjs/native/dune__exe__Speedtest.cmx)
src/libcryptokit_stubs.a(stubs-zlib.o): In function `caml_zlib_not_supported':
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:191: undefined reference to `alloc_small'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:189: undefined reference to `invalid_argument'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:194: undefined reference to `mlraise'
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking
    ocamlopt test/test.exe (exit 2)
(cd _build/default && /usr/local/home/amokhov/.opam/default/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -w -35 -g -o test/test.exe /usr/local/home/amokhov/.opam/default/lib/ocaml/unix.cmxa -I /usr/local/home/amokhov/.opam/default/lib/ocaml /usr/local/home/amokhov/.opam/default/lib/zarith/zarith.cmxa -I /usr/local/home/amokhov/.opam/default/lib/zarith src/cryptokit.cmxa -I src test/.test.eobjs/native/dune__exe__Test.cmx)
src/libcryptokit_stubs.a(stubs-zlib.o): In function `caml_zlib_not_supported':
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:191: undefined reference to `alloc_small'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:189: undefined reference to `invalid_argument'
/usr/local/home/amokhov/code/cryptokit/_build/default/src/stubs-zlib.c:194: undefined reference to `mlraise'
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking

The error messages I get aren't ideal but better than those from your example.

I would be perfectly happy if running ./configure was mandatory, and dune build doesn't attempt to run it by itself, if that's what causes the problem.

I don't think this is related to the problem. dune build doesn't attempt to run configure by itself, it simply falls back to reading the configuration settings from config_vars.ml.default that sets both of them to Auto.

[xavierleroy]

False alarm. Maybe I had leftover files, but after a git clean, dune build works.

The "no zlib" stub code is broken, which is why compilation fails in --disable-zlib mode, but that's something I'll fix next.

Time to merge! Thanks again for all the work.

[snowleopard]

Happy to help, and thank you for the review!

[rgrinberg]

Exciting news. Would it be possible to release a new version of crpytokit to opam?

[xavierleroy]

I released version 1.16 and did a opam publish. Hope it will work.