dune icon indicating copy to clipboard operation
dune copied to clipboard

[new feature] Mode-specific foreign stubs

Open voodoos opened this issue 3 years ago • 5 comments

This PR implements the proposal in https://github.com/ocaml/dune/issues/4639 It allows one to specify distinct flags and sources when building the same stubs for different modes (byte/native). This is only possible for foreign stubs, not foreign libraries. It should not be too hard to allow it for foreign-libraries too but this PR does not implement it right now.

All relevant tests are passing but there are still a few tasks remaining:

  • [x] Re-implement coherence checks for foreign_stubs sources
  • [x] Refactor and clean the implementation (remove traces of back-and-forth, fix todos and review interfaces)
  • [x] Add an experimental project option to enable/disable this feature
  • [x] Test on the original use case: the compiler build

voodoos avatar May 02 '22 19:05 voodoos

@mshinwell @snowleopard: I was able to build the flambda-backend with this reimplementation of mode-specific foreign stubs

The rebased "special-dune" which has a lot less patches than before is here: https://github.com/voodoos/dune/commits/special_dune And the small fixes to the compiler build system are here: https://github.com/voodoos/flambda-backend/commit/57ea3724afcc47c3af50cf3a0a0a19a37d931ec0

voodoos avatar May 12 '22 16:05 voodoos

This is very good news, thanks. cc @stedolan The only other things on special_dune should be two bugfixes, one of which we probably won't need anyway after some Flambda backend build system changes are made imminently. I'll get these split out as PRs.

mshinwell avatar May 12 '22 16:05 mshinwell

This PR is now ready for review: only the docs update is missing.

A lot of the code addition goes into the Mode.MultiDict data-structure that is a bit heavy but significantly simplify the rest of the changes. It will also be a necessity if we want to have more than two modes (or "variants") in the future.

I am not sure about the necessity of the dune-project option still... not using the feature should result in absolutely no changes for the user. The option could make it more clear that the syntax is not yet vanilla and may change before becoming a first class citizen of the language, but is that really necessary ?

cc @emillon @snowleopard

voodoos avatar Aug 26 '22 13:08 voodoos

@voodoos Great, thank you!

The changes to the Dune engine are small, and they look fine! I'd like @emillon to review the rest of the PR since he has more familiarity with Dune rules.

only the docs update is missing

Please don't leave the docs update for a separate PR :)

snowleopard avatar Aug 27 '22 16:08 snowleopard

Thanks for your review @emillon. I addressed the two biggest points: replacing the Hashtbl with a map (8bdcebd) and the project option with an experimental syntax (aacf42b).

I probably won't have time to address you other comments before the ICFP, but it is probably already worth it to review these two changes.

voodoos avatar Sep 09 '22 08:09 voodoos