Skip to content

Dune helper: don't inherit stderr#1263

Merged
let-def merged 1 commit intomasterfrom
dune-helper-windows-fix
Feb 11, 2021
Merged

Dune helper: don't inherit stderr#1263
let-def merged 1 commit intomasterfrom
dune-helper-windows-fix

Conversation

@let-def
Copy link
Copy Markdown
Contributor

@let-def let-def commented Feb 8, 2021

Dune helper used to cause Emacs to block under windows. This should no longer be the case.

It happened because Merlin's stderr file descriptor was inherited by Dune helper.
This is not a problem under Unix because replacing stderr causes the previous file descriptor to be closed. Under windows, the previous file handle stays open even if it is no longer stderr.

@voodoos @nojb

trefis
trefis approved these changes Feb 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

😍

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Feb 8, 2021

Thanks @let-def ! I tried this with the set up of #1256 , I get the following error (inside emacs) :
flag -filename: error, Unix.Unix_error(1, "set_close_on_exec", ""). Any ideas?

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Feb 8, 2021

(I cherry-picked your commit to the 3.4.2 branch)

@let-def
Copy link
Copy Markdown
Contributor Author

let-def commented Feb 8, 2021

@nojb Worrying, I didn't manage to reproduce yet, I will look again tomorrow.
However, can you try again after commenting the Unix.clear_close_on_exec Unix.stderr; line?

Looking at OCaml unix source, the error message is wrong, "set_close_on_exec" message is raised both by set and clear commands, and it will help to be sure where it fails. (The win32unix implementation is different, but since we are on cygwin, I guess it is the normal unix implementation that is used?)

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Feb 8, 2021

@nojb Worrying, I didn't manage to reproduce yet, I will look again tomorrow.
However, can you try again after commenting the Unix.clear_close_on_exec Unix.stderr; line?

No difference, but the failing call is set_close_on_exec (see below)

Looking at OCaml unix source, the error message is wrong, "set_close_on_exec" message is raised both by set and clear commands, and it will help to be sure where it fails. (The win32unix implementation is different, but since we are on cygwin, I guess it is the normal unix implementation that is used?)

I am not using Cygwin-compiled binaries, but rather binaries compiled using the msvc compiler, so we really are using win32unix.

@let-def
Copy link
Copy Markdown
Contributor Author

let-def commented Feb 10, 2021

Thanks for the explanation. Here it seems the bug is reliably fixed on Cygwin.
I am not sure how to investigate on MSVC, I will keep you posted.

@let-def let-def force-pushed the dune-helper-windows-fix branch from e904010 to 9a7e589 Compare February 11, 2021 14:12
@let-def
Copy link
Copy Markdown
Contributor Author

let-def commented Feb 11, 2021

@nojb I think I have a good fix. I tested with Mingw and MSVC. (curiously, I observed differences in handle management).

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Feb 11, 2021

@nojb I think I have a good fix. I tested with Mingw and MSVC. (curiously, I observed differences in handle management).

Thanks for investigating this tricky bug! I confirm that emacs no longer blocks with this commit.

@let-def
Copy link
Copy Markdown
Contributor Author

let-def commented Feb 11, 2021

Thanks for testing. The change should be released soon.

@let-def let-def merged commit ecde690 into master Feb 11, 2021
@let-def let-def deleted the dune-helper-windows-fix branch February 11, 2021 15:28
@nojb
Copy link
Copy Markdown
Contributor

nojb commented Feb 11, 2021

Thanks for testing. The change should be released soon.

Backporting to the 3.4.2 branch would also be appreciated. Thanks!

trefis pushed a commit that referenced this pull request Feb 16, 2021
@let-def @trefis
Fixup windows (#1263)
4d218e5
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-411)
0f0879b 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-412)
b23f5f0 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-412)
0f03a55 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-411)
ccfc74c 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] dot-merlin-reader (4.1)
f3ef1ab 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-412)
9bb8393 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] merlin (4.1-411)
0f728c3 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis added a commit to trefis/opam-repository that referenced this pull request Feb 16, 2021
@trefis
[new release] dot-merlin-reader (4.1)
b745377 
CHANGES:

Tue Feb 16 10:33:11 AM CET 2021

  + merlin binary:
    - fix windows paths canonicalization (ocaml/merlin#1254)
    - fix hanging on windows (ocaml/merlin#1256, ocaml/merlin#1263)
trefis pushed a commit that referenced this pull request Feb 23, 2021
@let-def @trefis
Fixup windows (#1263)
a679bb3
voodoos pushed a commit to voodoos/merlin that referenced this pull request Mar 3, 2021
@let-def @voodoos
Fixup windows (ocaml#1263)
89cc42b
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 13, 2021
@voodoos
[new release] merlin and dot-merlin-reader (3.5.0)
22e8ac0 
CHANGES:

Tue Apr 12 11:44:22 AM CET 2021

  + merlin binary
    - external configuration reading:
      + use relative paths to communicate with Dune when possible. This solves
        issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271,
        fixes ocaml/merlin#1288)
      + make the `workdir` configuration value when using the
        `dune ocaml-merlin` configuration provider the same as when using
        `dot-merlin-reader` so that ppxes behaves in the same way as before
        (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292)
    - destruct:  make the destruct command more resilient to ill-typed
      expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300)
    - Mppx: don't restore cookies after invocation. Ppx are invoked only once
      so there is no need to manage cookies. This small change should increase
      performance and should not change any other behavior (ocaml/merlin#1309)
    - windows:
      + system command variant: do not open a window console when
        launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714)
      + fix Emacs hanging when starting Merlin (ocaml/merlin#1263)
      + fix path canonicalization (ocaml/merlin#1254)
    - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261)
  + editor modes
    - emacs:
      + modernization of the elisp code and conformance with coding
        guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell )
      + use opam var where applicable (ocaml/merlin#1310)
      + fix "wrong number of argument" (ocaml/merlin#1250 by @atharvashukla, fixes ocaml/merlin#1234)
      + fix for Neovim's CursorMoved semantics (ocaml/merlin#1213 by @ddickstein)
    - vim & emacs : new client-side "merlin use package" commands, restoring
      previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191)
  + test suite
    - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)
@mroch
Copy link
Copy Markdown

mroch commented Jul 12, 2022

sorry for pinging such an old PR. can you explain the difference between the original version of this PR (e904010) using Unix.set_close_on_exec and the final version (9a7e589) that directly calls SetHandleInformation?

it seems like Unix.stderr calls _get_osfhandle (here) and then Unix.set_close_on_exec calls caml_win32_set_inherit (here) which calls the same SetHandleInformation (here)

I ask because we're doing the same thing using Unix.set_close_on_exec in Flow here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trefis trefis trefis approved these changes

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

@let-def @nojb @mroch @trefis