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

powershell scripts for installed binaries #20

Closed
wants to merge 4 commits into from

Conversation

bterlson
Copy link
Contributor

Let's get rid of "Terminate batch job (Y/N)" for Windows users using powershell!

```powershell
$myPath = Split-Path $myInvocation.MyCommand.Path -Parent
$node = Join-Path $myPath "node.exe"
$binPath = Join-Path $myPath "[ path to node_module bin script, e.g. node_modules\typescript\bin\tsc]"
Copy link

Choose a reason for hiding this comment

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

For a generated script, this should use single quotes.

@kumarharsh
Copy link

@bterson nice job. You can also add that MS (or atleast some of their top employees) itself recommends we use ps1 scripts. See microsoft/terminal#217 (comment)

Here's an example of a functionally identical powershell wrapper:

```powershell
$myPath = Split-Path $myInvocation.MyCommand.Path -Parent
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between this and $PSScriptRoot?

Copy link

@joeyaiello joeyaiello Aug 22, 2018

Choose a reason for hiding this comment

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

PM for PowerShell here, @bterlson reached out to me on this one.

Citing this SO article, if you care about support on Windows 7 or Server 2008 R2 where PowerShell has not been updated past the inbox PowerShell 2.0, you should stick with $myInvocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @joeyaiello! So IMO it's probably better to err on the side of caution and go with the more compatible $myInvocation.

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

I'd like to see a concrete response to #20 (comment) and https://twitter.com/paulcbetts/status/1031669429111640064 before ratifying this RFC. I'm otherwise 👍 so once that's tested and confirmed and we have understand what the behavior should be (and whether it might cause any big problems), we'll probably just ratify this. :)

This RFC is at a stage where it's probably fine to put together a PR for cmd-shim to make sure nothing surprising happens.

@bterlson
Copy link
Contributor Author

Roger that, thanks @zkat! I plan to test with a fresh install of Windows. FWIW, I believe it's going to work because scripts created from a local process (i.e. node.exe) should be considered local scripts rather than remote scripts. I'm unfortunately headed out for vacation as we speak, so if someone is super amped about this and wants to test this, I'd be happy! Otherwise, I'll be back in September.

@be5invis
Copy link

However the more reliable way should be, like, emitting an EXE?

@be5invis
Copy link

@bterlson
I digged into my PATHEXT and saw this...

.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.CPL

... and I saw these

WSF;WSH

These are WSH's batch files and runs blocks of VBScript/JScript (the very old version, stopped at ES3...), and it existed in Windows for over 20 years. I think they could be a very interesting solution for this particular problem.

@kumarharsh
Copy link

kumarharsh commented Sep 28, 2018

😂 Using such an ancient tech which very few people even remember how to program might be counter-productive to our goals here, no?

Btw, there is .js too there in your PATHEXT. Is that Javascript?

@be5invis
Copy link

@kumarharsh But, isn't CMD more ancient?

@kumarharsh
Copy link

kumarharsh commented Sep 28, 2018

Yeah, but the issue is about migrating from CMD to something more modern... migrating to JScript would not be ideal IMO

@be5invis
Copy link

@kumarharsh
The best way would be generating EXEs...
Actually you CAN modify an EXE, keeping it digitally signed, adding custom data at the end of the executable file, to do the job of CMDs.
Though bad antivirus software may consider Node as a virus...

https://blog.barthe.ph/2009/02/22/change-signed-executable/

@kumarharsh
Copy link

The technique sound great, apart from the virus caveat. Does Windows Defender catch these types of modifications to an EXE?

@be5invis
Copy link

be5invis commented Sep 28, 2018

@kumarharsh Not tried. Maybe I can try them in the incoming holiday (Oct 1-7).

@dahlbyk
Copy link

dahlbyk commented Sep 28, 2018

Though bad antivirus software may consider Node as a virus...

IMHO the Windows antivirus world is still a disaster, and should absolutely preclude any notion of shipping a .exe for a shim.

@be5invis
Copy link

@dahlbyk But if the emitted EXE is digitally signed, the situation could be different.
We may need to test them on Windows PC with different AV softwares installed.

@zkat
Copy link
Contributor

zkat commented Dec 20, 2018

Hey @bterlson! I assume you're going on another vacation very soon (lol), but when you're able, can you get around to test the open questions so we can get a better idea about what we'll be dealing with in re execpolicy and how it should be addressed?

@isaacs
Copy link
Contributor

isaacs commented Aug 15, 2019

Is it safe to move this to "implemented" since cmd-shim landed npm/cmd-shim#34 in [email protected], which is set for release in npm 6.11.0?

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Enhancement new feature or improvement labels Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants