Repository navigation
Fix use of UseShellExecute for .NET Core - #1291
Conversation
There was a problem hiding this comment.
Mike Richmond (@mirichmo) can anything else be done here?
There was a problem hiding this comment.
Quoc Truong (@quoctruong) this is in PackageManagement. Do you need to do something alternative here?
There was a problem hiding this comment.
Quoc Truong (@quoctruong), Raghu Shantha (@raghushantha) I'm changing it for now since it will fail at runtime otherwise.
There was a problem hiding this comment.
Quoc Truong (@quoctruong), Raghu Shantha (@raghushantha) You guys are more familiar with this code, can you please comment on how UseShellExecute = true can be avoided here for CoreCLR?
There was a problem hiding this comment.
Quoc Truong (@quoctruong) Raghu Shantha (@raghushantha) please be aware this might break you.
There was a problem hiding this comment.
Andy Jordan (@andschwa), Dongbo Wang (@daxian-dbw) The change looks good to me.
There was a problem hiding this comment.
Andy Jordan (@andschwa), Dongbo Wang (@daxian-dbw) - Only the MSU provider uses this code path. And MSU is N/A on Core platforms.
So the change is ok.
There was a problem hiding this comment.
Thanks for verifying 😄
|
While the documentation states this fact; the code reveals a different fact. |
fc0cd34 to
810b3de
Compare
|
Dongbo Wang (@daxian-dbw), Mike Richmond (@mirichmo) This ports |
There was a problem hiding this comment.
I know this looks weird. Having quotes inside of a string is blech. Since it's a string literal, each internal "" is a " so this gets interpolated as /c "start /b "" {0}" where the first "" is the empty optional title.
There was a problem hiding this comment.
This command doesn't work on NanoServer/IoT. This is not supposed to work on NanoServer/IoT anyways, but we need a way to detect that browser is not available, and throw the appropriate error message in that case.
One option is to check reg key on windows to find the default browser, and use the command of the default browser to open the uri. If the reg key is not there, then that means it's NanoServer/IoT.
The reg key indicating the default browser in windows is:
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\.html\UserChoice { ProgId }
get the value of ProgId and HKEY_CLASSES_ROOT\<ProgId-Value>\Shell\Open\command points to the executable.
There was a problem hiding this comment.
Using cmd start is to avoid having to check the registry. I'm happy to leave this unimplemented for Windows and throw a platform not supported exception until the Nano team can do this properly.
|
There is no Nano powershell team anymore, there is only one powershell team now. |
0c99a04 to
2332532
Compare
There was a problem hiding this comment.
Minor comment: It's always false in CoreCLR, so this line can be removed.
There was a problem hiding this comment.
This default is really poorly documented. That is to say, the documentation lies, so I thought I should be explicit. I can change it.
|
LGTM except for some minor comments and the failed CI build. |
|
Dongbo Wang (@daxian-dbw) the failing build is due to Travis updating the OS X images, and is unrelated to these changes. I'm currently investigating. Thanks for the review. |
2332532 to
cd718c9
Compare
Since `UseShellExecute` cannot be used.
To re-enable elevation, the process will need to fork. For now, don't fail fatally.
These currently fail at runtime on Nano, and will continue to do so. But this implements these much needed cmdlets for Open PowerShell. If there is a way to determine that PowerShell is being run on the Nano OS at runtime, then we could instead throw a platform unsupported exception, but this cannot be based on the target compilation framework, as .NET Core is shared for both Nano and Windows.
cd718c9 to
222cf1e
Compare
|
|
Thanks 222cf1e to
33bd17a
Compare
33bd17a to
50e6627
Compare
|
|
…xecute Fix use of UseShellExecute for .NET Core
UseShellExecuteis not supported on .NET Core because it is only MTA.