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

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!

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.

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?

Let's share the code for the two definitions.

Let's share the code with to_iflags above.

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.

Is this change needed?

500 seems too large; I suggest 5.

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.

Done!

Thanks, the current version looks good to me

Missing newline at EOF.

Missing newline at EOF.

Missing newline at EOF.

Missing newline at EOF.