Add link_deps field

[emillon]

Hi!

Description

In some cases, the linking step requires some dependencies (see #852). For example, passing a version script to the linker. The new (link_deps) field uses the dependency DSL already used in other places.

Questions

While working on this PR with @NathanReb we noted some questions about the implementation:

• should link_deps be an extra argument, or should it be embedded into link_flags? (in the latter case, maybe we can rename the combined argument).
• is exe the correct place to put this, or should it be on Buildable.t? (it seems that libraries are handled completely by the OCaml toolchain, so there are no external dependencies)
• as a followup for a next PR, how can we add support for the %{dep:path} syntax? it seems that the magic to do that is in Super_context.Action.expand_step1, but this would apply to all places where the action syntax is supported. Is this what you meant?

Thanks!

[ghost]

should link_deps be an extra argument, or should it be embedded into link_flags? (in the latter case, maybe we can rename the combined argument).

What would the combined argument look like? Is it the idea of writing (-ccopt "-Wl,--version-script %{dep:version.map}")? In general, we need both the ability to specify dependencies directly on the command line because it is convenient and on the side because some dependencies don't appear on the command line.

is exe the correct place to put this, or should it be on Buildable.t? (it seems that libraries are handled completely by the OCaml toolchain, so there are no external dependencies)

We do allow custom link flags for libraries, and some of them are passed to the C compiler, so I guess it would make sense as well to have link dependencies for libraries. Although it's probably much less common. The fields for libraries are named differently as well:

• library_flags: passed to ocamlc/ocamlopt
• c_library_flags: passed to ocamlmklib as it and to ocamlc/ocamlopt wrapped with -cclib

So I guess the name for link_deps would need to be consistent with these, or these should be renamed.

as a followup for a next PR, how can we add support for the %{dep:path} syntax? it seems that the magic to do that is in Super_context.Action.expand_step1, but this would apply to all places where the action syntax is supported. Is this what you meant?

Yes. This code would need to be refactored so that we can use it for flag lists as well.

[emillon]

What would the combined argument look like?

Sorry, that wasn't very clear but I was referring to the arguments passed into Exe.build_and_link_many - not the jbuild syntax.

Instead of adding an extra argument to that function (and other ones in Exe), it's possible to embed these dependencies into the Build.t value passed as link_flags in Gen_rules:

~link_flags:(SC.Deps.interpret sctx ~scope ~dir exes.link_deps >>^ ignore >>> link_flags)

Would that be preferable?

[ghost]

Ah, I see. Yes that makes sense. In fact, we could allow to use the special syntax ${^} (which will become %{deps}) in flags, at which point combining them makes even more sense. So combining them now seems good.

[rgrinberg]

@emillon would you mind updating CHANGES and the manual btw?

[emillon]

• Rebased & fixed the tests (now that the compat symlinks are gone)
• Updated CHANGES
• The field is documented in jbuild.rst, is there another place to document it?
• Is it OK to extend this to libraries in a further PR?

[ghost]

You need to add a dune-project file to the test as well (this is needed since #874). The rest looks good.

The field is documented in jbuild.rst, is there another place to document it?

Nope, that's the only place

Is it OK to extend this to libraries in a further PR?

Sure

[emillon]

Oops, forgot to git add it. Should be 👍 now.

[ghost]

Looks good

[emillon]

Thanks!