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

PSNativeCmdlet class - wrapper for native applications #13428

Closed
iSazonov opened this issue Aug 13, 2020 · 21 comments
Closed

PSNativeCmdlet class - wrapper for native applications #13428

iSazonov opened this issue Aug 13, 2020 · 21 comments
Labels
Issue-Enhancement the issue is more of a feature request than a bug WG-Interactive-Console the console experience

Comments

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 13, 2020

Motivations

Very long history and fears of inevitable breaking changes

#1995
#7388
#1908
#13068
#12491
#12975
#10722
#10741
#9006
#3996
#13393
#13579
#13640 (comment) PSNativePSPathResolution
#10509 (comment)
#9343

PowerShell/PowerShell-RFC#90
PowerShell/PowerShell-RFC#88

It seems to be not all

Proposal

Introduce new PSNativeCmdlet class as an successor of the PSCmdlet class which will work as a wrapper for native apps.

Capabilities

All traditional cmdlet capabilities:

  • Parameter completion
  • Strong typed parameters
  • Argument validation attributes
  • Transformation attributes
  • Parameter sets
  • Dynamic parameters

Additionally:

  • input/output encodings
  • specific error output handling
  • specific error code handling
  • specific argument parsing, escaping (requires a hook in Binder)
  • output parser (for strong typed output to PowerShell pipeline) Feature: Register-OutputParser #10741
  • having special wrappers for specific applications like cmd.exe or msiexec
  • specific hook for Suggestion System
  • alternative optimizations for native pipelines

Benefits

The main thing is that this class can be integrated into the Engine subsystems selectively and gradually without total breaking changes.
And all subsystems themselves can be enhanced gradually.
All existing Engine possibilities remain. Wrappers can overlap its or bypass to its.

The class itself can also improve gradually.

We can add flags which subsystems a particular wrapper can support. For example say that only Completions. Or run it like a regular cmdlet. Or run it as a normal external command, but use the output parser.

We could add new option in Start-Process so that it uses capabilities of the new class too.

It is compiled and works fast. (We could think about script support too)

Easy adoption

Easy adoption for #1995 - We can implement @mklement0's Invoke-NativeShell internally in PSNativeCmdlet. Users can load a module with wrappers and get new behaviors or unload the module and get previous behavior. Or we could add flags in PSNativeCmdlet which turn on/off capabilities on the fly for the specific wrapper.
Git is a good example of gradual adoption. We could implement git wrapper to address argument parsing issues and it will work with posh-git module. Later we could enhance the wrapper with posh-git capabilities in some way.

Based on telemetry we will able to decide when to make the new behavior as default for all.

We can collect wrappers for the most popular commands in a separate standard module and a separate PowerShell project repository. The community will be able to quickly add wrappers and improve them. Other vendors and communities may publish modules with their wrappers for their applications.

Additional thoughts

This approach could address questions in @JamesWTruher's blog posts about how PowerShell can take better advantage of native executables
https://devblogs.microsoft.com/powershell/native-commands-in-powershell-a-new-approach/
https://devblogs.microsoft.com/powershell/native-commands-in-powershell-a-new-approach-part-2/

Since all native applications are built without a single parameter schema, these wrappers seem to be the only universal solution.

For Windows OS MSFT could make https://github.com/dotnet/command-line-api standard for all OS utilities. As result, PowerShell could benefits from this automatically loading meta data from managed dll-s.

@iSazonov iSazonov added Issue-Enhancement the issue is more of a feature request than a bug WG-Engine core PowerShell engine, interpreter, and runtime labels Aug 13, 2020
@iSazonov
Copy link
Collaborator Author

Don't replicate ALL discussions to the issue - please continue discussions in specific issues.

@vexx32
Copy link
Collaborator

vexx32 commented Aug 14, 2020

I'm not really fully clear what this all means, but inferring what I can, it sounds like the idea is to build a whole new command processor to handle this -- among several other related tie-ins to existing subsystems.

If I'm honest, I think this is probably massively complicated to approach in this manner. It may well be a more complete approach to resolving the problems we're seeing, but I think the necessary time to put this kind of change together and review it, and then figure out how to test it... would be pretty significant.

I'm not sure the slated benefit is worth the level of complexity being proposed here. 🤔

@iSazonov
Copy link
Collaborator Author

I'm not sure the slated benefit is worth the level of complexity being proposed here.

You see a complicity only because all features from different issues we want implement are enumerated at once.
Really every feature will implemented separately - so it is the same complicity as implementation of one feature. The same for testing.
For example, a parameter completion is as simple as define primitive cmdlet. I think to create a demo for this.

@vexx32
Copy link
Collaborator

vexx32 commented Aug 14, 2020

To be clear - the surface features I think are desirable.

But I don't think the proposed implementation is the way to go, there seem to be too many pieces to fix what really boil down to 2-3 much simpler requests:

  1. Argument parsing
  2. Tab completion
  3. Alternate ways of handling stderr/stdout

In my own opinion, it's not worth duplicating significant portions of the engine code. These issues are, effectively, either bugs or feature requests that can be resolved in a much less expensive fashion -- if we're willing to tackle the issue of them introducing breaking changes here and there.

It's my opinion that this approach is significantly more expensive in all respects than simply taking the necessary cost of a couple of breaking changes.

@iSazonov
Copy link
Collaborator Author

In my own opinion, it's not worth duplicating significant portions of the engine code.

Where do you see "duplications" in Engine? The proposal says about (1) step-by-step (2) injection in some Engine subsystems.

Why do you say about complicity the work?

  • What about simplicity for consumers? It is very simple for user to add one method in the class and get desired behavior without thinking about how Engine works.
  • Maybe it is complex for me but not for MSFT experts. You know MSFT team has limited resources and as result postponed many useful features. But they could inject the new class in completion code and community get possibility to add completers for native commands. Then they could inject the class in pipeline output and community get possibility to create output parsers for native command. And so on. Mainly - small MSFT team efforts lead to significant ecosystem improvements.

@rkeithhill
Copy link
Collaborator

rkeithhill commented Aug 14, 2020

I can't speak to the complexity of the implementation of a PSNativeCmdlet type but the issues around using native exes have been around for 15 years. I don't want to see another "bandaid" (ahem --%) applied. So, whatever gets done, I'd love to see that adequate time & thought is given to the implementation the team comes up with. But please, let's not add yet another bandaid that will require additional fixes down the road and leave users with more bandaids to determine if they should use or not.

I realize you can't anticipate everything. I suspect the team working on v1 never anticipated running on macOS and Linux. But given everything we do know, let's make sure the solution is well thought out and implemented even if it winds up being complex. Honestly, I'd rather wait a bit longer to really nail these native exe issues. In the meantime, modules like @mklement0's native could help fill the gap.

@yecril71pl
Copy link
Contributor

That would require command providers to bundle manifests that PowerShell can read.

@iSazonov
Copy link
Collaborator Author

I don't want to see another "bandaid" (ahem --%) applied.

@rkeithhill With the proposal end users will run native applications as is without any additional --% and cmdlets. They only have to load the module with wrappers.

@mklement0
Copy link
Contributor

mklement0 commented Aug 16, 2020

@iSazonov, command-individual wrappers are not the right solution to this problem.

We don't need to wrappers for external executables, we just need argument passing to work, fundamentally, with any executable.

To put it differently, quoting the first part of the linked blog series:

PowerShell is a great shell, it can execute any executable, the same way that any good shell can do. No change is needed, just run kubectl and you’re done!

It's precisely because that is not true that we need a fundamental fix - and that is the focus of #1995.

#1995 (comment) now lays out the fix in detail:

  • which is trivial and complete on Unix
  • which would be trivial on Windows if we limited ourselves to supporting executables that adhere to the MS C/C++ conventions ...
    • ... but since calling calling batch files and msiexec-style CLIs, neither of which adhere to the convention, is common, we should accommodate them too
    • for any - then truly rare - edge cases not covered by these accommodations, there's --% or the more flexible ins (Invoke-NativeShell).

The only question is: how do we make this fix available, if a direct fix is - at least for now - not an option?

To be clear: a direct fix is clearly the most desirable option, given that it is a severe handicap for a shell to not properly pass arguments to external executables.

Shipping an ie function as a stopgap that can later become a no-op once a direct fix is implemented is a low-ceremony (opt-in, per-call) solution that is easy to use, publicize and document:


As for the other areas of functionality mentioned in this proposal, as summarized by @vexx32 above:
They would be enhancements, as opposed to the urgently needed bug fix required for #1995:

  1. Tab completion

You don't need a new, cmdlet-derived class to get tab completion with external executables; for instance, @bergmeister's posh-cli module bundles a growing number of tab-completion modules for popular CLIs.

  1. Alternate ways of handling stderr/stdout

PowerShell/PowerShell-RFC#88 should address the fundamental lack of integration of external executables with PowerShell's error handling.

#13361 just fixed the problem with 2> redirections from external executables unexpectedly being sensitive to $ErrorActionPreference = 'Stop'; a related fix regarding $? is pending: #13393

#4332 would give us the ability to capture stderr output in a variable.

Overall, I'm not convinced we need engine-level support for wrapping the functionality of executables, such as parsing their text output into objects.

@iSazonov
Copy link
Collaborator Author

@mklement0 All you say about #1995 right in thoughts but... PowerShell lives with this over 15 years. And you know MSFT team said a day before - "we postpone this until 7.2". You can read this "We will think about it next year". It's a polite form to say NO. So you have two alternatives - (1) stop posting about this until 7.2 (and then wait 7.3, 7.4...) (2) find an compromise solution.
And again - I don't want to turn this issue into an endless discussion - they are already there.

This issue is also about in-depth refactoring. Today @daxian-dbw and MSFT team are thinking/working on SMA refactoring to loadable subsystems. As part of the work they could thing about refactoring these subsystems because this code is old and overgrown with extensions over the years. I suppose its partial refactoring is inevitable when creating subsystems. PSNativeCmdlets proposal is a way to make internal and external design simpler and clearer without breaking changes and the implementation faster and more compact. I believe this proposal has even more possibilities than announced.

@mklement0
Copy link
Contributor

mklement0 commented Aug 16, 2020

@iSazonov

PowerShell lives with this over 15 years.

Indeed, and any day longer is unacceptable.

(1) stop posting about this until 7.2

I don't think there's any reason to wait; if this really won't happen until 7.2, then we have more time for discussing and honing proposals, though I think the above proposal - which took a while to work out in detail - is stable now.

In general, I believe in describing what (debatably) should be the right solution, even if the powers that be decide against it.

(2) find a compromise solution.

What I'm proposing is a compromise solution - one that's easy to implement in the short term.
It specifically doesn't preclude a direct, breaking fix later and is even forward-compatible with one.
It doesn't break any existing code while giving users a very simple way to get the correct behavior.

This issue is also about in-depth refactoring.

I don't see any connection between this proposal and #1995.
The changes required to fix the latter are limited to core functionality in NativeCommandParameterBinderController.cs and NativeCommandProcessor.cs.

@Luiz-Monad
Copy link

Luiz-Monad commented Oct 29, 2022

its been two years since the last comment - do you want to give some context of what happened here? was PSNativeCmdlet added? or was something else done? or is idea just being refused outright?

This is the status #15143 (comment)

They added $PSNativeCommandArgumentPassing='Windows' which is the compromise #13428 (comment)

It works correctly for most of the applications now (at least those that use CommandLineToArgvW), but you have to opt-in. But it has an exception for cmd.exe to use the old parsing, which does work if you pass a single string, the escaping go as is.

Its Not great, not terrible.

#15408
Commit
It was merged NativeCommandProcessor.cs L248

@mklement0
Copy link
Contributor

@Luiz-Monad

you have to opt-in

I can't speak to the official plans, but since the experimental feature was made a stable feature in preview.8,
maintaining backward compatibility would require opt-out, given that the $PSNativeCommandArgumentPassing value is currently Standard (Unix-like platforms) and Windows (Windows).

If these defaults are retained for the official v7.3 release, they amount to a substantial breaking change.


it has an exception for cmd.exe to use the old parsing, which does work if you pass a single string

Assuming you mean encoding all arguments to pass to an executable via a single string: No, you could never do that, and you won't be able to now.

Also, the exception with Windows in effect applies not just to cmd.exe, but also to the WSH executables and their associated extensions; the docs already reflect that: the $PSNativeCommandArgumentPassing preference variable

It's also worth repeating that the old parsing is horribly broken, and that continuing to force users to resort to it - which requires knowing the specifics of how it is broken and carrying that unfortunate legacy forward - was a terrible decision.

And let's not forget high-profile CLIs such as msiexec and msdeploy, which will continue to require obscure workarounds in order to ensure partial quoting of arguments - and implementing those workarounds with Standard or Windows in effect requires resorting to --% or calling via cmd /c (whereas existing workarounds will break, if the old behavior is opt-in).

@Luiz-Monad
Copy link

Luiz-Monad commented Oct 31, 2022

Assuming you mean encoding all arguments to pass to an executable via a single string: No, you could never do that, and you won't be able to now.

That's what I got from reading the source code with the changes, I thought it would in theory make the cmd works, but you are right, that won't solve the problem, its just keeping the old behavior so you don't have a breaking change when you use 'Windows', its the old broken behavior, not what it should be.

The way the code is written, there's no accounting to make the encoding of the single string right for the cmd, it doesn't really solve the problem. I overlooked it.

@Luiz-Monad
Copy link

Luiz-Monad commented Oct 31, 2022

If these defaults are retained for the official v7.3 release, they amount to a substantial breaking change

The default, if I'm understanding now, that it will be the correct behavior for anything that's not the cmd and others. And the compatibility behavior for the cmd. Its the worst of the solutions, but at least I can call cmake now correctly for example.

But it's not going to be a breaking change if you use windows, even if windows is the default and opt-out is Legacy.

It's also worth repeating that the old parsing is horribly broken, and that continuing to force users to resort to it - which requires knowing the specifics of how it is broken and carrying that unfortunate legacy forward - was a terrible decision.

That's what I though it would do when you choose the opt-in Standard on WindowsDesktop, but there's no code to make it right, it just does CommandLineToArgvW, which DOESN'T work for cmd, such a shame.

And yet again, if they ship it, as-is, they are going to make it worse by not providing the proper behavior. That enum is going to become Hell !

    public enum NativeArgumentPassingStyle
    {
        Legacy = 0, // legacy is legacy
        Standard = 1, // this is just CommandLineToArgvW, breaks everything basically
        Windows = 2, // this is CommandLineToArgvW for anything but `cmd`  on windows
        NowRealStandardThatWorksAndIsNotOptOut = 3 // does what your InvokeNative Powershell module does for `cmd` and `Standard` for others.
 /// all else is legacylegacy, this becomes opt-in behavior , windows becomes legacywindows and opt-out , what a freak way of dealing with versioning 
    }

Maybe they should just have published a new cmdlet (that's actually just ie) for that and called it a day.

@mklement0
Copy link
Contributor

mklement0 commented Oct 31, 2022

Hear, hear, @Luiz-Monad.
If you haven't already seen it, check out #6745

While I share your frustration with respect to the issue at hand, the situation isn't quite as dire as you made it sound - though undoubtedly still bad enough:

If the current defaults (Standard on Unix, Windows on Windows) make it into the stable v7.3 version:

Update: They did make into v7.3.0, but it looks like some version after v7.3.1 will revert to the legacy behavior by default and make the new, correct behavior opt-in rather than opt-out, but on Windows only - see #18694. That is, the future defaults will most likely be Standard on Unix, and Legacy on Windows, and the following has been updated to reflect this expected future behavior.

  • Impact on existing code, IF it employs workarounds for calls to external programs, namely for (a) arguments with embedded " chars. and (b) empty-string arguments ('')

    • Unix:
      • All existing workarounds will break, unless $PSNativeCommandArgumentPassing is set to Legacy.
    • Windows:
      • All existing workarounds will continue to function (unless $PSNativeCommandArgumentPassing is explicitly set to Standard or Windows).
  • Impact on future code:

    • Unix:

      • Life is good overall: argument-passing will finally work as it should, consistently, with all external programs.
    • Windows:

      • New code that expects the correct behavior will have to set $PSNativeCommandArgumentPassing = 'Standard' explicitly.

        • That is, future generations of PowerShell users are saddled with obscure legacy debt, namely the broken argument-passing behavior of the default Legacy mode.
      • With $PSNativeCommandArgumentPassing = 'Standard' set by the user (Windows mode isn't worth considering, because it builds on the obscure, broken old behavior, selectively, based on a hard-coded list of executables):

        • Argument-passing will work properly only with programs that follow the C++ command-line parsing rules - Standard will work as expected; from what I can tell, programs that use the CommandLineToArgv WinAPI function use those rules too.

        • For all other programs, i.e. those with special quoting / escaping requirements, which notably includes batch files and high-profile CLIs such as msiexec and msdeploy.exe:

          • Given that the proposed accommodations for these CLIs were not implemented, many more workarounds than necessary will continue to be required.

          • A fundamental challenge is to even recognize the need for such workarounds:

            • Given that batch-file entry points for CLIs aren't uncommon, script authors may not even be aware that they're calling a batch file and that workarounds are therefore needed.
            • For CLIs that require that certain arguments be partially double-quoted, it won't be obvious why calls with, say, FOO="bar baz" fail, given that PowerShell's behind-the-scenes re-quoting (to "FOO=bar baz" in this example) is hidden from view.
          • Workarounds (in non-Legacy mode) currently require --% or cmd /c.


Maybe they should just have published a new cmdlet (that's actually just ie) for that and called it a day.

That certainly would have made for a less painful situation, but it's hard to justify having to use a cmdlet / function for something as fundamental to a shell as calling external programs with arguments is.

Given that & is situationally already needed for calling external programs, perhaps a new operator would have been less hideous, but it would still be a very visible symbol (in both sense of the word) of the fact that the original functionality is broken; plus, it wouldn't be easy to come up with a new symbol (sequence), and introducing new metacharacters is always problematic.

It's also worth noting that the current preference-variable-based solution is inherently problematic, as all preference variables are, due to PowerShell's dynamic scoping: Unless their values are carefully managed in terms of their scope (saving and restoring values), it is easy to inadvertently impact unrelated code that runs in the same scope or descendant scopes.

@yecril71pl
Copy link
Contributor

Given that & is situationally already needed for calling external programs,

but only if the path to the external program is evaluated from an expression, which is needed because of the implicit yield; an explicit yield operator would have been a much cleaner concept

it would not be easy to come up with a new symbol (sequence)

&* (simulate argument vector) would be intuitive and unobtrusive
&/ (use command line) would eliminate --% but allow expansions; the responsibility of making the command line consistent with the syntax supported by the called tool would be on the caller

&/ CMD /C START '"HELLO WORLD"' PAUSE

@yecril71pl
Copy link
Contributor

yecril71pl commented Nov 1, 2022

That's what I though it would do when you choose the opt-in Standard on WindowsDesktop, but there's no code to make it right, it just does CommandLineToArgvW, which DOESN'T work for cmd, such a shame.

The responsibility to parse the command line is on the called tool. The standard command shell does not even have a concept of an argument list, it expects a dictionary of named options, so the standard procedure to split into an argument list does not apply and there is no shame about that. Named options are better than an argument list unless you really need to pass a homogeneous list of items. UNIX tools simulate parsing option dictionaries using a system‑imposed intermediate step of argument lists—which is a legacy feature and an inexhaustible source of funny misunderstandings and unexpected behaviour.

@Luiz-Monad
Copy link

While I share your frustration with respect to the issue at hand, the situation isn't quite as dire as you made it sound

Oh no, its not that dire, I can keep using your module any time. I'm just waiting for the proper solution, one day. I have hope still.

@Luiz-Monad
Copy link

Luiz-Monad commented Nov 9, 2022

The standard command shell does not even have a concept of an argument list, it expects a dictionary of named options, so the standard procedure to split into an argument list does not apply and there is no shame about that.

Well, any program can parse its single commandline string whatever way it wants, it is what it is. The shame is not having the feature like Invoke-NativeShell that serializes for us. That does this:

Script authors must understand when and how the old parsing was broken and what the specific workarounds are; that is, future generations of PowerShell users are saddled with obscure legacy debt.

Also, given that batch-file entry points for CLIs aren't uncommon, script authors may not even be aware that they're calling a batch file and that workarounds are therefore needed.

I'll keep using https://github.com/mklement0/Native . No worries !

@mklement0
Copy link
Contributor

Please see #18660 (comment) for the continuation of this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement the issue is more of a feature request than a bug WG-Interactive-Console the console experience
Projects
None yet
Development

No branches or pull requests

8 participants
@mklement0 @yecril71pl @Luiz-Monad @rkeithhill @SteveL-MSFT @iSazonov @vexx32 and others