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

draft of how to enable shell integration with commands #348

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SteveL-MSFT
Copy link
Member

No description provided.

Comment on lines 139 to 141
The JSON manifest file name would be `<name>.command.json`.
The `<name>` portion is only used to differentiate between multiple manifests and does not need to match the executable name,
but it is recommended to do so.
Copy link
Member

Choose a reason for hiding this comment

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

A JSON manifest file would be placed alongside the executable (or technically anywhere within $env:PATH)

If manifest files don't have to be placed alongside the executable, then the <name> part should be exactly the executable name, otherwise there is no easy way to tell if a xxx.command.json file corresponds to an executable. Trying to read the execuable key from every .command.json file along the way of searching in PATH would be too inefficient.

IMHO, the manifest file name should be <executable-name>.<optional-part-for-differentiating-multiple-manifest>.command.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your proposal seems reasonable. The key thing is that at runtime, the executable is from the manifest content and not the filename of the manifest. I don't know how often executable names collide, but it seems like it would be rare. A convention like: <executable>.<org/author>.command.json seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this to update


### Command shell integration manifest

The JSON manifest file name would be `<name>.command.json`.
Copy link
Member

Choose a reason for hiding this comment

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

maybe use the name <--->.shell.json

Copy link
Member Author

Choose a reason for hiding this comment

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

This manifest is for the command and not the shell, though.

Copy link
Member

Choose a reason for hiding this comment

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

It's for shell integration though -- its purpose is for a shell to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe <>.shellintegration.json?

Draft-Accepted/RFCXXXX-Command-Shell-Integration.md Outdated Show resolved Hide resolved

In the case of PowerShell, predictors and feedback providers are assemblies so the value is the path to the assembly
instead of a `.ps1` script.
To enable support for MSIX installed tools and not load the dll directly from their location, PowerShell will
Copy link
Member

Choose a reason for hiding this comment

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

The API to resolve the actual location of a MSIX application is undocumented IIRC. Is there a documented API available today?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had resolved this with the APPX team. I'll follow-up so we can use this.


PowerShell has an extensibility model for Predictors and Feedback Providers.
Typically, these are implemented as modules with a cmdlet to register them with the PowerShell engine.
In this case, the extension would still need to be managed code, but would be automatically discovered and loaded by the shell.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what you are proposing here. It sounds like PowerShell already has an extension mechanism for predictors and other providers (I assume you mean ISubsystem and if so you should probably just say it), and that non-PowerShell shells should have a similar extension mechanism but not managed code?

Copy link
Collaborator

@StevenBucher98 StevenBucher98 May 23, 2023

Choose a reason for hiding this comment

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

I am also confused what is being proposed here. We ultimately want auto registration of particular feedback providers and predictors right? Is this something shell integration is going to do or how does this related to native commands


### Discovery

A JSON manifest file would be placed alongside the executable (or technically anywhere within `$env:PATH`) and discovered by the shell.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make it clear that JSON file is for a individual command line tool. Is there a naming convention that must be followed? How is the manifest file distinguished from other possible json files?

Suggested change
A JSON manifest file would be placed alongside the executable (or technically anywhere within `$env:PATH`) and discovered by the shell.
A tool specific JSON manifest file would be placed alongside the tool executable (or technically anywhere within `$env:PATH`) and discovered by the shell.

to a different location.
Shells should look for the manifest file in the target location of a symlink or reparse point in addition to within `$env:PATH`.

In the case that multiple JSON manifests are for the same executable, the first one wins.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a simple tool install path example to illustrate path options where manifest files are probed for.

}
```

The mandatory fields are:
Copy link
Contributor

Choose a reason for hiding this comment

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

When are manifest files probed for ... on first time tool execution? Is tool manifest information cached? I assume manifest validation is performed at that time and an error is displayed if validation fails? Will there be a shell command to load/validate a tool manifest file (for tool development/debugging purposes)?

}
```

Multiple shells can be specified and up to the shell to determine which one to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Multiple shells can be specified and up to the shell to determine which one to use.
Multiple shells can be specified and it is up to the shell to determine which one to use.

To enable support for MSIX installed tools and not load the dll directly from their location, PowerShell will
copy the dll to the user's folder if it is not already there (SHA256 hash comparison) and load it from there:

- On Unix, the dll will be copied to `$HOME/.local/share/powershell/Predictors` and `$HOME/.local/share/powershell/FeedbackProviders`
Copy link
Contributor

Choose a reason for hiding this comment

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

At what point are the tool binaries copied? I hope it is not done at tool runtime. I feel we should distinguish between tool runtime and set-up time, so that there is a separate tool registration step when predictors/providers are registered.

Otherwise, I feel some IT users will feel uncomfortable having binaries copied on their systems, without their knowledge.


### PowerShell opt-out of automatic registration of extensions

Users may want to selectively disable the automatic registration of extensions for a tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should separate extension registration from extension loading. I am not sure what 'auto registration' means. If registration means copying binaries, I think enterprise users may be uncomfortable with it.

PowerShell has an extensibility model for Predictors and Feedback Providers.
Typically, these are implemented as modules with a cmdlet to register them with the PowerShell engine.
In this case, the extension would still need to be managed code, but would be automatically discovered and loaded by the shell.
Since the user installed the tool, the expectation is they want to use it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this statement is exclusive to tools that hope to register with feedbackprovider and or predictors?

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.

7 participants