-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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): Fix arguments parsing method of Invoke-ExternalCommand()
#5839
Conversation
I initially planned to address it in #5124, but upon realizing that the PR was a refactoring, I decided to make it more manageable for review by separating the fix. |
I have tested it with almost all manifests broken by #5065 , including cygwin (dash format args) and miniconda (slash format args), this patch works in both PS5 and PS7. Those two manifests are still problematic - cygwin shows GUI and miniconda won't install to directory having a dash symbol, but that's nothing wrong with this PR, args were parsed all correctly. I don't see any existing app in offical buckets having an installer using args not starting with dash or slash. The regex is well crafted! |
Invoke-ExternalCommand()
Invoke-ExternalCommand()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any questions now, thank you for your work on this @niheaven.
Imo this is a critical part of the core therefore I suggest one more reviewer on this, but I don't know if any member is currently available. Please review when you are available. Good luck! /cc @ScoopInstaller/maintainers
I will merge this to develop after 24 hours for further testing. This seems to be the more appropriate approach. |
Description
Final PR for stable version (#5424)
Before handling the argument list, it splits by finding spaces that are not in a path or before params (starting with
/
or-
) and adding them toProcess.StartInfo.ArgumentList
orProcess.StartInfo.Arguments
.Example: (https://regex101.com/r/HynlRG/1)
Motivation and Context
Invoke-ExternalCommand
#5065How Has This Been Tested?
Test with the following manifests, and scoop path with/without spaces.
msmpi
ida-free
Checklist:
develop
branch.