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
Native invocation using ArgumentList
#14692
Native invocation using ArgumentList
#14692
Conversation
/cc @mklement0 Please look the PR. |
|
For the variable, I prefer |
Not sure if it's just a typo, but to be clear: this isn't about argument parsing, it's about argument passing (which, on Windows only, also involves encoding, as a single string - which
I can see why you would want to avoid the word "fix" in the variable name - even though that's what it comes down to. Again, what matters to the user is only the following - not technical characterizations based on plumbing users may not even know of:
Thus, if you do want an
|
7f6581d
to
dcb79f5
Compare
@PowerShell/powershell-committee reviewed this, we agreed on automatic variable |
dcb79f5
to
0985d70
Compare
test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1
Show resolved
Hide resolved
eb105bd
to
337877e
Compare
da24160
to
e4f7558
Compare
src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs
Outdated
Show resolved
Hide resolved
test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1
Show resolved
Hide resolved
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.
LGTM
@daxian-dbw Can you update your review? |
src/System.Management.Automation/engine/NativeCommandParameterBinder.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/NativeCommandParameterBinder.cs
Outdated
Show resolved
Hide resolved
@mklement0 Do you want to review the PR? |
…Binder.cs Co-authored-by: Ilya <darpa@yandex.ru>
…Binder.cs Co-authored-by: Ilya <darpa@yandex.ru>
@PoshChan Please remind me in 1 hour |
@TravisEz13, this is the reminder you requested 1 hour ago |
Since this PR introduces a breaking change anyway and is being implemented as an experimental feature, I would like to understand why don't we implement mklement0's proposal in full as an experimental feature and close the long history?
Also I asked previously and still wonder why @mklement0 is not in a PowerShell Committee. I believe MSFT team would have saved a lot of time and effort in this case. |
@iSazonov can you open a new issue w.r.t. #14692 (comment) and mark it for committee review? |
@PoshChan Please remind me in 1 Hour |
The (hopefully) final proposal is now in #14747 (comment) |
@TravisEz13, this is the reminder you requested 1 Hour ago |
ArgumentList
🎉 Handy links: |
[7.2.0-preview.5] - 2021-04-14 * Breaking Changes - Make PowerShell Linux deb and RPM packages universal (#15109) - Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035) - Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!) * Experimental Features - `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692) * Engine Updates and Fixes - Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!) * General Cmdlet Updates and Fixes - Fix SSH remoting connection never finishing with misconfigured endpoint (#15175) - Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969) - Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048) - Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!) - Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!) - Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!) - Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!) - Allow `Set-Clipboard` to accept empty string (#14579) - Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943) - Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077) * Code Cleanup <details> <summary> <p>We thank the following contributors!</p> <p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p> </summary> <ul> <li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li> <li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li> <li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li> <li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li> <li>Remove unnecessary <code>Array</code> -> <code>List</code> -> <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li> <li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li> <li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li> <li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li> </ul> </details> * Tools - Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!) * Tests - Add the missing tag in Host Utilities tests (#14983) - Update `copy-props` version in `package.json` (#15124) * Build and Packaging Improvements <details> <summary> <p>We thank the following contributors!</p> <p>@JustinGrote</p> </summary> <ul> <li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li> <li>Make package validation regex accept universal Linux packages (#15226)</li> <li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li> <li>Make MSI and EXE signing always copy to fix daily build (#15191)</li> <li>Sign internals of EXE package so that it works correctly when signed (#15132)</li> <li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li> <li>Update daily release tag format to work with new Microsoft Update work (#15164)</li> <li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li> <li>Treat rebuild branches like release branches (#15099)</li> <li>Update WiX to 3.11.2 (#15097)</li> <li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li> <li>Allow patching of preview releases (#15074)</li> <li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li> <li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li> <li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li> </ul> </details> * Documentation and Help Content - Merge `7.2.0-preview.4` changes to master (#15056) - Update `README` and `metadata.json` (#15046) - Fix broken links for `dotnet` CLI (#14937) [7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
This adds a new behavior for native executables which uses ArgumentList as the command arguments for the native executable.
PR Summary
The experimental feature PSNativeCommandArgumentPassing when enabled will use the
ArgumentList
property of theStartProcessInfo
object rather than our current mechanism of reconstructing a string when invoking a native executable.$PSNativeCommandArgumentPassing
is a new automatic variable which allows for runtime selection of behavior.$PSNativeCommandArgumentPassing
may be set to eitherLegacy
orStandard
,Legacy
is the historic behavior. The default when the experimental feature is enabled is the newStandard
behavior.This new behavior is a breaking change from current behavior. This may break scripts and automation which work-around the various issues when invoking native applications. Historically, quotes must be escaped and it is not possible to provide empty arguments to a native application. This PR attempts to address these issues.
new behaviors made available by this change
the new behavior does not change invocations which look like this:
Additionally, parameter tracing is now provided so
Trace-Command
will provide useful information for debuggingPR Context
Invoking native applications is sometimes very problematic because of how the application is invoked. A new property is available on the
ProcessStartInfo
object which hopes to improve the experience. This PR takes advantage of this new property.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.The impact on completions may be affected if those completers are calling native applications.