Skip to content

Windows: correctly redirect stderr to stdout of called ppx #1413

Merged
let-def merged 1 commit intoocaml:masterfrom
MisterDA:windows-createprocess
Dec 20, 2021
Merged

Windows: correctly redirect stderr to stdout of called ppx #1413
let-def merged 1 commit intoocaml:masterfrom
MisterDA:windows-createprocess

Conversation

@MisterDA
Copy link
Copy Markdown
Contributor

@MisterDA MisterDA commented Dec 9, 2021 •
edited
Loading

There's no need for quoting the files as we're not calling the ppx
through a shell.

Redirect the stderr of the child process to which of the parent (as
posix system(3)), and the stdout of the child to the stderr of the
child (as merlin's ppx_commandline).

Setting up the redirection in the parent process has the downside that
we must set the bInheritHandles flag of CreateProcess, which means
that if merlin leaks a handle (not setting it non-inheritable /
cloexec), the ppx process will have access to it.

cc @let-def @nojb

nojb
nojb reviewed Dec 9, 2021
View reviewed changes
@MisterDA MisterDA force-pushed the windows-createprocess branch from 994d899 to c37d756 Compare December 9, 2021 11:26
nojb
nojb reviewed Dec 9, 2021
View reviewed changes
@MisterDA MisterDA force-pushed the windows-createprocess branch from c37d756 to 2e0bc5c Compare December 9, 2021 17:44
nojb
nojb reviewed Dec 14, 2021
View reviewed changes
@MisterDA MisterDA force-pushed the windows-createprocess branch from 2e0bc5c to 0be39d4 Compare December 17, 2021 16:56
@MisterDA
Copy link
Copy Markdown
Contributor Author

MisterDA commented Dec 18, 2021

I think with the latest push the code should be right: we still need to quote the arguments given to the program, we add 1>&2 when we're not running on Windows. For the process creation, we disable stdin of the child. We get the stderr of the parent, check whether it is inheritable (keep-on-exec) (it should be). If not, we duplicate it and mark it inheritable. We set the child's stderr and stdout to that handle.

nojb
nojb reviewed Dec 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nojb nojb 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, but I think there are a couple of points to correct (and a stylistic suggestion).

@MisterDA MisterDA force-pushed the windows-createprocess branch from 0be39d4 to 8eed93b Compare December 20, 2021 11:52
@MisterDA
Copy link
Copy Markdown
Contributor Author

MisterDA commented Dec 20, 2021

Thanks for the thorough review! A lot of omissions on my end...

nojb
nojb approved these changes Dec 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM. I can give this a try on my setup a bit later today and confirm that it works.

nojb
nojb approved these changes Dec 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I tried it very briefly and it seems to work. LGTM.

@let-def
Copy link
Copy Markdown
Contributor

let-def commented Dec 20, 2021

Thanks @MisterDA and @nojb.

@MisterDA: can you update the changelog (CHANGES.md)?
And I will merge.

@MisterDA MisterDA force-pushed the windows-createprocess branch from 8eed93b to 8733ca0 Compare December 20, 2021 15:11
@MisterDA
Copy link
Copy Markdown
Contributor Author

MisterDA commented Dec 20, 2021

@MisterDA: can you update the changelog (CHANGES.md)? And I will merge.

Done.

nojb
nojb reviewed Dec 20, 2021
View reviewed changes
@MisterDA
Windows: correctly redirect stderr to stdout of called ppx
bf8e7b0 
Redirect the stdout of the child process to which of the parent (as
posix system(3)), and the stderr of the child to the stdout of the
child (as merlin's ppx_commandline).

Setting up the redirection in the parent process has the downside that
we must set the bInheritHandles flag of CreateProcess, which means
that if merlin doesn't set a handle non-inheritable (close-on-exec),
the ppx process will have access to it.

Reorder a bit the logging to print the parameters the ppx is called
with, and use an Unicode environment for the child process.
@MisterDA MisterDA force-pushed the windows-createprocess branch from 8733ca0 to bf8e7b0 Compare December 20, 2021 15:25
@let-def
Copy link
Copy Markdown
Contributor

let-def commented Dec 20, 2021

Thank you ;).

@let-def let-def merged commit f1d1cfc into ocaml:master Dec 20, 2021
@MisterDA MisterDA deleted the windows-createprocess branch December 20, 2021 15:34
@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Dec 21, 2021

@MisterDA should we backport this patch to Merlin for OCaml 411 and/or 412 in your opinion ?

@MisterDA
Copy link
Copy Markdown
Contributor Author

MisterDA commented Dec 21, 2021

Yes. Merlin is still usable, but without the patch is broken when using ppx's. It cannot do anything when the code is in extension points (so that breaks for instance lwt_ppx or ppx_expect) or cannot show the type of functions generated by derivers (for instance ppx_sexp or yojson). I ignored errors on derivers, but on extension points it's more annoying not to be able to use Merlin on bigger chunks of code.
I don't think the patch would be difficult to backport.

voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-413)
11d8687 
CHANGES:

Tue Apr  5 20:59:42 CEST 2020

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-412)
5d18fa3 
CHANGES:

Tue Apr  5 21:12:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - improve load path performance (ocaml/merlin#1323)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-411)
64dcc60 
CHANGES:

Tue Apr 5 21:17:21 PM CET 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handling of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - update quick setup instructions for emacs (ocaml/merlin#1380, @ScriptDevil)
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin 4.5-414/413/412/411 and dot-merlin-reader 4.2
5940165 
CHANGES for 414:

Tue Apr  5 20:51:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
    - use the new "shapes" generated by the compiler to perform precise
      jump-to-definition (ocaml/merlin#1431)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)

CHANGES for 413:

Tue Apr  5 20:59:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)

CHANGES for 412:

Tue Apr  5 21:12:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - improve load path performance (ocaml/merlin#1323)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)

CHANGES for 411:

Tue Apr 5 21:17:21 PM CET 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handling of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - update quick setup instructions for emacs (ocaml/merlin#1380, @ScriptDevil)
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
@voodoos voodoos mentioned this pull request Apr 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nojb nojb nojb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MisterDA @let-def @voodoos @nojb