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.

Sign up for GitHub

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

Jump to bottom

Fix FLG -pp ppx.exe -as-pp/-dump-ast on Windows #1723

Merged
merged 6 commits into from
Jan 30, 2024
Merged

Fix FLG -pp ppx.exe -as-pp/-dump-ast on Windows #1723

merged 6 commits into from
Jan 30, 2024

Conversation

jonahbeckford
Copy link
Contributor

Windows is special cased in Merlin to have a direct
program execution without any shell so shell redirection
does not work.

This is a narrow, Windows-only fix for
#1722

I don't particularly like this one since it is based on heuristics. And it does seem like there is a lot of unfixed code in other packages that is required to get -as-pp working on Windows. Perhaps someone more knowledgeable about Merlin can make this cleaner.

@jonahbeckford
Fix FLG -pp ppx.exe -as-pp/-dump-ast on Windows
f3106eb 
Windows is special cased in Merlin to have a direct
program execution without any shell, so shell redirection
does not work.

This is a narrow, Windows-only fix for
ocaml#1722
@jonahbeckford
Include standalone [ppx.exe] in Win32 no-shell fixes
ded91cf
@jonahbeckford
outfile option in ml_merlin_system_command
70711db
nojb
nojb reviewed Jan 26, 2024
View reviewed changes
Copy link
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.

Exactly what I had in mind, thanks!

src/utils/std.ml Outdated Show resolved Hide resolved
nojb
nojb approved these changes Jan 27, 2024
View reviewed changes
Copy link
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

Someone with commit privileges will need to approve/merge (cc @voodoos)

voodoos
voodoos approved these changes Jan 29, 2024
View reviewed changes
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.

I am not expert enough to comment the Windows code, it looks all right and I will trust your judgement on it.

The rest of the PR looks fine to me, I just made a few stylistic propositions.

CHANGES.md Outdated Show resolved Hide resolved
src/utils/std.ml Outdated Show resolved Hide resolved
src/utils/std.ml Outdated Show resolved Hide resolved
@jonahbeckford @voodoos
Apply suggestions from code review
992af90 
Style conformity

Co-authored-by: Ulysse <[email protected]>
@jonahbeckford jonahbeckford mentioned this pull request Jan 29, 2024
Closed
@nojb
Copy link
Contributor

nojb commented Jan 30, 2024

Friendly ping @voodoos: I think the PR can now be merged.

@voodoos
Copy link
Collaborator

voodoos commented Jan 30, 2024

Thanks @jonahbeckford !

@voodoos voodoos merged commit 05e90d8 into ocaml:master Jan 30, 2024
12 checks passed
voodoos added a commit to voodoos/merlin that referenced this pull request Feb 20, 2024
@voodoos
[B] Fix FLG -pp ppx.exe -as-pp/-dump-ast on Windows (ocaml#1723)
12d9e57 
from jonahbeckford/fix-noshellredirect-win32
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin (3 packages) (4.14-414)
1e53e68 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.14-500)
7905af8 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin (3 packages) (4.14-501)
613d86e 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin-lib, merlin (4.14-501/500/414)
6b881a5 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
@voodoos voodoos mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos approved these changes

@nojb nojb nojb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Backport to 501
Status: 4.14-500
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonahbeckford @nojb @voodoos