Postinstall not fit for Windows

waiblinger commented 2 years ago

Hi, since the update yesterday I can't install my modules, since the postinstall script uses "||", which isn`t supported on Windows in the given context.

`At line:1 char:53
+  node -e "try{require('./_postinstall')}catch(e){}" || exit 0
+                                                     ~~
The token '||' is not a valid statement separator in this version.
    + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : InvalidEndOfLine`

medikoo commented 2 years ago

@waiblinger thanks for the report, what terminal are you using on Windows? I was actually testing it on Windows prior release and all worked well.

waiblinger commented 2 years ago

I use the integrated terminal in Visual Studio Code (so cmd). The project ran yesterday befor the changes.

waiblinger commented 2 years ago

@medikoo I tried powershell as well. Same problem

ilyachalov commented 1 year ago

@medikoo I'm trying to install 'gulp-cli' from 'npm' and I have the same problem (Windows 10):

windows10-powershell

|| is pipeline chain operator. They appeared in 'PowerShell 7':

https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pipeline_chain_operators

'Windows Powershell' (version 5.1) comes with 'Windows 10', executable file 'powershell.exe'. 'Powershell 7' can be obtained separately, executable file 'pwsh.exe'. 'Windows Powershell' (version 5.1) and 'Powershell 7' installed side by side.

'Windows Powershell' (version 5.1) does not have an pipeline chain operator ||.

I think the line powershell -c node -e "try{require('./_postinstall')}catch(e){}" || exit 0 need to replace with pwsh -c node -e "try{require('./_postinstall')}catch(e){}" || exit 0

Then it will work for those who have installed 'PowerShell 7'.

medikoo commented 1 year ago

I think the line powershell -c node -e "try{require('./_postinstall')}catch(e){}" || exit 0 need to replace with pwsh -c node -e "try{require('./_postinstall')}catch(e){}" || exit 0

It's actually an npm that adds powershell -c, so it'll be good to confirm if that happens with latest npm version, and eventually report issue over there.

ilyachalov commented 1 year ago

@medikoo Thanks for the reply. I've already figured it out.

npm can be configured in command line:

> npm config set script-shell="pwsh"

And now I have succeeded (I also installed 'PowerShell 7'):

windows10-pwsh-ok

Thanks again. 🎉🎉🎉

mrJli commented 1 year ago

Windows 11, PowerShell 7.2.6, i try this npm config set script-shell="pwsh" but

ilyachalov commented 1 year ago

@mrJli Are you really running the installation in PowerShell? PowerShell command prompt usually looks like this PS C:\>. You have the command prompt shown as $. Perhaps you are using 'Git Bash' (or some other shell) rather than 'PowerShell'?

Can you show a screenshot without cropping the window title?

mrJli commented 1 year ago

@mrJli Are you really running the installation in PowerShell? PowerShell command prompt usually looks like this PS C:\>. You have the command prompt shown as $. Perhaps you are using 'Git Bash' (or some other shell) rather than 'PowerShell'?

Can you show a screenshot without cropping the window title?

ilyachalov commented 1 year ago

@mrJli Earlier it was about a mistake with operator ||. To fix this error I used 'PowerShell 7' and 'npm' setting npm config set script-shell="pwsh".

You have a different situation. You have a unary operator --, which is not needed there:

pwsh -c --  node -e "try{require('./_postinstall')}catch(e){}" || exit 0

'es5-ext' is responsible for part of the command: https://github.com/medikoo/es5-ext/blob/main/package.json

 node -e "try{require('./_postinstall')}catch(e){}" || exit 0

I think 'npm' is responsible for the initial part:

pwsh -c --

Maybe it's a bug in npm: https://github.com/npm/cli/issues/5332

What version of 'npm' do you have (npm --version, I have 8.12.1)? Maybe you should use a different version of 'npm'.

mrJli commented 1 year ago

npm version - 8.19.2 i try to install 8.12.1 and all works, thanks

rjwut-chg commented 1 year ago

I have zero issue with the message and intent behind the postinstall script, but perhaps unintended consequences like this are a good argument for limiting that sort of thing to the README.md file instead.

kieselai commented 11 months ago

This was quite frustrating to figure out, since I was calling my script from PowerShell core, but this postinstall script was still being called from PowerShell 5. I haven't had issues on any other machine I've ran it on, but this one apparently didn't have pwsh configured as the "script-shell".
Results on Google simply suggested using the compatible syntax for PowerShell 5, which isn't helpful when it's not a script I'm executing directly.
It wasn't until going directly to this repo and looking at the open issues that I saw what the solution was. It's obvious in hindsight, but was a bit of a runaround getting here.
I know cross compatibility with windows is a pain, but this is the first package I've ran into this particular issue.

If it helps, here are the versions of software/libs I'm running: PowerShell: 7.3.4 Node: v18.15.0 pnpm: 8.6.3 npm: 9.4.0

And the solution for me was to set pwsh as the script-shell: npm config set script-shell="pwsh"

kieselai commented 11 months ago

The more I look into it, I think the specific problem might occur if PowerShell 5 (or earlier) was previously specified as the script-shell. cmd.exe actually does allow "||" and "&&" operators, so I think the problem is specifically with older versions of PowerShell being the default script-shell.

kieselai commented 11 months ago

It would have been PowerShell 6 and earlier with the problem, my mistake