[new feature] Mode-specific foreign stubs by voodoos · Pull Request #5649 · ocaml/dune

voodoos · 2022-05-02T19:11:32Z

This PR implements the proposal in #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:

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

voodoos · 2022-05-12T16:25:16Z

@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: voodoos/flambda-backend@57ea372

mshinwell · 2022-05-12T16:54:26Z

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.

src/dune_engine/dune_project.ml

src/dune_engine/dune_project.mli

src/dune_rules/dune_package.ml

src/dune_rules/foreign.ml

src/dune_rules/install_rules.ml

src/dune_rules/lib_rules.ml

voodoos · 2022-08-26T13:57:41Z

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

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

…/fsmd-exe.t/run.t Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

rgrinberg · 2022-10-14T14:51:13Z

src/ocaml/mode.mli

+
+(** [Select] is a utility module that represents a mode selection. *)
+module Select : sig
+  type mode = t


@emillon can you remove this type alias?

rgrinberg · 2022-10-14T14:51:21Z

src/ocaml/mode.mli

+    apply to any mode while keys of the form [Select.Only _] designate values
+    that apply to specific modes. *)
+module Map : sig
+  type mode = t


rgrinberg · 2022-10-14T14:52:53Z

src/ocaml/mode.mli

+    (** Returns the list of values associated to a specific mode. If the
+        [and_all] option (which defaults to [false]) is set to true then values
+        which are not associated to a specific mode are also returned. *)
+    val for_only : ?and_all:bool -> 'a t -> mode -> 'a list


Please remove this optional argument as well.

rgrinberg · 2022-10-14T14:55:06Z

src/dune_rules/foreign.ml


-  let object_name t =
-    t.path |> Path.Build.split_extension |> fst |> Path.Build.basename
+  let object_name ?(with_mode_suffix = true) t =


Can you remove this optional argument? Thanks.

rgrinberg · 2022-10-14T15:00:37Z

src/dune_rules/foreign.mli

  val decode : t Dune_lang.Decoder.t

-  val build_paths : t -> ext_obj:string -> dir:Path.Build.t -> Path.Build.t list
+  val build_paths : t -> ext_obj:string -> dir:Path.Build.t -> Path.t list


Why was this type changed?

I had to modify consumers of the output of this function for my changes and noticed that all its usages where followed by a List.map ~f:Path.build so I "upstreamed" it.

rgrinberg · 2022-10-14T15:02:09Z

src/dune_rules/foreign.ml

+      match mode with
+      | Some _ when for_library ->
+        User_error.raise ~loc:loc_mode
+          [ Pp.textf "The field \"mode\" is not available for foreign_libraries"


"foreign_libraries" isn't a stanza. Should be singular or just "foreign libraries"

rgrinberg · 2022-10-14T15:03:33Z

src/dune_rules/foreign.mli

-     file [some/path/name.cpp]. *)
-  val object_name : t -> string
+     file [some/path/name.cpp]. [with_mode_suffix] defaults to true. *)
+  val object_name : ?with_mode_suffix:bool -> t -> string


Also, can you explain how this with_mode_suffix is supposed to work and how should the caller decide to provide it?

I completely removed it: 7137b68

It was only used to hide the mode suffix in an error message which was probably even more confusing.

I finally added it back (with an additional function) in order to improve the error message:
571a4f3

rgrinberg · 2022-10-14T16:26:50Z

src/dune_rules/dune_file.ml

+      if List.is_empty t.buildable.foreign_stubs then None
+      else Some (Foreign.Archive.stubs (Lib_name.Local.to_string (snd t.name)))
+    in
+    (lib_stubs, List.map ~f:snd t.buildable.foreign_archives)


Instead of returning a tuple, let's just have two separate functions. The return type is just confusing and there's no performance to gain from computing them together.

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) CHANGES: - macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg) - Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg) - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses `Findlib`). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231, fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter) - The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin ...)`, the `META` file for it is built before calling `coqdep`. This enables the use of the new `Findlib`-based loading method in Coq 8.16; however as of Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 , workarounds ocaml/dune#5767, @ejgallego) - Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs) - Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528, @emillon) - Add a new experimental feature `mode_specific_stubs` that allows the specification of different flags and sources for foreign stubs depending on the build mode (ocaml/dune#5649, @voodoos)

voodoos marked this pull request as ready for review May 3, 2022 00:04

voodoos marked this pull request as draft May 3, 2022 00:07

voodoos force-pushed the feature/foreign-stubs branch from 6d95bea to 081fe68 Compare May 3, 2022 21:01

voodoos force-pushed the feature/foreign-stubs branch 2 times, most recently from dc7cf64 to a9f9ca2 Compare May 13, 2022 14:16

voodoos force-pushed the feature/foreign-stubs branch 2 times, most recently from 51850dd to 380817f Compare June 3, 2022 08:45

voodoos force-pushed the feature/foreign-stubs branch 5 times, most recently from 4b7a730 to 1ed47ef Compare July 15, 2022 09:56

emillon reviewed Jul 19, 2022

View reviewed changes

emillon added this to the 3.5.0 milestone Jul 19, 2022

emillon mentioned this pull request Aug 1, 2022

Set mode-only flags #1481

Open

voodoos force-pushed the feature/foreign-stubs branch 5 times, most recently from c4319cc to 895bfbe Compare August 3, 2022 16:06

snowleopard mentioned this pull request Aug 4, 2022

confused by Dune foreign_libraries doc: how to define a C library used by C stubs? #4409

Open

voodoos force-pushed the feature/foreign-stubs branch 5 times, most recently from c12b677 to b8de130 Compare August 26, 2022 13:35

voodoos marked this pull request as ready for review August 26, 2022 13:57

voodoos and others added 15 commits October 14, 2022 15:20

Update the tests with the new option

da768a6

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Remove useless project parameter in decoder

d914377

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Reword comment

ef52b89

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Simplify conditionals by using Mode.Dict.Set.to_list

23f540e

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Simplify c-stubs in tests

ffba254

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Clarify the fsmd-lib test results

ef1306c

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Use "dependEnt" in all tests

5fba312

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Add documentation for the feature

93b18e4

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Add a changelog entry

795a2ad

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Apply suggestions from code review

f3ff303

Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

tests: Apply suggestions from code review

6ea6223

Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

tests: another missing colon

1f9cefd

Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Update test/blackbox-tests/test-cases/foreign-stubs/fs-mode-dependent…

2edfe79

…/fsmd-exe.t/run.t Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Signed-off-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Remove todo

9d09e6f

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Remove alias and fix test formatting.

4e8a8c0

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos force-pushed the feature/foreign-stubs branch from 4066e27 to 4e8a8c0 Compare October 14, 2022 13:24

emillon merged commit bc0a0c3 into ocaml:main Oct 14, 2022

rgrinberg reviewed Oct 14, 2022

View reviewed changes

emillon mentioned this pull request Oct 14, 2022

Address comments on #5649 #6232

Closed

rgrinberg reviewed Oct 14, 2022

View reviewed changes

emillon mentioned this pull request Oct 19, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) ocaml/opam-repository#22317

Merged

jchavarri mentioned this pull request May 12, 2023

melange: expose a mechanism to enable conditional compilation #7717

Closed

nojb mentioned this pull request Jun 17, 2025

Dune builds C stubs with bytecode_cflags and never native_cflags #11923

Open

Conversation

voodoos commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshinwell commented May 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voodoos commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voodoos Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

voodoos commented May 2, 2022 •

edited

Loading

voodoos commented May 12, 2022 •

edited

Loading

voodoos commented Aug 26, 2022 •

edited

Loading

voodoos Oct 17, 2022 •

edited

Loading