Issues with the (system ...) pre-processing action

[kit-ty-kate]

While testing the dune cache system, I encountered an issue with elpi in the forme of the following error message:

-     ocamldep src/.elpi.objs/parser.pp.mli.d (exit 2)
- (cd _build/default && /home/opam/.opam/4.10.0+trunk/bin/ocamldep.opt -modules -intf src/parser.pp.mli) > _build/default/src/.elpi.objs/parser.pp.mli.d
- >> Fatal error: OCaml and preprocessor have incompatible versions
- Fatal error: exception Misc.Fatal_error
-     ocamldep src/.elpi.objs/parser.pp.ml.d (exit 2)
- (cd _build/default && /home/opam/.opam/4.10.0+trunk/bin/ocamldep.opt -modules -impl src/parser.pp.ml) > _build/default/src/.elpi.objs/parser.pp.ml.d
- >> Fatal error: OCaml and preprocessor have incompatible versions
- Fatal error: exception Misc.Fatal_error
-       ocamlc src/.elpi.objs/byte/elpi__Builtin.{cmo,cmt}

The issue here is that they use the (system ...) command to pre-process one of the module using camlp5: https://github.com/LPCIC/elpi/blob/e0492dbf7196861c2afbb72fb05ab2774c79f965/src/dune#L8

The issue also happens if we build the project under one opam switch and then again with another one. For instance:

$ opam switch 4.08
$ dune build
$ opam switch 4.09
$ dune build
[same error message as above]

The issue is here that the (system ...) function is opaque for dune and no information about the command used is known to dune so the parser module will be reused when it should not.

One way to fix that would be to define a proper rule for this module, then dune would know that it depends on camlp5 and would know when to not reuse the same output.

This works, however I'm creating this issue because I think the (system ...) command should be better advertised as being potentially unsafe. Maybe using a warning, renaming it to (unsafe_system ...) or something like that or even simply remove it in some way or another to force the users to use proper rules.

Specifications

• Version of dune (output of dune --version): 2.1.3

[rgrinberg]

This works, however I'm creating this issue because I think the (system ...) command should be better advertised as being potentially unsafe. Maybe using a warning, renaming it to (unsafe_system ...) or something like that or even simply remove it in some way or another to force the users to use proper rules.

system is just as unsafe as bash in this context. I think unsafe is the wrong to characterize this however. The contract for these general actions is that the user must specify targets and dependencies manually & we've added a sandbox mode to ensure that this is indeed the case. I would suggest that we start encouraging users to make sure their rules work with sandboxing whenever possible.

[emillon]

The action in question is

(action (system "camlp5o -I . -I +camlp5 pa_extend.cmo pa_lexer.cmo %{input-file}"))

To dune, this is just an opaque string (where it will interpolate %{input-file}). It does now know that this uses camlp5o. The right fix in that case is to use (run) which declares a dependency to the executable, and also for a better solution turn pa_extend.cmo etc to %{dep:pa_extend.cmo} if they are part of the build directory.