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

feat(core): Improve argument concatenation in Invoke-ExternalCommand #5065

Merged

Conversation

lewis-yeung
Copy link
Contributor

Description

Use ArgumentList in PowerShell 6.1 and later (built on .NET Core 2.1+), otherwise escape arguments using the general MSVC Runtime's rules in older versions.

In the past, due to the arbitraryness of arguments, we are not able to handle all cases where arguments may contain spaces, quotes or other special characters for every automatic installer script that uses Invoke-ExternalCommand. With this change, we don't have to escape arguments manually for most external commands any more. But for the following exceptions who basically don't follow the general MSVC Runtime's rules of parsing command line, just simply concatenate arguments with spaces to build the argument list string, which means we have to escape arguments manually for them: (case insensitive)

  • Well-known executables: cmd.exe, cscript.exe, wscript.exe and msiexec.exe.
  • Files with these suffixes: .bat, .cmd, .js, .vbs and .wsf.

Ref: MS docs

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

lib/core.ps1 Outdated Show resolved Hide resolved
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

LGTM, but may need more tests in develop

@niheaven niheaven merged commit 288aee9 into ScoopInstaller:develop Jul 25, 2022
@lewis-yeung lewis-yeung deleted the improve-arg-concatenation branch July 25, 2022 16:31
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
escape char `\" is not needed anymore
related: #9026
related: ScoopInstaller/Scoop#5065
@niheaven
Copy link
Member

And thank @issaclin32 for the fixes of incompatibility. 😭

issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Main that referenced this pull request Aug 11, 2022
* crc changed their file structure, need to modify accordingly
* escape char `\" is not needed anymore
* related: ScoopInstaller/Extras#9026
* related: ScoopInstaller/Scoop#5065
issaclin32 added a commit to ScoopInstaller/Main that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Main that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 11, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 19, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Aug 19, 2022
issaclin32 added a commit to ScoopInstaller/Versions that referenced this pull request Aug 19, 2022
* related: ScoopInstaller/Scoop#5065
* related: ScoopInstaller/Extras#9060
* Remove autoupdate because last update was in 2019
issaclin32 added a commit to ScoopInstaller/Versions that referenced this pull request Aug 19, 2022
* related: ScoopInstaller/Scoop#5065
* related: ScoopInstaller/Extras#9060
* Remove autoupdate because last update was in 2019
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 21, 2022
Anki's installer does not work with quoted args
related: ScoopInstaller/Scoop#5065
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 21, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 21, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 21, 2022
issaclin32 added a commit to ScoopInstaller/Nonportable that referenced this pull request Aug 21, 2022
The app's installer does not work properly with quoted args
related: ScoopInstaller/Scoop#5065
issaclin32 added a commit to matthewjberger/scoop-nerd-fonts that referenced this pull request Aug 24, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Sep 1, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Sep 1, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Sep 2, 2022
issaclin32 added a commit to ScoopInstaller/Extras that referenced this pull request Sep 2, 2022
@chawyehsu
Copy link
Member

Can we revert this? It's a broken implementation.

@niheaven
Copy link
Member

Revert it in develop, do a release, and then try to make it more robust. Is it okay? @ScoopInstaller/maintainers @lewis-yeung

@lewis-yeung
Copy link
Contributor Author

@niheaven Seems to make sense. But what about those manifests that have already been adjusted for this feature? Or if you already have an idea on how to make a bidirectionally compatible implementation?

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.

3 participants