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

Add yarn.ps1, fixes #6092 (bad Ctrl C behavior) #6093

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

mikemaccana
Copy link
Contributor

@mikemaccana mikemaccana commented Jul 12, 2018

#6092

Summary

Not a substantial pull request. cmd is unmaintained, according to its authors at Microsoft. One of the issues is that the termination behavior (Ctrl C) for cmd scripts cannot be modifed, but there are likely other issues with cmd being unmaintained.

Test plan

All Windows yarn commands still work, with no effect on speed and output.

@arcanis
Copy link
Member

arcanis commented Jul 13, 2018

Can you detail what you mean with "the termination behavior (Ctrl C) for cmd scripts cannot be modifed"? What exactly doesn't work?

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Jul 13, 2018

Hi Arcanis! You can see the discussion with Microsoft re cmd termination behavior in the linked bug. Short version is that cmd requires Ctrl C twice, and everyone on the cmd and console team at Microsoft firmly recommend new software not be made using cmd.

@arcanis
Copy link
Member

arcanis commented Jul 14, 2018

What do you think about microsoft/terminal#217 (comment)? It seems like a valid concern.

It seems to me that cmd scripts are generally used in the place of exe symlinks and shebang scripts, which PowerShell is not a good replacement for - if you switch out yarn.cmd for yarn.ps1, that's fine if you're already in PowerShell, but if you're in cmd, or some other shell, or using ShellExecute or some scripting language's equivalent, you have to write pwsh -c yarn, so then why not get rid of the script altogether and do node yarn.js?

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Jul 16, 2018

@arcanis Comment is accurate but doesn't affect anything:

  • Yes, if I was calling yarn.js from something other than the command line, I'd call node yarn.js and not use yarn.ps1.
  • In the exact same way, right now, if I was calling yarn.js from something other than the command line, I'd call node yarn.js and not use yarn.cmd.

The purpose of the ps1 or cmd file is a quick alias to be run from the shell. The default shell in Windows is Powershell (see Win X). Powershell is also the only maintained shell, according to the cmd authors. Moving to powershell (as well as fixing termination issues) means that when something goes wrong we don't have to debug an unsupported shell.

@parkovski
Copy link

I'm the one with the concerned comment. I just did an experiment and found that PowerShell will pick a .ps1 file before a .cmd, so rather than replace the script, can you just add the PowerShell script? That should give you working ^C behavior in PowerShell without breaking cmd.

@kumarharsh
Copy link

@mikemaccana quick question - are you proposing that the .cmd file still be retained?

I followed the discussion here and came to this issue. I created a yarn.ps1 file in my yarn installation folder and then ran yarn. It works very well! Ctrl+C doesn't ask the question Terminate batch job (Y/N)?. So I imagined that we could remove the .cmd file and the .ps1 file would be enough.

But if I remove the yarn.cmd file, then the yarn.ps1 script fails - it opens up yarn.js in my text editor instead! This is a slightly weird side-effect.

@mikemaccana
Copy link
Contributor Author

@kumarharsh not sure what the purpose of the cmd file would be. yarn.ps will help default command line users, node can run the yarn.js itself. Only reason I could see if for users who explicitly launch cmd as an interactive terminal in 2018 - most of those are probably doing that out of ignorance so education might be better.

Are you using the ps1 from this commit, or a file you made?

@kumarharsh
Copy link

I created a ps1 file with the contents of this PR. It's weird that removing the cmd file stops the ps1 file from working - I'm using pwsh as my default shell fwiw.

@arcanis
Copy link
Member

arcanis commented Jul 19, 2018

Only reason I could see if for users who explicitly launch cmd as an interactive terminal in 2018 - most of those are probably doing that out of ignorance so education might be better.

Fwiw I hate* powershell even more than I hate cmd, so I tend to use cmd at home. I doubt I'm the only one, so I'd say it's safer to keep both.

(*) Hate is a strong word tbh, it's just that I never got the time to learn the Powershell syntaxes, and I mostly use WSL nowadays anyway, so it's not a priority.

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Please commit back the cmd file

@Gudahtt
Copy link
Member

Gudahtt commented Aug 5, 2018

Keeping yarn.cmd is definitely the safer option.

A common method of determining whether a file is executable in Windows is to compare its file extension with the extensions listed in the PathExt environment variable. PS1 is typically not in that list, but CMD is.

node-which is one example of a library that uses this method. If yarn.cmd was removed, then anything that uses node-which directly to find the yarn executable would instead find yarn.js (because .JS is typically in PathExt, after .CMD). If they then tried to execute yarn.js directly on Windows, it would blow up. Libraries like cross-spawn are smart enough to also read the shebang from yarn.js and execute it with node instead, but... a shebang is not conventional in Windows, and most libraries aren't as smart as cross-spawn. It's conceivable that removing yarn.cmd would break things, even putting cmd.exe interactive terminal users aside.

Fixes yarnpkg#6902. Leave in yarn.cmd in case anyone is still using cmd on Windows in 2018.
@mikemaccana mikemaccana changed the title Make Windows command executable use powershell rather than cmd. Fixes… Add yarn.ps1, fixes #6092 (bad Ctrl C behavior) Aug 21, 2018
@buildsize
Copy link

buildsize bot commented Aug 21, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.01 MB 926.37 KB -108.32 KB (10%)
yarn-[version].js 4.12 MB 4.01 MB -113.02 KB (3%)
yarn-legacy-[version].js 4.05 MB 4.17 MB 119.28 KB (3%)
yarn-v[version].tar.gz 1.02 MB 931.78 KB -108.09 KB (10%)
yarn_[version]all.deb 746.3 KB 685.81 KB -60.5 KB (8%)

@mikemaccana
Copy link
Contributor Author

As requested, I've updated the PR and left yarn.cmd for any people still running cmd.

@mikemaccana
Copy link
Contributor Author

Relevant discussion on npm: npm/rfcs#20

@arcanis
Copy link
Member

arcanis commented Aug 21, 2018

Perfect, thanks!

@arcanis arcanis merged commit 01f9adc into yarnpkg:master Aug 21, 2018
function Get-ScriptDirectory {
$Invocation = (Get-Variable MyInvocation -Scope 1).Value
Split-Path $Invocation.MyCommand.Path
}

Choose a reason for hiding this comment

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

Is there any difference between the output of this function and $PSScriptRoot?

Copy link
Member

@Gudahtt Gudahtt Aug 22, 2018

Choose a reason for hiding this comment

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

$PSScriptRoot may not have been considered because it was introduced in PowerShell 3. It would not work for users of older versions of PowerShell.

Edit: Sorry, forgot to answer your question. No, I don't believe there is any difference.

Copy link
Contributor Author

@mikemaccana mikemaccana Aug 22, 2018

Choose a reason for hiding this comment

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

As the author $PSScriptRoot wasn't considered because I just learnt about it from @felixfbecker. 😀 Do we want to support Windows 7 default (Powershell 2) or not though?

Choose a reason for hiding this comment

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

If $PSScriptRoot was introduced in PS 3 that would mean it is not supported in Windows 7 - I don't know what the support policy is here from yarn / NodeJS?

@ghost
Copy link

ghost commented Oct 5, 2018

I use cmd because I don't like powershell, but also because && doesn't work in powershell [1] and there are a lot of package.json files out there that utilize this very handy trick to run 2 different commands from the same scripts entry (e.g. "prepublish": "./node_modules/.bin/projectz compile && npm test").

[1] npm/npm#4040 (comment)

@felixfbecker
Copy link

@waynebloss npm always uses cmd for scripts, even when running them from PowerShell.

@ghost
Copy link

ghost commented Oct 5, 2018

@felixfbecker OK, thanks. I'm not sure if that was always the case or not - I've been using Node since very early on in Windows and well, regardless of what I do with powershell I always have problems :)

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.

6 participants