Is it possible to remove the .cppo extensions?

[aantron]

We currently have two files named src/unix/lwt_unix.cppo.ml and src/unix/lwt_unix.cppo.mli, because adding the .cppo extension was the traditional way to identify files that need preprocessing to the cppo OCamlbuild plugin. However, this bit in the jbuilder docs suggests that it is not necessary anymore.

I think it would be nice to get rid of the extension. It would have kept the history tidier if we hadn't had to introduce it in the first place. So, this is just a low-priority issue is just about tidiness in filenames and git history.

cc @andrewray for any info :)

[andrewray]

I think my reading of the docs is the same as yours, however, the pps stanza would presumably apply to the whole (unix) library, whereas currently, it is specifically applied to only the required files (probably a very tiny, and likely unnoticeable, build performance issue).

[aantron]

Ok, if that's the case, maybe it's better to leave it alone for now.

On a bit of a tangent, I had some difficulty combining both a PPX and cppo with a pps stanza (or stanzas) in a toy example. I didn't really exhaust all possible ways to combine them. Do you have any experience with this?

We may need this to port coverage to the new Jbuilder (which I am working on now by finishing off the port of Bisect).

[andrewray]

I suspect the pps stanza is just about flexible enough to support a simple project with cppo, but is really all about efficiently applying a dozen ppx's at a time.

I would keep up with the push to get Bisect as a first class citizen in the jbuilder world. Having used Bisect (both old and new) a few times, I find it really useful.

[aantron]

Ok, it is possible to both apply a PPX and preprocess with cppo, but it requires the cppo rule stanza, and so presumably a source file with a name like foo.cppo.ml. It would be nice if one could both apply cppo and PPXs to source named foo.ml. Maybe it's already possible using multiple rule stanzas, or a single complex one, but I doubt it, and it doesn't seem possible using the preprocess stanza.

cc @diml. This is not a high priority, but it would make files like lwt_unix.cppo.ml, where we both want to use cppo, and apply Bisect_ppx, ever so slightly more maintainable.

[ghost]

It's not hard to support as jbuilder already uses intermediate files to handle preprocessing. We just need to support chains of preprocessors. Can you open a ticket on jbuilder so that we keep track of this?

[andrewray]

@antron @diml I have been testing this by trying to add bisect_ppx to lwt_unix. Did you actually manage to get cppo and a ppx to work here? It seems the cppo rule is not triggered before the ppx is applied, and thus the build fails.

[aantron]

@andrewray Indeed, I remember seeing what you are reporting as well, i.e. PPX triggered before the cppo rule, and the build completely failing. That's why I applied Bisect to only the core in my crappy WIP PR :p

However, I tried this more recently:

foo.cppo.ml:

#define FOO bar

let () =
  let p =
    let%lwt FOO = Lwt.return () in
    Lwt.return ()
  in
  ignore p;

  prerr_endline "foo"

jbuild:

(jbuild_version 1)

(rule
 ((targets (foo.ml))
  (deps    (foo.cppo.ml))
  (action  (run ${bin:cppo} ${<} -o ${@}))))

(executable
 ((name foo)
  (preprocess (pps (lwt.ppx)))
  (libraries (lwt))))

Build:

λ jbuilder clean
λ jbuilder build foo.exe && _build/default/foo.exe
        cppo foo.ml
    ocamlopt .ppx/lwt.ppx/ppx.exe
         ppx foo.pp.ml
    ocamldep foo.depends.ocamldep-output
      ocamlc foo.{cmi,cmo,cmt}
    ocamlopt foo.{cmx,o}
    ocamlopt foo.exe
foo

Since it seemed to work, I assumed that I had made some kind of mistake in the past. But, perhaps not.

Maybe the ordering of actions depends on some hidden factor, like filename, order of stanzas, or executable/library stanzas being used?

[ghost]

It might have been with an older version of jbuilder. Initially it was taking all files ending with .ml as modules. This was changed to not consider files with several dots, so that foo.cppo.ml is ignored.

[andrewray]

The problem exists with the dev version of jbuilder.

I've tried creating a simple example library with cppo/ppx but it works OK. Something subtle seems to be going on with lwt.unix.

[andrewray]

I've found the problem - cppo is adding a #line directive pointing to lwt_unix.cppo.ml. Due to the various changes of directory during the build, when the ppx runs it can't find lwt_unix.cppo.ml and errors out. Adding chdir ${ROOT} around the cppo call fixes the issue.

I'll prepare a pull request later.

[aantron]

when the ppx runs it can't find lwt_unix.cppo.ml and errors out.

Without looking deeply, this sounds like a Bisect problem. If so, I want to note that we are trying to move away from reading concrete syntax or dealing with #line (something we inherited from Camlp4 days, and haven't fully removed). I'm not sure what the target of the PR is/was going to be, but see also aantron/bisect_ppx#83 (comment).

[andrewray]

That would also fix it. This is what's needed in the mean time:

diff --git a/src/unix/jbuild b/src/unix/jbuild
index abdb384..bfe2756 100644
--- a/src/unix/jbuild
+++ b/src/unix/jbuild
@@ -5,12 +5,12 @@
 (rule
  ((targets (lwt_unix.ml))
   (deps    (lwt_unix.cppo.ml))
-  (action  (run ${bin:cppo} -V OCAML:${ocaml_version} ${<} -o ${@}))))
+  (action  (chdir ${ROOT} (run ${bin:cppo} -V OCAML:${ocaml_version} ${<} -o ${@})))))
 
 (rule
  ((targets (lwt_unix.mli))
   (deps    (lwt_unix.cppo.mli))
-  (action  (run ${bin:cppo} -V OCAML:${ocaml_version} ${<} -o ${@}))))
+  (action  (chdir ${ROOT} (run ${bin:cppo} -V OCAML:${ocaml_version} ${<} -o ${@})))))
 
 ;; lwt feature discovery
 ;;
@@ -129,4 +129,5 @@
   ))
   (c_flags (:include unix_c_flags.sexp))
   (c_library_flags (:include unix_c_library_flags.sexp))
+  (preprocess (pps (?bisect_ppx)))
 ))

[aantron]

That doesn't look bad. We'll probably have to (EDIT: temporarily) use it, because addressing it in Bisect_ppx could take a release or two.

[aantron]

Closing this, as we'll probably learn from the Jbuilder changelog when this is possible.