8000 Make 'Start-Job' throw terminating error when PowerShell is being hosted by daxian-dbw · Pull Request #9128 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Make 'Start-Job' throw terminating error when PowerShell is being hosted#9128

Merged
adityapatwardhan merged 3 commits intoPowerShell:masterfrom
daxian-dbw:error
Mar 14, 2019
Merged

Make 'Start-Job' throw terminating error when PowerShell is being hosted#9128
adityapatwardhan merged 3 commits intoPowerShell:masterfrom
daxian-dbw:error

Conversation

@daxian-dbw
Copy link
Member
@daxian-dbw daxian-dbw commented Mar 12, 2019

PR Summary

Related: #7052

The pwsh executable is not available under $PSHOME in the scenarios where PowerShell is being hosted in other applications, like PowerShell Azure Functions.
In those cases, Start-Job won't work as there is no pwsh for us to reliably start a out-of-proc Runspace. So make Start-Job throw a terminating error in such cases and recommend ThreadJob module.

Also suppressed -HostName and -SubSystem parameters that are incorrectly exposed by Start-Job.

PR Context

PR Checklist

@daxian-dbw daxian-dbw requested review from SteveL-MSFT and TylerLeonhardt and removed request for BrucePay and mirichmo March 12, 2019 22:36

if ((!string.IsNullOrEmpty(procArch)) && (procArch.Equals("amd64", StringComparison.OrdinalIgnoreCase) ||
procArch.Equals("ia64", StringComparison.OrdinalIgnoreCase)))
if (!string.IsNullOrEmpty(procArch) && (procArch.Equals("amd64", StringComparison.OrdinalIgnoreCase) ||
Copy link
Member

Choose a reason for hiding this comment

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

@adityapatwardhan does this need to be updated for ARM?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but what is the expected behavior on using useWow64 when run on arm64?

Copy link
Member Author
@daxian-dbw daxian-dbw Mar 12, 2019

Choose a reason for hiding this comment

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

I don't think we should support the -RunAs32 parameter on Start-Job anymore. To start a 32-bit process pwsh, it requires the win-32 pwsh package to be installed, and there is no way to guarantee that, and I don't think we should even bother to search for that installation.

If you look at the current implementation closely, you will find that when useWow64 is true, we are actually using the same default pwsh under $PSHOME. So it's already broken. No one reports it because no one is using it.

I think when -RunAs32 is specified in Start-Job, we check if the current process is 64-bit, if so, we throw terminating error. If not, then we continue to run because the underlying pwsh is a 32-bit executable anyways. But this should be done by a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the -RunAs32 is probably not used much anymore since the world has moved to 64-bit and makes even less sense for pwsh

Copy link
Member

Choose a reason for hiding this comment

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

We can do that in a separate PR and not block this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I will submit a separate PR for that.

Copy link
Member
@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan adityapatwardhan merged commit cdb28a1 into PowerShell:master Mar 14, 2019
@daxian-dbw daxian-dbw deleted the error branch March 14, 2019 01:06
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 16, 2019
@TravisEz13 TravisEz13 added this to the 6.2.0-GA-Consider milestone Mar 18, 2019
@TravisEz13 TravisEz13 modified the milestones: 6.2.0-GA-Consider, 6.2.0 Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

0