[RFC] Add support for building menhir in workspace

[nojb]

I am working on porting menhir to dune (there is initial agreement from the author, see https://github.com/nojb/menhir/tree/switch_to_dune if interested). It looks like we need a small change to dune so that the (menhir ...) stanza can work with a menhir which is in built out of the current workspace.

The situation is very similar to js_of_ocaml: when executed, menhir needs to find a "standard library" file (called standard.mly), but the location of this file is chosen at installation time and is then hard-coded in the executable so it won't work before the executable is installed. Luckily one can override the standard library directory on the command-line by passing a --stdlib option.

This PR makes it so that when there is a menhir package in the current workspace, menhir invocations add the necessary flag so that the standard library can be found.

Note that this is a theoretical point right now as there isn't a released menhir that can be built with dune yet (but this patch can be tested with the branch referenced above).

In any case, I do not want to merge this now because things may change with the switch to dune but rather to put it out there for discussion.

Thanks!

[rgrinberg]

I think we've encountered problems similar to this one in the past and Jeremie even proposed a general solution: #1185

I'm not against the solution proposed here as a temporary means, but I think that we should converge towards the relocation library eventually.

[ghost]

Agreed, having a general solution would be nice since this is a general problem. However, it will take some time to develop it, mature it and get it adopted. So the proposed PR seems fine indeed.

[ghost]

Regarding the implementation, what about systematically passing --stdlib <menhir-lib-dir> independently of whether menhir is part of the workspace or not? In particular, this would allow to consistently register the dependency on standard.mly just as we do for cmi files. You could use Artifacts.Public_libs.file_of_lib to locate standard.mly. That would work independently of whether menhir is installed or not.

[nojb]

Regarding the implementation, what about systematically passing --stdlib <menhir-lib-dir> independently of whether menhir is part of the workspace or not? In particular, this would allow to consistently register the dependency on standard.mly just as we do for cmi files. You could use Artifacts.Public_libs.file_of_lib to locate standard.mly. That would work independently of whether menhir is installed or not.

I thought about that, but there are some complications:

• If not being installed with opam/ocamlfind (eg Windows) the location of standard.mly can be arbitrary, and there is no easy way to find it out as far as I know. There was a similar problem with js_of_ocaml, treated in js_of_ocaml: look for runtime in the same dir as executable if ocamlfind pkg unavailable #1467.

• In fact there is no menhir library. When installed with opam the file standard.mly is installed inside the menhir package directory (which is otherwise empty).

Of course we could do what you suggest when it is possible (if a menhir package is found in the installed world), but we should probably still keep some kind of fallback for the non-opam situation.

[ghost]

Ok, if we still need the fallback let's not bother.

[ghost]

@nojb, this is ready to merge BTW :)

[nojb]

@nojb, this is ready to merge BTW :)

Ok, I'm merging this with the caveat that it may be tweaked before the dune version of menhir is released.

[ghost]

Ah, sorry I missed that point in your original comment! If you expect that things might change, then I agree that it's definitely a good idea to wait until it's ready :)

[trefis]

Somewhat related (not that much, but I didn't know where else to complain): the path to this standard.mly file appears in the generated .ml parser (in line directives).
And if you decide to "snapshot" your parser (as we do in merlin with a (promote ) rule) to not depend on menhir in release mode, then depending on whether it's you or your collaborator who commited the snapshot last, one of you will get noise on every build of the project.

This is actually very annoying.
One thing that could be done in menhir would be to add a subcommand (e.g. menhir stdlib-path) so that dune can find it, copy it somewhere under _build, and pass that directory to menhir with the --stdlib flag.
Does that sound crazy to you guys?

[nojb]

This is actually very annoying.

I agree!

One thing that could be done in menhir would be to add a subcommand (e.g. menhir stdlib-path) so that dune can find it, copy it somewhere under _build, and pass that directory to menhir with the --stdlib flag.
Does that sound crazy to you guys?

Adding an option to menhir to find out the location of standard.mly makes sense to me (it would also allow to record the dependency on it uniformly whether menhir is installed or in the workspace).

Copying it into the _build subdirectory seems a bit ad-hoc, but why not ?

[trefis]

Copying it into the _build subdirectory seems a bit ad-hoc, but why not ?

Sure, that last bit is less important. (I can make my own adhoc rule in dune files if the subcommand exists).

Ping @fpottier

[bobot]

Do you think #2483 could have been also a solution if menhir had an environment variable to replace the option --stdlib? Just to know.

[nojb]

Do you think #2483 could have been also a solution if menhir had an environment variable to replace the option --stdlib? Just to know.

I guess that if an environment variable was available, then indeed dune could set that when invoking menhir. As far as I can see, this is independent of #2483, which is about features exposed to the end user.

[rgrinberg]

This feature needs some tests and an appropriate change log entry. Also, shall we enable it only for a newer version of the menhir extension?

[nojb]

This feature needs some tests and an appropriate change log entry. Also, shall we enable it only for a newer version of the menhir extension?

I will try to add some tests. For the second question, I don't think it is worth versioning this change, since it applies only when menhir is built out of the workspace, which is not possible with any of the released versions of menhir.

[nojb]

For info, we ended up solving this problem in a different way (we embed the file in question directly inside the executable), so this PR is no longer needed. See https://gitlab.inria.fr/fpottier/menhir/merge_requests/10 for the details.

[nojb]

(by the way, this also fixes the problem reported by @trefis due to changing line directives)

[trefis]

Awesome, thanks @nojb!