Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merlin: communicate STDLIB directive #4211

Merged
merged 3 commits into from
Feb 17, 2021
Merged

merlin: communicate STDLIB directive #4211

merged 3 commits into from
Feb 17, 2021

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Feb 10, 2021

Make it so that dune communicates the standard library directory to merlin. This makes it possible to use merlin with an OCaml compiler other than the one it was compiled with.

Fixes #4188

@nojb nojb requested a review from voodoos February 10, 2021 11:48
@nojb
Copy link
Collaborator Author

nojb commented Feb 10, 2021

There is a CI failure which I don't understand, any ideas?

./dune.exe runtest
File "test/blackbox-tests/test-cases/merlin/github4125.t/run.t", line 1, characters 0-0:
         git (internal) (exit 1)
(cd _build/.sandbox/259d57d1d05fbb64c642522c5909613e/default && /usr/bin/git --no-pager diff --no-index --color=always -u ../../../default/test/blackbox-tests/test-cases/merlin/github4125.t/run.t test/blackbox-tests/test-cases/merlin/github4125.t/run.t.corrected)
diff --git a/../../../default/test/blackbox-tests/test-cases/merlin/github4125.t/run.t b/test/blackbox-tests/test-cases/merlin/github4125.t/run.t.corrected
index 8df63f0..1639ec6 100644
--- a/../../../default/test/blackbox-tests/test-cases/merlin/github4125.t/run.t
+++ b/test/blackbox-tests/test-cases/merlin/github4125.t/run.t.corrected
@@ -8,7 +8,7 @@
   $ dune ocaml-merlin --dump-config="$(pwd)" |
   > sed 's#'$(opam config var --switch default prefix)'#OPAM_PREFIX#'
   Foo
-  ((STDLIB OPAM_PREFIX/lib/ocaml)
+  ((STDLIB /usr/lib/ocaml)
    (EXCLUDE_QUERY_DIR)
    (B
     $TESTCASE_ROOT/_build/cross/.foo.objs/byte)
Makefile:73: recipe for target 'test' failed
make: *** [test] Error 1
'make test' failed with code 2

@ghost
Copy link

ghost commented Feb 10, 2021

It could be because in the CI ocaml is installed globally while on your machine ocaml was installed by opam. As a result the paths to the stdlib directory differs. You could try rewriting ocamlc -where rather than opam config var prefix.

BTW, instead of sed you can also set the environment variable BUILD_PATH_PREFIX_MAP from inside a .t file to rewrite paths:

  $ export BUILD_PATH_PREFIX_MAP="/OCAML=`ocamlc -where`:$BUILD_PATH_PREFIX_MAP"
  $ ocamlc -where
  /OCAML

@nojb
Copy link
Collaborator Author

nojb commented Feb 10, 2021

It could be because in the CI ocaml is installed globally while on your machine ocaml was installed by opam. As a result the paths to the stdlib directory differs. You could try rewriting ocamlc -where rather than opam config var prefix.

Yes, but if I do that then the test will may/will fail when run in some developer's machine. Perhaps I should try to do both? I will investigate a bit more.

BTW, instead of sed you can also set the environment variable BUILD_PATH_PREFIX_MAP from inside a .t file to rewrite paths:
...

Thanks for the tip. I used sed because this was already used in some other merlin tests.

@nojb
Copy link
Collaborator Author

nojb commented Feb 11, 2021

It could be because in the CI ocaml is installed globally while on your machine ocaml was installed by opam. As a result the paths to the stdlib directory differs. You could try rewriting ocamlc -where rather than opam config var prefix.

I pushed a fix for the test that appears to work always: opam exec --switch default -- ocamlc -where.

@ghost
Copy link

ghost commented Feb 11, 2021

I would expect just ocamlc -where to be more reliable actually. Dune will use the ocamlc in the PATH (unless the test has a dune-workspace file that says otherwise). So for Dune the stdlib directory will be the output of ocamlc -where and that is what dune will communicate to merlin.

On the other hand, if the opam switch currently in use is not default (for instance because the developer is using a local opam switch), then opam exec --switch default -- ocamlc -where will return a different result.

@nojb
Copy link
Collaborator Author

nojb commented Feb 11, 2021

(unless the test has a dune-workspace file that says otherwise)

That's actually the case in this particular test :) It uses the default opam switch. Since the rest of the testsuite is run under a non-default switch, the compiler in the path does not match the one of the default switch.

@ghost
Copy link

ghost commented Feb 11, 2021

Ah, I see. That seems right then

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A changelog entry seems relevant for this BTW.

I'm not a merlin expert, but looking at the messages in the linked tickets, that does seem like the right thing to do to me.

@nojb
Copy link
Collaborator Author

nojb commented Feb 16, 2021

Friendly ping @voodoos

@nojb nojb force-pushed the merlin_stdlib branch 2 times, most recently from b92ad89 to 8624a8c Compare February 16, 2021 10:00
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I looked at Context.stdlib_dir and it is always populated with the path from the ocamlc -config of the current compiler.

If this is the expected behavior then it's all good (I am asking confirmation because I am not an expert, but this looks like the correct thing to do).

Also it my be good to add a call to merlin in one of the tests to check that it understands the directive as expected, something like:
ocamlmerlin single dump-configuration -filename foo.ml < foo.ml | jq ".value.merlin.stdlib"

@nojb
Copy link
Collaborator Author

nojb commented Feb 16, 2021

Looks good to me.

I looked at Context.stdlib_dir and it is always populated with the path from the ocamlc -config of the current compiler.

If this is the expected behavior then it's all good (I am asking confirmation because I am not an expert, but this looks like the correct thing to do).

Yes, that's the expected behaviour. The point is that the ocamlc in the path may not be the same compiler used to build merlin itself.

Also it my be good to add a call to merlin in one of the tests to check that it understands the directive as expected, something like:
ocamlmerlin single dump-configuration -filename foo.ml < foo.ml | jq ".value.merlin.stdlib"

OK, will do.

@nojb
Copy link
Collaborator Author

nojb commented Feb 16, 2021

Also it my be good to add a call to merlin in one of the tests to check that it understands the directive as expected, something like:
ocamlmerlin single dump-configuration -filename foo.ml < foo.ml | jq ".value.merlin.stdlib"

OK, will do.

I tried doing this, but for some reason I always get the error: "No config found for file \"x.ml\". Try calling dune build.". You can see the output of the ocamlmerlin command in the merlin-stdlib.t test. Any ideas?

@rgrinberg
Copy link
Member

Do we backport this to 2.8.3?

@voodoos
Copy link
Collaborator

voodoos commented Feb 17, 2021

Also it my be good to add a call to merlin in one of the tests to check that it understands the directive as expected, something like:
ocamlmerlin single dump-configuration -filename foo.ml < foo.ml | jq ".value.merlin.stdlib"

OK, will do.

I tried doing this, but for some reason I always get the error: "No config found for file \"x.ml\". Try calling dune build.". You can see the output of the ocamlmerlin command in the merlin-stdlib.t test. Any ideas?

Ok it works when adding a dune-workspace file at the root of the test.
I think the dune ocaml-merlin process started by Merlin ignore the INSIDE_DUNE env variable and try to locate its root which is then outside of the test sandbox.

The best is probably not to test it as it won't be reproducible if Dune is in a larger workspace.

(However it works as expected: "stdlib": "/Users/ulysse/.opam/next-dune/lib/ocaml",)

nojb added 3 commits February 17, 2021 14:48
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb
Copy link
Collaborator Author

nojb commented Feb 17, 2021

The best is probably not to test it as it won't be reproducible if Dune is in a larger workspace.

Thanks, got it. I will merge once the CI is green.

@nojb
Copy link
Collaborator Author

nojb commented Feb 17, 2021

Do we backport this to 2.8.3?

I would say yes.

@rgrinberg rgrinberg added this to the 2.8.3 milestone Feb 17, 2021
@nojb nojb merged commit dc0e0b8 into ocaml:main Feb 17, 2021
@nojb nojb deleted the merlin_stdlib branch February 17, 2021 17:26
voodoos pushed a commit to voodoos/dune that referenced this pull request Feb 22, 2021
rgrinberg pushed a commit that referenced this pull request Mar 7, 2021
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Mar 7, 2021
…ne-action-plugin, dune-private-libs and dune-glob (2.8.3)

CHANGES:

- Make `patdiff` show refined diffs (ocaml/dune#4257, fixes ocaml/dune#4254, @hakuch)

- Fixed a bug that could result in needless recompilation under Windows due to
  case differences in the result of `Sys.getcwd` (observed under `emacs`).
  (ocaml/dune#3966, @nojb).

- Restore compatibility with Coq < 8.10 for coq-lang < 0.3 , document
  that `(using coq 0.3)` does require Coq 8.10 at least (ocaml/dune#4224, fixes
  ocaml/dune#4142, @ejgallego)

- Add a META rule for 'compiler-libs.native-toplevel' (ocaml/dune#4175, @AltGr)

- No longer call `chmod` on symbolic links (fixes ocaml/dune#4195, @dannywillems)

- Dune no longer automatically create or edit `dune-project` files
  (ocaml/dune#4239, fixes ocaml/dune#4108, @jeremiedimino)

- Have `dune` communicate the location of the standard library directory to
  `merlin` (ocaml/dune#4211, fixes ocaml/dune#4188, @nojb)

- Workaround incorrect exception raised by Unix.utimes (OCaml PR#8857) in
  Path.touch on Windows (ocaml/dune#4223, @dra27)

- `dune ocaml-merlin` is now able to provide configuration for source files in
  the `_build` directory. (ocaml/dune#4274, @voodoos)

- Automatically delete left-over Merlin files when rebuilding for the first time
  a project previously built with Dune `<= 2.7`. (ocaml/dune#4261, @voodoos, @aalekseyev)

- Fix `ppx.exe` being compiled for the wrong target when cross-compiling
  (ocaml/dune#3751, fixes ocaml/dune#3698, @toots)

- `dune top` correctly escapes the generated toplevel directives, and make it
  easier for `dune top` to locate C stubs associated to concerned libraries.
  (ocaml/dune#4242, fixes ocaml/dune#4231, @nojb)

- Do not pass include directories containing native objects when compiling
  bytecode (ocaml/dune#4200, @nojb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dune should communicate the standard library directory to merlin
3 participants