Open jberdine opened 3 years ago
Cc @voodoos who may know about this.
Yes it is a known issue and in fact it is already fixed on master
(and the 411
branch).
It will be part of the next release.
You can pin the following branch if you would like to try it beforehand: opam pin https://github.com/ocaml/merlin.git#411
Thanks, I will retest. So far in my full environment (which uses 4.12), opam pin add dune --dev
does not resolve this issue.
It is expected because the issue in on Merlin's side, not Dune's.
opam pin https://github.com/ocaml/merlin.git
should fix it for 4.12.
(for OCaml 4.11 use opam pin https://github.com/ocaml/merlin.git#411
)
Please tell me if that is effectively the case when you find the time to try 🙂
Ah, sorry for messing up the test.
I have just tried dune 2.8.0-395-g65404cf973 and ocamlmerlin v4.1-59-g0caac250d6 . The test case in the linked PR does now work as expected. There file not found error persists in my full environment, so there is something else going on that I need to figure out.
I will also try to reproduce.
When isolating you test from Dune's test tree and building it with dune 2.8.3
I do get the error using merlin 4.1-412
:
{"class":"return","value":[{"start":{"line":0,"col":-1},"end":{"line":0,"col":-1},"type":"typer","sub":[],"valid":true,"message":"I/O error: bar.ml: No such file or\ndirectory"}],"notifications":[],"timing":{"clock":61,"cpu":33,"query":1,"pp":0,"reader":4,"ppx":25,"typer":4,"error":0}}
However after pinning opam pin https://github.com/ocaml/merlin.git
I get the expected behaviour:
{"class":"return","value":[],"notifications":[],"timing":{"clock":60,"cpu":36,"query":1,"pp":0,"reader":4,"ppx":22,"typer":9,"error":0}}
I think the issue comes from the fact that you try to build your test from inside Dune's tree. Calling dune build
from that folder does nothing. To have a working test you need a run.t
file with instructions that will be played in a sandboxed environment when running the tests. (and it not really possible to test ocamlmerlin
behavior in Dune tests because of the sandboxing)
(I believe Cram tests folders are treated similarly to data_only
folders)
I pushed a commit to your branch with a working test that can be run using dune build @dune-merlin-ppx_expect
:
https://github.com/ocaml/dune/pull/4478/commits/6a3653874f782049f3ca8afec77efd2894de4366
It does work when Merlin master is installed, but not with current stable (so the CI is expected to fail).
@voodoos I updated the test case in https://github.com/ocaml/dune/pull/4478 so that it hits the file not found error with:
❯ dune --version
2.8.0-395-g65404cf973
and
❯ ocamlmerlin -version
The Merlin toolkit version v4.1-59-g0caac250d6, for Ocaml 4.12.0
The main point seems that using a subdir
stanza leads to the file not being found.
Indeed, this is an issue. Dune should pass more information to Merlin to handle these cases.
If I am not mistaken this is probably not a new issue, that is, not due to the new "interop", as Merlin has always been considering the ppx workdir
to be the directory in which the nearest .merlin
(and dune
file) resides.
There is a discussion on how to improve the current behaviour (which is known to be imperfect) in this Merlin issue: https://github.com/ocaml/merlin/issues/1292.
Your test is a good example of broken behaviour, thank you for taking the time to putting it in place, we will try to improve that in the next iterations of Dune.
Thanks for the confirmation. It seems that a number of things have to conspire to trigger this behavior.
I'm not sure whether or not to categorize this as a new issue. On one hand, with dune 2.7.1, this example works. On the other hand, that seems to be because the .merlin
file is generated in the lib
subdir even though the nearest enclosing dune
file is in the directory above, which makes the paths correct perhaps just by lucky coincidence. I wonder if there is a "quick fix" change in similar spirit to https://github.com/ocaml/merlin/pull/1284 that would set the workdir to where the .merlin
file used to be generated, which with subdir
stanzas is not necessarily where the nearest enclosing dune
file is.
Maybe this is more relevant to the linked merlin issue, but it seems clearly desirable for dune to pass enough information to merlin that merlin can replicate the ppx.exe invocation that dune uses for the "real" build. It does make me wonder if it might be more robust over the longer term to use a higher-level abstraction of e.g. having dune pass the full command to merlin, rather than merlin having to reconstruct it. This would be somewhat analogous to compilation databases.
Oh you are right, because dune generated the .merlin
in the subdir it was working before and this is a regression with the new interop. We definitively need to improve that.
Meanwhile, I just tried a silly (but easy) workaround: adding an empty dune
file to your subdir tricks Merlin into the correct behaviour. (with merlin master
that is pending release).
Reopening since there is still an issue to solve. We might open a new issue without all the debug history (or hide a few comments).
@voodoos any updates on this? It's not clear to me what still needs to be done for this issue.
Not sure any more, I will try to reproduce when I find the time. If I remind well there is still room for improvement in the way Merlin starts the ppxes (that might differ from what Dune does).
Expected Behavior
With the project in https://github.com/ocaml/dune/pull/4478, building with dune and then querying using ocamlmerlin succeeds:
Actual Behavior
With dune 2.7.1 and merlin 4.1, the actual behavior is as expected. But with later versions of dune, ocamlmerlin cannot find the source file:
Reproduction
Specifications
dune
(output ofdune --version
): 2.8.5ocaml
(output ofocamlc --version
): 4.11.1