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

fix(core): use binjumper to work-around 'Terminate batch job' prompts on Windows #1808

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Sep 5, 2020

What's the problem this PR addresses?

Due to the use of cmd jumper-files to spawn binaries, terminating scripts on Windows is horrendously annoying since CMD provides no way to disable the Terminate batch job (Y/N) prompt

Fixes #1592

How did you fix it?

Create the jumper files using https://github.com/merceyz/binjumper to avoid the prompts

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I have verified that all automated PR checks pass.

@merceyz merceyz requested a review from arcanis as a code owner September 5, 2020 22:01
@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch from 22d7aa8 to 33fe322 Compare September 5, 2020 23:19
@merceyz merceyz marked this pull request as draft September 6, 2020 00:34
@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch from 33fe322 to 82cb285 Compare September 6, 2020 19:34
@merceyz merceyz marked this pull request as ready for review September 6, 2020 19:35
@arcanis
Copy link
Member

arcanis commented Sep 6, 2020

since CMD provides no way to disable the Terminate batch job (Y/N) prompt

Would creating a Powershell script instead of CMD work? The builtin binaries approach is a bit magical, I'm slightly worry it would open us to weird differences between spawning a binary from a script versus from a shellscript spawned by a script 🤔

@merceyz
Copy link
Member Author

merceyz commented Sep 9, 2020

Would creating a Powershell script instead of CMD work?

While Powershell doesn't create the prompt it seems cross-spawn doesn't know how to handle them, I added .ps1 to the PATHEXT but it just opens the file in Visual Studio.

The builtin binaries approach is a bit magical, I'm slightly worry it would open us to weird differences between spawning a binary from a script versus from a shellscript spawned by a script

The difference would be the prompt, otherwise it shouldn't be noticable 🤔

@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch from 82cb285 to 37be660 Compare September 9, 2020 20:35
@arcanis
Copy link
Member

arcanis commented Sep 17, 2020

Related: microsoft/terminal#217

@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch 2 times, most recently from 82eb8ea to cccad39 Compare September 19, 2020 20:58
@merceyz merceyz marked this pull request as draft September 19, 2020 21:10
@merceyz merceyz changed the title feat: treat binaries as cli builtins when spawning them fix(core): use binjumper to work around 'Terminate batch job' prompts on Windows Sep 27, 2020
@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch 2 times, most recently from 107f807 to d54dab6 Compare September 27, 2020 13:10
@merceyz merceyz marked this pull request as ready for review September 27, 2020 15:34
@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch 2 times, most recently from 724845b to ef76fa2 Compare September 27, 2020 21:54
@merceyz merceyz changed the title fix(core): use binjumper to work around 'Terminate batch job' prompts on Windows fix(core): use binjumper to work-around 'Terminate batch job' prompts on Windows Sep 27, 2020
@merceyz merceyz force-pushed the merceyz/terminate-batch-job branch from ef76fa2 to b6300bc Compare September 28, 2020 06:58
@arcanis arcanis merged commit 97a66ff into master Sep 30, 2020
@arcanis arcanis deleted the merceyz/terminate-batch-job branch September 30, 2020 13:34
@cowboyd
Copy link

cowboyd commented Nov 12, 2020

Can we backport this to 1.x? I'd be happy to give it a shot.

@arcanis
Copy link
Member

arcanis commented Nov 13, 2020

@cowboyd 1.x is in feature freeze, only security issues will be addressed. I suggest you take a look at the migration guide if you need features introduced in the 2.x.

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.

[Bug] Terminate batch job (Y/N)?
3 participants