-
Notifications
You must be signed in to change notification settings - Fork 233
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
Request: do not use chdir for ppx #1420
Request: do not use chdir for ppx #1420
Comments
cc @let-def |
I think this would be a nice improvement, even outside ocamllsp. |
Agreed with let-def, no special need for a new dependency. |
The advantage of using spawn is that we'll have a single code path for spawning processes in lsp. That seems useful. if you don't want a dependency, you could also vendor the spawn library inside merlin. Just a suggestion, feel free to do it however you prefer. I'm just happy to see chdir go away. |
One issue I can see is that is that |
Is that a problem in practice? In dune, we only generate ppx invocations that are independent of sh. And even if we needed sh, we could always spawn something like: |
I'm not sure, but having to quote things always makes me nervous :) |
In merlin on Windows we're already using |
When I was manually writing .merlin files, I would use some shell syntax in the For the invocation, it would be nice to use By the way, I think Merlin should still chdir to Finally, there are two notions of "working directory" in Merlin:
I don't know if this subtlety can be communicated via the protocol; if not it is something to consider for the future. |
Hopefully nobody has to write a .merlin file manually ever again :)
Yes, fork (or vfork on linux) + chdir is the way to go. posix_spawn can be added as an optimization later (we plan to do it in spawn in fact)
Why do you think so? |
The way spawn does is it is chdir in the child and then exec. Exactly as you've said. |
I've prepared the following patch that seems to work: I avoided the use of chdir even for linux as it's always in a racy environment. I tested it on windows/mac and it seems to work. |
I left a few commets about the Windows stubs there. |
@rgrinberg: Sorry for the late reply. In the past, we had users that launched merlin from a directory that was sometimes deleted soon after (for instance, after a build system cleanup). The behavior was quite chaotic after the working directory was removed (most stuff work, but occasional actions would fail) |
As long as this behavior isn't forced on those that use merlin as a library, I'm ok with it. |
What is the status on this ? It is one of the last items blocking the direct use of Merlin as a lib in ocaml-lsp. |
Yes, it's been used in LSP for a while now. One thing that still leaves me unsatisfied about the patch is the indirection to shell. I'd much prefer if the preprocessing command was interpreted in the same that dune does. I suppose I can submit a patch if the issue above doesn't deter you. |
@rgrinberg, @let-def, any news on this ? |
CHANGES: Thu Nov 24 13:31:42 CEST 2022 + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Do not change temporarily Merlin's cwd when starting a PPX (ocaml/merlin#1521, fixes ocaml/merlin#1420) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) - Add a new `verbosity=smart` mode for type enclosing that only expand modules' types (ocaml/merlin#1374, @ulugbekna) - Improve locate for labels' declarations in the current buffer. (ocaml/merlin#1505, fixes ocaml/merlin#1524) - Fix locate on module without implementation (ocaml/merlin#1522, fixes ocaml/merlin#1519) - Allow program name customization when merlin is used as a library. (ocaml/merlin#1532) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) - emacs: update CI for newer releases and fix some warnings (ocaml/merlin#1454, @mattiase) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comment documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512) - Add tests for interactions between locate and record labels (ocaml/merlin#1505) - Add test showing an issue with locate and implicit transitive deps
CHANGES: + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Do not change temporarily Merlin's cwd when starting a PPX (ocaml/merlin#1521, fixes ocaml/merlin#1420) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) - Add a new `verbosity=smart` mode for type enclosing that only expand modules' types (ocaml/merlin#1374, @ulugbekna) - Improve locate for labels' declarations in the current buffer. (ocaml/merlin#1505, fixes ocaml/merlin#1524) - Fix locate on module without implementation (ocaml/merlin#1522, fixes ocaml/merlin#1519) - Allow program name customization when merlin is used as a library. (ocaml/merlin#1532) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) - emacs: update CI for newer releases and fix some warnings (ocaml/merlin#1454, @mattiase) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comment documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512) - Add tests for interactions between locate and record labels (ocaml/merlin#1505) - Add test showing an issue with locate and implicit transitive deps [new release] merlin (4.7-413) CHANGES: + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comments documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512) [new release] merlin (4.7-412) CHANGES: + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comment documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512)
CHANGES: Thu Nov 24 17:49:42 CEST 2022 + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Do not change temporarily Merlin's cwd when starting a PPX (ocaml/merlin#1521, fixes ocaml/merlin#1420) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) - Add a new `verbosity=smart` mode for type enclosing that only expand modules' types (ocaml/merlin#1374, @ulugbekna) - Improve locate for labels' declarations in the current buffer. (ocaml/merlin#1505, fixes ocaml/merlin#1524) - Fix locate on module without implementation (ocaml/merlin#1522, fixes ocaml/merlin#1519) - Allow program name customization when merlin is used as a library. (ocaml/merlin#1532) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) - emacs: update CI for newer releases and fix some warnings (ocaml/merlin#1454, @mattiase) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comment documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512) - Add tests for interactions between locate and record labels (ocaml/merlin#1505) - Add test showing an issue with locate and implicit transitive deps
CHANGES: + merlin binary - Replace custom "holes" AST nodes by extensions. This restores binary compatibility and fixes issues with PPXs when using typed-holes. (ocaml/merlin#1503) - Do not change temporarily Merlin's cwd when starting a PPX (ocaml/merlin#1521, fixes ocaml/merlin#1420) - Fix a parsing issue when declaring the `(??)` custom prefix operator. (ocaml/merlin#1507, fixes ocaml/merlin#1506) - Fix variant constructors' comments grouping (ocaml/merlin#1516, @mheiber, fixes ocaml/merlin#1513) - Filter-out duplicates from the `enclosing` command result (ocaml/merlin#1512) - Add a new `verbosity=smart` mode for type enclosing that only expand modules' types (ocaml/merlin#1374, @ulugbekna) - Improve locate for labels' declarations in the current buffer. (ocaml/merlin#1505, fixes ocaml/merlin#1524) - Fix locate on module without implementation (ocaml/merlin#1522, fixes ocaml/merlin#1519) - Allow program name customization when merlin is used as a library. (ocaml/merlin#1532) + editor modes - vim: load the plugin when necessary if it wasn't loaded before (ocaml/merlin#1511) - emacs: update CI for newer releases and fix some warnings (ocaml/merlin#1454, @mattiase) + test suite - Add tests for constructors' documentation (ocaml/merlin#1511) - Add test cases for label comment documentation (ocaml/merlin#1526, @mheiber) - Add a test for the `enclosing` command (ocaml/merlin#1512) - Add tests for interactions between locate and record labels (ocaml/merlin#1505) - Add test showing an issue with locate and implicit transitive deps
This isn't something user facing, rather a matter of making it easy to use merlin correctly in ocamllsp. You might be aware that ocamllsp runs the merlin pipeline in a dedicated thread to make sure the server stays responsive while merlin is busy. That works well until preprocessing is involved.
chdir
is not threadsafe and therefore merlin's use of it for running pp/ppx breaks ocamllsp in some subtle (and painfully hard to debug) ways. Thus my humble request from the merlin team is to accept a patch that removes the use of chdir.I haven't yet written the patch, but I'm thinking that it can be done by using the spawn library. This library is successfully used by dune and ocamllsp itself so it is well tested. It's also light on the dependencies.
The only problem I could imagine is that merlin has some code for launching processes on Windows. So perhaps we can keep the current code for windows and add a cwd argument? Or improve spawn in a way that makes this custom code unnecessary? cc @MisterDA
The text was updated successfully, but these errors were encountered: