A request for dynamically defined worktrees

trefis commented 5 years ago

In merlin, we want to support several versions of OCaml, and up to now we've done that using a conjunction of (copy_files ..) and (rule (action (copy ...)) ...)`.

Our source tree looks something like this:

src
|-- foo
|-- ocaml
|   |-- parsing
|   |   |-- 402
|   |   |   |-- ...
|   |   |-- ...
|   |   `-- dune
|   |-- typing
|   |   |-- 402
|   |   |   |-- ...
|   |   |-- ...
|   |-- preprocess
|   |   |-- 402
|   |   |   |-- dune
|   |   |   |-- lexer_raw.mli
|   |   |   |-- lexer_raw.mll
|   |   |   `-- parser_raw.mly
.   .   .
.   .   .
.   .   .
|   |   |-- 407
|   |   |   |-- dune
|   |   |   |-- lexer_raw.mli
|   |   |   |-- lexer_raw.mll
|   |   |   `-- parser_raw.mly
|   |   `-- dune
...

And you can imagine that in each subdir foo of src/ocaml/ the dune looks somewhat like this:

(* -*- tuareg -*- *)

module J = Jbuild_plugin.V1

let ver =
  Scanf.sscanf J.ocaml_version "%s@.%s@." (fun maj min -> maj ^ min)
;;

Printf.ksprintf J.send {|
(copy_files# %s/*.ml{,i})

(library
 (name foo)
 (libraries bar baz))
|} ver

Problem 1

This works reasonably well, except for the preprocess directory where we have one extra constraint: we want to stage the output of menhir, because we're calling it with the --cmly flag whose output is not stable (i.e. different versions of menhir will not necessarily generate the same thing).

As a result, I cannot just add the following to src/preprocess/dune:

(copy_files <the version_selected>/parser_raw.mly)

(menhir
 (modules parser_raw)
 (flags :standard --cmly))

I want to stick a (mode promote) field in that menhir stanza, and as a result I have to move it to the subdir.

Problem 2

I can't just move the stanza to the subdir and copy the generated files to the parent, because:

Error: I can't determine what library/executable the files produced by this stanza are part of.

So I have to give up the use of (copy_files ...) and use (include_subdirs ...) instead.

Problem 3

Since I only want to include one of the subdirs, I thought I'd have this in src/ocaml/preprocess/dune:

(* -*- tuareg -*- *)

module J = Jbuild_plugin.V1

let ver =
  Scanf.sscanf J.ocaml_version "%s@.%s@." (fun maj min -> maj ^ min)
;;

Printf.ksprintf J.send {|
(dirs %s)
(include_subdirs unqualified)

(library
 (name preprocess)
 (libraries parsing))
|} ver

Unfortunately:

File "_build/.dune/default/src/ocaml/preprocess/dune", line 2, characters 1-5:
2 | (dirs 407)
     ^^^^
Error: Unknown constructor dirs

That is: (dirs ...) is interpreted too early, and isn't recognized in generated dune files. Apparently the same limitation exists with the deprecated (ignored_subdirs ...).

Since that can't be used, perhaps I should just stop the recursion using (include_subdirs no) in the subdirectories I don't want.

Problem 4

At this point, once all the dune files have been generated, I have the following dune files (assuming I'm building on 4.07):

src/ocaml/preprocess/dune:

(include_subdirs unqualified)

(library
 (name preprocess)
 (libraries parsing))

src/ocaml/preprocess/406/dune:

(include_subdirs no)
(menhir
 (mode promote)
 (modules parser_raw)
 (flags :standard --inspection --table --cmly))

src/ocaml/preprocess/407/dune:


(menhir
 (mode promote)
 (modules parser_raw)
 (flags :standard --inspection --table --cmly))

But when I run dune build, I end up with:

File "src/ocaml/preprocess/406/dune", line 1, characters 0-0:
Warning: File parser_raw.ml is both generated by a rule and present in the source tree.
As a result, the rule is currently ignored, however this will become an error in the future.
Delete file src/ocaml/preprocess/406/parser_raw.ml to get rid of this warning.
File "src/ocaml/preprocess/406/dune", line 1, characters 0-0:
Warning: File parser_raw.mli is both generated by a rule and present in the source tree.
As a result, the rule is currently ignored, however this will become an error in the future.
Delete file src/ocaml/preprocess/406/parser_raw.mli to get rid of this warning.

Which makes me think that something's wrong. Perhaps I should just use library variants?

Problem 5

I can't. The .mli is generated by menhir.

Problem 6

What we decided to do, is to not use the menhir stanza, and instead rely on a hand-written invocation of menhir, and some copy_files stanzas. With the promotion happening when building a particular alias.

And we ended up with something like this:

Which actually broke silently a while back, because dune changed where it puts .cmi files. That clearly doesn't seem like the way to go.

I just need something to restrict which subdirectories are included / excluded based on a dynamic check (here the version of caml, but it could also have been the ast_magic_number, or something like that).

It would make things much nicer, and simpler.

ghost commented 5 years ago

If we added a mode (promote_into <dir>) that would be the same as promote but would copy the files in <dir> rather than the current directory, would that completely solve the problem?

trefis commented 5 years ago

So you're suggesting:

src
|-- foo
|-- ocaml
|   |-- preprocess
|   |   |-- 402
|   |   |   |-- lexer_raw.mli
|   |   |   |-- lexer_raw.mll
|   |   |   |-- parser_raw.ml
|   |   |   |-- parser_raw.mli
|   |   |   `-- parser_raw.mly
.   .   .
.   .   .
|   |   |-- 407
|   |   |   |-- lexer_raw.mli
|   |   |   |-- lexer_raw.mll
|   |   |   |-- parser_raw.ml
|   |   |   |-- parser_raw.mli
|   |   |   `-- parser_raw.mly
|   |   `-- dune
...

With the dune file containing:

(copy_files# <version>/*.ml{,i,y})

(menhir
  (modules parser_raw)
  (mode (promote_into <version>))
  (flags ...))

That feels a bit icky, but I guess it would work. Is it possible to make it so that the line directives in parser_raw.ml point to src/ocaml/preprocess/version/parser_raw.mly.

ghost commented 5 years ago

Is it possible to make it so that the line directives in parser_raw.ml point to src/ocaml/preprocess/version/parser_raw.mly.

Isn't it the case already?

trefis commented 5 years ago

Ah, the line directives are relative, so I guess that would work indeed.

ghost commented 5 years ago

Ok, I'm favour of adding support for (promote_into ...) then. It's the simplest solution to implement. Dynamically defined file trees would be good to have and have been requested for other cases, though it's a much more invasive change so it's likely to take more time to implement.

trefis commented 5 years ago

Fine by me. Obviously it would be greatly appreciated if that was implemented rather soon. Everyone always wants things ASAP, but the reason I started looking at all this again is because I'm going to have to write a 408/ subdir in there. And I'd rather not have to go and hunt for the .cmi files in weird hidden subdirectories of _build/.

rgrinberg commented 5 years ago

I can't. The .mli is generated by menhir.

Why is that a limitation? virtual_modules should still accept modules which are generated. Or do you mean to say, it's not possible to reasonably write an implementation for this module?

rgrinberg commented 5 years ago

The situation with the cmi files is clearly not sustainable. My plan was to add a form like this:

%{lib:mylib:public_intfs}

So that it's possible to select all the public cmi's for a particular library. However, what you're looking for seems to be more fine grained.

Is it possible for you organize stuff into sub libraries to make the above work? Or do you really need to select the cmi's based on a set of modules in a directory?

trefis commented 5 years ago

Why is that a limitation? virtual_modules should still accept modules which are generated. Or do you mean to say, it's not possible to reasonably write an implementation for this module?

I understood https://dune.readthedocs.io/en/latest/variants.html#virtual-library as "you have to have an .mli for a virtual module". And in my case, both the .ml and the .mli would be generated in the variant, i.e. I have neither when defining the virtual library.

So that it's possible to select all the public cmi's for a particular library. However, what you're looking for seems to be more fine grained. Is it possible for you organize stuff into sub libraries to make the above work? Or do you really need to select the cmi's based on a set of modules in a directory?

IIUC you're suggesting I keep the hand written invocation of menhir, but that I replace the -I somedir that one can see here. By ... I'm not sure what, it looks like your variable would expand to a list of cmi files, which I have no way to feed to menhir. If it just expands to a directory name however, then -I %{lib:parsing:public_intfs} would be fine. As it turns out, I've already organised things so that the directories I'm adding to the include path already are in a 1-to-1 mapping with libraries.

ghost commented 5 years ago

Expanding to a directory wouldn't work as dune wouldn't know that menhir is going to read the cmi files inside this directory. For instance it might end up running menhir before the cmi files are produced.

promote_into still seem like the simplest and most viable solution to me here. In particular, when we have the choice between a solution that involves accessing cm* files and a solution that doesn't, as a rule of thumb we should always choose the latter.

rgrinberg commented 5 years ago

I understood dune.readthedocs.io/en/latest/variants.html#virtual-library as "you have to have an .mli for a virtual module". And in my case, both the .ml and the .mli would be generated in the variant, i.e. I have neither when defining the virtual library.

But if every library has its own interface, you need to have some sort of conditional compilation down the pipeline as well? I guess the only way to make this work with variants is to have some unified library that abstracts away the different OCaml frontends.

IIUC you're suggesting I keep the hand written invocation of menhir, but that I replace the -I somedir that one can see here. By ... I'm not sure what, it looks like your variable would expand to a list of cmi files, which I have no way to feed to menhir. If it just expands to a directory name however, then -I %{lib:parsing:public_intfs} would be fine. As it turns out, I've already organised things so that the directories I'm adding to the include path already are in a 1-to-1 mapping with libraries.

Yeah, that's what I was suggesting. Expanding to just a directory won't work because dune won't record the dependencies which is annoying. Back to the drawing board.

ocaml / dune