Add support for -H compiler flag #10644

nojb · 2024-06-11T15:57:33Z

This PR adds support to Dune to use the -H compiler flag introduced in ocaml/ocaml#12246, which provides good semantics to the (implicit_transitive_deps false).

This work was carried out by @MA0010 as part of an internship at LexiFi. I am opening this PR but @MA0010 will take the lead during the review process.

The first commit contains the meat of the change: we keep track of which dependencies are "direct" (= mentioned in the Dune file) and which dependencies are purely transitive. For direct dependencies we use -I and for transitive dependencies we use -H (of course, only for OCaml >= 5.2). The use of -H is also guarded by Dune version 3.17 (this is to minimize the possibility of breakage, also there are some corner cases where the change is not completely backwards-compatible).
The second commit adds an explicit unix dependency to some vendored libraries. This is needed because unix is mentioned in the source of these libraries, but the library is not mentioned in the Dune stanzas. The libraries are able to build before this PR because OCaml adds -I +unix implicitly (for recent enough versions of OCaml where unix is installed in the separate directory +unix). However, after this PR, the compiler passes -H +unix which disables the implicit addition of -I +unix breaking compilation (again, because Unix is a direct dependency of these libraries).
The third commit contains some tests. Writing the tests was a bit involved due to the need to have them pass both with versions of OCaml supporting -H and those that do not. Suggestions for improvement welcome.
The last commit bumps the Dune version to 3.17. We could consider not bumping the Dune language version since the new mode is almost always a strict improvement for users of (implicit_transitive_deps false) (modulo corner cases such as the one mentioned in the second bullet point). Opinions welcome.

Future work:

Adapt the Merlin machinery to pass -H as needed (there is already an open PR for this: Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535).
Maybe adapt the ocaml-index machinery accordingly (cc @voodoos)

Fixes #9333
Fixes ocsigen/tyxml#274
Fixes #2733
Fixes #4963

emillon · 2024-06-11T19:44:29Z

Hi, thanks for this PR and the detailed description. There are a couple things we can land first:

the 3.17 language version (we'll need it anyway and 3.16 has branched)
the fix for unix is useful anyway because it triggers a warning on 5.x and we're going to bump the test suite to use this version soon

Regarding your question:

Writing the tests was a bit involved due to the need to have them pass both with versions of OCaml supporting -H and those that do not. Suggestions for improvement welcome.

Yes. the reason it's painful is that we're only testing a single version of the compiler in CI, but you can add tests that require 5.x and mark them with (enabled_if (>= %{ocaml_version} 5.2)). This means that make test will work for them locally, and will work on 5.x in CI when we bump the CI version. Similarly for tests that rely on 4.x. They'll become dead tests when we bump the tested version but that can still be useful for some time.

MA0010 · 2024-06-12T13:43:31Z

Hello, thanks for the comment.

the 3.17 language version (we'll need it anyway and 3.16 has branched)

the fix for unix is useful anyway because it triggers a warning on 5.x and we're going to bump the test suite to use this version soon.

I have opened a separate PR to land the changes you proposed (see #10645). As you mentioned, those can be merged independently of the feature to be added through this PR. I will remove them from this PR as soon as they get merged successfully.

rgrinberg · 2024-06-13T21:39:50Z

src/dune_rules/lib.mli

@@ -235,3 +235,5 @@ module Local : sig

  include Comparable_intf.S with type key := t
 end
+
+module Tbl : Hashtbl.S with type key := t


I don't think you need this since our Table module is polymorphic. let table = Table.create (module Lib) should work

rgrinberg · 2024-06-13T21:41:07Z

src/dune_rules/lib_flags.mli

+
+  val include_flags
+    :  ?project:Dune_project.t
+    -> ?direct:(Lib.t -> bool)


Why can't we just pass a list: ([ Direct | Indirect] * Lib.t) list as an argument? I'm not a fan of passing around callbacks.

Hello,
I have removed the passed callback. Instead, now two lists are passed, one containing the direct dependencies (requires_direct) and the other contains the hidden dependencies (requires_hidden). The requires_hidden is also exposed in Compilation_context.t, and so can be used in other places when needed.

The code have changed a little bit, and the diff in lib_flags.ml is now smaller.

Review appreciated!

rgrinberg · 2024-06-13T21:43:01Z

src/dune_rules/compilation_context.ml

-    else requires_compile
+    then Memo.Lazy.force requires_link, Resolve.Memo.return (fun _ -> true)
+    else if Version.supports_hidden_includes ocaml.version
+            && Dune_project.dune_version project >= (3, 17)


I think this mode still needs to be guarded behind implicit_transitive_deps. Once we experiment with it more, we can enable it by default for newer compilers perhaps.

Sorry, but I do not seem to have understood well your point. The new feature is invoked only when implicit_transitive_deps is false, according to the written logic, or am I missing something?

Thanks for the review.

Yeah, that wasn't very clear at all. Let me explain the behavior I would like to achieve:

When we're using (implicit_transitive_deps false) and OCaml >= 5.2.0, we should use use your new implementation with the -H flag.

When we're using (implicit_transitive_deps false) and OCaml < 5.2.0, we should go back to using the old implementation of hiding -I flags.

When we're using (implicit_transitive_deps true), it should disable using -H flags completely.

The issue with the current behavior is that users do expect upgrading to 3.17 to be relatively easy. With this change, they will now need to look at all their transitive deps. While I agree that it's a good thing, it's too much of a breakage to introduce in a minor version bump.

Therefore, I suggest that we guard this behind a feature flag. We already have a feature flag though implicit_transitive_deps, so we can just reuse it.

rgrinberg · 2024-06-13T21:44:38Z

@voodoos could you review the implementation as well? I think you needed something similar for merlin. It would be good to see what can be shared.

voodoos

I think the way dependencies are exposed right now is too restricting. The Lib.Compile.t API only provide us with either require_compile, the list of direct dependencies, or require_link, the list of all transitive dependencies including the direct ones.

This means that we have to compute the disjunction require_link \ require_compile to get the actual "hidden" or "extra" dependencies. Right now this is needed in 3 places: rules for indexation (#10422), rules for merlin config (#10535) and this PR.

One issue is that require_compile and require_link are lists which prevent an efficient computation of the disjunction. In #10422 and #10535 this is done in a somewhat inefficient way using multiple calls to List.mem. This PR introduces a more efficient approach by using a set (the (lib, unit) Hashtbl.t) to perform more efficient membership tests.

I think we should update Lib.Compile.t to either provide an additional requires_extra list of libs that is computed in an efficient way and share the result, or have requires_compile and requires_link be Sets so that consumers can efficiently compute the disjunction without too much code duplication

This would allow the different places using the "extra deps" to do it in a uniform way.

Signed-off-by: HasanA <[email protected]>

…en (to be re-checked) Signed-off-by: HasanA <[email protected]>

MA0010 · 2024-06-18T15:10:34Z

Concerning the last two commits, they contain edits based on the reviews and comments (cc @voodoos @rgrinberg), resting the main goal of keeping track of hidden deps unchanged. Those dependencies are now tracked by a new list hidden_requires exposed in the Compilation_context.t type. In short:

The first of the new commits keeps the previous approach of using hashtables to compute the hidden dependencies, but now instead of being passed by a callback, they are stored in list requires_hidden and exposed in Compilation_context.t for external use cc (@voodoos). This list is then used to insert the -H flag for the correct deps.
The second one simply removes the custom hashtable module from lib.ml, in favor of the already defined Table module (@rgrinberg). Also, the ocaml_index.ml file is adapted to use the new requires_hidden list, however in a naive way, and maybe can be used to adapt the machinery better (cc @voodoos )

rgrinberg · 2024-06-19T00:06:32Z

src/dune_rules/lib_flags.ml

+      to_iflags
+        (include_paths ?project ts_direct { lib_mode = mode; melange_emit = false })
+    in
+    Command.Args.S [ direct_includes; hidden_includes ]


Isn't this re-ordering the include flags? e.g. if we have (libraries x y), we'd have closure(x) closure(y), and now we have x y closure(x) closure(y)?

If I understand things correctly, I don't think anything is being re-ordered; instead some extra -H flags are being added at the "end" of the command-line when implicit_transitive_deps is false:

If implicit_transitive_deps is true, then direct_includes is requires_link (from Compilation_context), and hidden_includes is empty; this is the same as today, and nothing changes.

If implicit_transitive_deps is false (and OCaml is recent enough, etc) direct_includes is requires_compile (from Compilation_context), and hidden_includes contains some extra flags relative to today, but the order of direct_includes does not change.

Do you agree with this understanding @MA0010?

nojb · 2024-06-20T06:24:05Z

src/dune_rules/lib_flags.ml

+    let hidden_includes =
+      to_hflags
+        (include_paths ?project ts_hidden { lib_mode = mode; melange_emit = false })
+    in
+    let direct_includes =
+      to_iflags
+        (include_paths ?project ts_direct { lib_mode = mode; melange_emit = false })
+    in


Let's share the code for the two definitions.

nojb · 2024-06-20T06:24:21Z

src/dune_rules/lib_flags.ml

+  let to_hflags dirs =
+    Command.Args.S
+      (Path.Set.fold dirs ~init:[] ~f:(fun dir acc ->
+         Command.Args.Path dir :: A "-H" :: acc)
+       |> List.rev)
+  ;;
+


Let's share the code with to_iflags above.

nojb · 2024-06-20T06:28:40Z

src/dune_rules/compilation_context.ml

+    ~project
+    ~opaque
+    ~direct_requires
+    ~(hidden_requires : Lib_flags.L.t Resolve.Memo.t)


Suggested change

~(hidden_requires : Lib_flags.L.t Resolve.Memo.t)

~hidden_requires

Signed-off-by: HasanA <[email protected]>

Leonidas-from-XIV · 2024-06-21T13:05:41Z

src/dune_rules/lib_flags.ml

@@ -85,10 +85,10 @@ let link_deps sctx t mode =
 module L = struct
  type nonrec t = Lib.t list

-  let to_iflags dirs =
+  let to_iflags ?(flag = "-I") dirs =


In our codebase we often try to avoid default flags, as the defaults are often easy to miss and in this case "to_iflags" which generates -H flags is somewhat strange.

As such I suggest making this a labelled to_flag function and

let to_ifags = to_flags "-I" let to_hflags = to_flags "-H"

This prevents having to change all the code while allowing an easy way to get an -H flag.

Hello, thanks for the remark. You are right, it is even safer to do as you suggested. I have pushed the edits.

You might not even need to expose to_hflags in the mli if nobody outside the module is going to use it. That way the compiler can warn about unused functions in case the code stops using to_hflags.

You are right for now to_hflags is not used anywhere outside lib_flags.mli, but I exposed it for the sake of symmetry (since if to_iflags is to be used somewhere in some later work, to_hflags would be also needed).

But maybe it is better for now to remove it from the interface.

Signed-off-by: HasanA <[email protected]>

nojb · 2024-06-24T14:26:34Z

src/dune_rules/compilation_context.ml

  in
-  let sandbox = Sandbox_config.no_special_requirements in


Is this change needed?

nojb · 2024-06-24T14:27:34Z

src/dune_rules/compilation_context.ml

+        let open Resolve.Memo.O in
+        let+ requires = requires_compile
+        and+ requires_link = Memo.Lazy.force requires_link in
+        let requires_table = Table.create (module Lib) 500 in


500 seems too large; I suggest 5.

nojb · 2024-06-24T14:28:33Z

doc/changes/10644.md

+- Add support for the -H flag (introduced in Ocaml compiler 5.2) in dune
+  (only supported for lang versions 3.17 and higher). This  adaptation gives
+  the correct semantics for `implicit_transitive_deps = false` mode, since then
+  transitive dependencies are included with -H flag and so are not directly accessible
+  but visible for the compiler. (#10644, fixes (#9333, #ocsigen/tyxml#274, #2733, #4963),
+  @MA0100)


Suggested change

- Add support for the -H flag (introduced in Ocaml compiler 5.2) in dune

(only supported for lang versions 3.17 and higher). This adaptation gives

the correct semantics for `implicit_transitive_deps = false` mode, since then

transitive dependencies are included with -H flag and so are not directly accessible

but visible for the compiler. (#10644, fixes (#9333, #ocsigen/tyxml#274, #2733, #4963),

@MA0100)

- Add support for the -H flag (introduced in OCaml compiler 5.2) in dune

(only supported for lang versions 3.17 and higher). This adaptation gives

the correct semantics for `(implicit_transitive_deps false)`.

(#10644, fixes #9333, ocsigen/tyxml#274, #2733, #4963, @MA0100)

nojb · 2024-06-24T14:46:18Z

src/dune_rules/merlin/ocaml_index.ml

-        let* req_link = Compilation_context.requires_link cctx in
-        let+ req_compile = Compilation_context.requires_compile cctx in
-        List.filter req_link ~f:(fun l -> not (List.exists req_compile ~f:(Lib.equal l)))
+        Compilation_context.requires_hidden cctx


This doesn't seem quite right. For new versions of OCaml (>= 5.2) and version of the Dune language >= 3.17, Dune will pass -H to the compiler and these flags will be recorded in the .cmt files and will, in turn, be read by ocaml-index. Accordingly, in these cases we do not need to do anything special here. It is only for backwards compatibility that we can keep the above logic (ie if OCaml is < 5.2 or if the Dune language veresion if < 3.17). Also, the flags should all be passed with -I, not -H in this case.

@voodoos sounds OK?

Yes, that's completely right. If Dune >= 3.17 there is no more additional flags to pass to the indexer. (Note that indexing is not available prior to OCaml 5.2, so there is no need to check that here.)

Thanks for the confirmation! So only the check for OCaml lang version < 3.17 is needed.

Thanks, the current version looks good to me

nojb · 2024-06-24T14:48:49Z

Friendly ping @anmonteiro: are you planning to review the PR? I feel this is getting ready to be merged, so do not hesitate to leave a note if you are planning to do so (which would be appreciated, of course!).

Signed-off-by: HasanA <[email protected]>

nojb · 2024-06-27T12:33:52Z

src/dune_rules/compilation_context.ml

+        let () =
+          List.iter ~f:(fun lib -> Table.set requires_table lib ()) requires_compile
+        in


Suggested change

let () =

List.iter ~f:(fun lib -> Table.set requires_table lib ()) requires_compile

in

List.iter ~f:(fun lib -> Table.set requires_table lib ()) requires_compile;

nojb · 2024-06-27T12:36:00Z

test/blackbox-tests/test-cases/hidden-deps-unsupported.t/dune

+(executable
+ (name run)
+ (modules run)
+ (libraries bar))


Missing newline at EOF.

nojb · 2024-06-27T12:36:10Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/tyxml/run.ml

@@ -0,0 +1,2 @@
+open Tyxml.Html
+let _ = p [ a [ txt "a" ] ]


Missing newline at EOF.

nojb · 2024-06-27T12:36:21Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/tyxml/dune

@@ -0,0 +1,3 @@
+(executable
+  (name run)
+  (libraries tyxml))


Missing newline at EOF.

nojb · 2024-06-27T12:36:30Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.ml

@@ -0,0 +1 @@
+let _ = Bar.y


Missing newline at EOF.

nojb · 2024-06-27T12:41:30Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.t

+
+

Suggested change

nojb · 2024-06-27T12:41:38Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.t

+  > EOF
+  $ echo "$(getincludes I)"
+  .foo.objs/byte.foo.objs/byte.foo.objs/native
+


Suggested change

nojb · 2024-06-27T12:41:56Z

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.t

+  $ dune build --root=./tyxml
+  Entering directory 'tyxml'
+  Leaving directory 'tyxml'
+


Suggested change

nojb · 2024-06-27T12:42:11Z

test/blackbox-tests/test-cases/hidden-deps-unsupported.t/run.t

+
+
+


Suggested change

nojb · 2024-06-27T12:42:17Z

test/blackbox-tests/test-cases/hidden-deps-unsupported.t/run.t

+  > EOF
+  $ echo "$(getincludes I)"
+  .foo.objs/byte.foo.objs/byte.foo.objs/native
+


Suggested change

nojb · 2024-06-27T12:44:25Z

@MA0010 I left a few cosmetic tweaks to do before merging, otherwise this is almost ready.

I would put both tests under a common directory hidden-deps.t though.

Signed-off-by: HasanA <[email protected]>

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Small test cleanup

Signed-off-by: HasanA <[email protected]>

nojb · 2024-06-27T13:50:46Z

I think all issues have been addressed and the PR is ready to go.

@emillon: would you like to do the honours? (as I have been supervising the PR, I prefer not to merge it myself).

nojb · 2024-06-27T13:51:02Z

I think all issues have been addressed and the PR is ready to go.

@emillon: would you like to do the honours? (as I have been supervising the PR, I prefer not to merge it myself).

Tha PR should be squash-merged by the way.

Signed-off-by: HasanA <[email protected]>

emillon · 2024-06-28T07:13:40Z

@nojb yes, I'll give it a last look, squash and merge.

doc/changes/10644.md

emillon · 2024-06-28T07:39:50Z

I can't push to the original branch but that should be fine.

nojb · 2024-06-28T07:57:15Z

I can't push to the original branch but that should be fine.

I think this was my fault (I opened the PR from a branch in @MA0010's fork which disables this mechanism).

@MA0010 can you fix the remaining issue so that we can merge? Thanks!

emillon · 2024-06-28T08:04:41Z

the last issues are just cosmetic - newlines etc and the changelog can be fixed at release time.
Thanks a lot to everyone involved here. This is the consequence of a multi-year effort that started in dune 1.7 (5 years ago), made its way to the compiler, and back here!

kit-ty-kate · 2024-07-01T11:23:48Z

Thank you so much for this work!

MA0010 force-pushed the hidden_deps branch from 1930b3a to 374892d Compare June 11, 2024 16:02

MA0010 mentioned this pull request Jun 12, 2024

Bump dune lang version to 3.17 + add unix deps to vendor #10645

Merged

rgrinberg reviewed Jun 13, 2024

View reviewed changes

anmonteiro self-requested a review June 13, 2024 21:46

MA0010 force-pushed the hidden_deps branch 2 times, most recently from 514d4f3 to af51c9a Compare June 14, 2024 14:48

voodoos reviewed Jun 17, 2024

View reviewed changes

MA0010 mentioned this pull request Jun 17, 2024

Fix generating sites modules failure when implicit_transitive_deps set to false #10650

Merged

MA0010 added 5 commits June 18, 2024 15:46

Add support for -H

a4a6bf9

Signed-off-by: HasanA <[email protected]>

Add tests

663ec11

Signed-off-by: HasanA <[email protected]>

fix: requires_compile returns the correct deps

42f1798

Signed-off-by: HasanA <[email protected]>

add requires_hidden list + adpat lib_flags to use the new list

1bf8bf8

Signed-off-by: HasanA <[email protected]>

removed Hashtbl from lib.ml + ocaml_index.ml adapted to requires_hidd…

1893536

…en (to be re-checked) Signed-off-by: HasanA <[email protected]>

MA0010 force-pushed the hidden_deps branch from af51c9a to 1893536 Compare June 18, 2024 13:57

rgrinberg reviewed Jun 19, 2024

View reviewed changes

nojb commented Jun 20, 2024

View reviewed changes

some code cleaning

0c07a77

Signed-off-by: HasanA <[email protected]>

Leonidas-from-XIV reviewed Jun 21, 2024

View reviewed changes

MA0010 added 2 commits June 21, 2024 16:46

add to_flags function

366ca51

Signed-off-by: HasanA <[email protected]>

add CHANGES file

d9df9e4

Signed-off-by: HasanA <[email protected]>

nojb commented Jun 24, 2024

View reviewed changes

MA0010 added 2 commits June 24, 2024 16:51

Merge remote-tracking branch 'upstream/main' into hidden_deps

9fae1f7

Suggested edits

3607a91

Signed-off-by: HasanA <[email protected]>

nojb commented Jun 27, 2024

View reviewed changes

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.ml Outdated

@@ -0,0 +1 @@

let _ = Bar.y

Copy link

Collaborator Author

nojb Jun 27, 2024

Choose a reason for hiding this comment

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

Missing newline at EOF.

nojb commented Jun 27, 2024

View reviewed changes

test/blackbox-tests/test-cases/hidden-deps-supported.t/run.t Outdated

Comment on lines 23 to 24

Copy link

Collaborator Author

nojb Jun 27, 2024

Choose a reason for hiding this comment

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

Suggested change

nojb commented Jun 27, 2024

View reviewed changes

test/blackbox-tests/test-cases/hidden-deps-unsupported.t/run.t Outdated

Comment on lines 23 to 25

Copy link

Collaborator Author

nojb Jun 27, 2024

Choose a reason for hiding this comment

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

Suggested change

nojb commented Jun 27, 2024

View reviewed changes

MA0010 and others added 6 commits June 27, 2024 15:06

cosmetic tweaks

51d6e2f

Signed-off-by: HasanA <[email protected]>

Cleanup

83bb651

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Cleanup

2812ddb

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Merge pull request #1 from nojb/hidden-deps-simplify

80a1c02

Small test cleanup

Merge remote-tracking branch 'upstream/main' into hidden_deps

9d8c029

tweaks

c11722d

Signed-off-by: HasanA <[email protected]>

final formatting

f1882cc

Signed-off-by: HasanA <[email protected]>

emillon reviewed Jun 28, 2024

View reviewed changes

doc/changes/10644.md Show resolved Hide resolved

emillon merged commit d35fc98 into ocaml:main Jun 28, 2024
28 checks passed

nojb mentioned this pull request Jun 28, 2024

Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

Merged

MA0010 deleted the hidden_deps branch July 10, 2024 16:09

nojb mentioned this pull request Aug 6, 2024

Add a -H flag, second attempt ocaml/ocaml#12246

Merged

	~(hidden_requires : Lib_flags.L.t Resolve.Memo.t)
	~hidden_requires

Add support for -H compiler flag #10644

Add support for -H compiler flag #10644

Conversation

nojb commented Jun 11, 2024

emillon commented Jun 11, 2024

MA0010 commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Jun 13, 2024

voodoos left a comment • edited Loading

Choose a reason for hiding this comment

MA0010 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Jun 27, 2024

nojb commented Jun 27, 2024

nojb commented Jun 27, 2024

emillon commented Jun 28, 2024

emillon commented Jun 28, 2024

nojb commented Jun 28, 2024

emillon commented Jun 28, 2024

kit-ty-kate commented Jul 1, 2024

voodoos left a comment •

edited

Loading

nojb Jun 24, 2024 •

edited

Loading