Dune: Don't introduce dependency in case of using `copy` from DSL for building stubs.

[XVilka]

For example if you have something like

(library
 (name mylib_c)
 (public_name mylib.c)
 (modules)
 (self_stubs_build_archive mylib_c_stubs)

(rule
 (deps (source_tree c))
 (targets libmylib_c_stubs.a dllmylib_c_stubs.so)
 (action (progn
          (chdir c (run make)))
          (copy c/libmylib.a libmylib_c_stubs.a)
          (copy c/libmylib.so dllmylib_c_stubs.so)))

then it introduces dependency to mylib/c/libmylib.a
See https://discuss.ocaml.org/t/dune-depend-on-non-dune-libraries-in-the-workspace/3150/7 and https://discuss.ocaml.org/t/strange-error-in-dune-building/2979/3 for more context.

Opening an issue, since I wasn't able to find anything in the issue tracker about this. Would be awesome to fix this one day and we have to remove make -C c before calling dune in our buildscripts.

[rgrinberg]

I'm still not quite sure what the issue here is or what is the feature request. It seems clear to me that (copy c/mylib.a ...) should introduce a dependency on c/mylib.a. In the above example, the make step should just be a separate rule that creates those sources for the copy.

Is there any reason why 2 rules aren't acceptable here?

[jeremiedimino]

It's probably because the c directory is a data only directory. That's how most of such foreign sandboxed builds are setup. The current workaround is to use a shell script to call make + copy the files. It's not that big of a deal given that make requires a shell anyway.

[ygrek]

this is quite confusing behaviour and it is not very well documented anywhere in the manual? In particular "dealing with foreign libraries" example leads to this issue but doesn't mention it.

[roddyyaga]

This still seems to be an issue -- as far as I can tell this section of the manual is still wrong. I think the copy commands should be run cp or similar.

[jeremiedimino]

We should introduce a (no_inference) field to disable dependencies/targets inference. This should be easy, if anyone is interested we can give some pointers for how to do it.

[roddyyaga]

@jeremiedimino I'm happy to try! Not familiar with the dune codebase, where should I be looking?

[jeremiedimino]

Great!

So while writing up some pointers, I just remembered that there are quite a few places in the language where we take an action and automatically infer dependencies. Rather than add (no_inference) fields everywhere, it seems more general to add a (no_infer <action>) action.

To do that, you need to add the corresponding constructor in src/dune/action_intf.ml. You then need to add a parser for it in action_ast.ml in the decode value in the Make functor. You can then let type errors show you various places to update. In most places, the code to write should be straightforward. The only interesting place is in the Infer sub-module in action_unexpanded.ml, where you want to cut the recursion when you encounter this constructor and just return the accumulator.

Once this is done, you just need to add a test for this as described here and add documentation for the new action in doc/concepts.rst, in the User actions section.

Let us know if you need anything else!

[snowleopard]

In a parallel discussion, we came up with another possible solution, which is more difficult to implement. The solution is to make it possible to declare dependencies and targets for run actions, e.g. to allow the following syntax:

(rule
 (deps (source_tree c))
 (action (progn
          (chdir c (run make (targets c/libmylib.a c/libmylib.so)))
          (copy c/libmylib.a libmylib_c_stubs.a)
          (copy c/libmylib.so dllmylib_c_stubs.so)))

To make this work, inference should correctly handle intermediate files, such as c/libmylib.a, so that they are not considered dependencies of the whole action.

This has a disadvantage that the syntax of the action DSL becomes more complex. On the other hand, having additional deps and targets annotations on a run feels natural in the build system context.

In any case, a way to opt out of inference, such as no_infer, sounds useful too, so it's fine as a short-term solution.

[jeremiedimino]

3456 provides a solution for this