Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix the indicator for redirecting standard input#2749

Merged
vors merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
daxian-dbw:nativecommanddaxian-dbw/PowerShell:nativecommandCopy head branch name to clipboard
Nov 22, 2016
Merged

Fix the indicator for redirecting standard input#2749
vors merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
daxian-dbw:nativecommanddaxian-dbw/PowerShell:nativecommandCopy head branch name to clipboard

Conversation

@daxian-dbw

@daxian-dbw daxian-dbw commented Nov 21, 2016

Copy link
Copy Markdown
Member

Fix #2745
PipelinePosition starts from 1, so the indicator for redirecting standard input should be PipelinePosition > 1.

When there is input to a native command like 1,2,3 | <native>, the native command is still marked as position 1 in the pipeline because the input doesn't take up pipeline position. So we cannot simply using PipelinePosition to decide whether standard input needs to be redirected.

The new fix swaps the running order of the following code in PipelineProcessor.Start, so as to make sure ExpectingInput is set before calling DoPrepare. Then in NativeCommandProcessor.Prepare we can use MyInvocation.ExpectingInput to check whether to redirect standard input.

commandProcessor.DoPrepare(psDefaultParameterValues);
myInfo.ExpectingInput = commandProcessor.IsPipelineInputExpected();

@daxian-dbw

Copy link
Copy Markdown
Member Author

Will add a test in a few mins.

@daxian-dbw

Copy link
Copy Markdown
Member Author

@vors @lzybkr From reading the code, I feel it's fine to swap the order of calling DoPrepare and assigning myInfo.ExpectingInput, but I would need to run all tests to verify. If you know something that will obviously be broken by this change, please let me know.

@daxian-dbw

daxian-dbw commented Nov 21, 2016

Copy link
Copy Markdown
Member Author

stty may not be on the Travis CI build. I will come up with a more robust test. Never mind, my typos caused the test failures. I will run all tests in both Linux and Windows to verify if swapping the order breaks anything.

myInfo.PipelineIterationInfo = pipelineIterationInfo;
commandProcessor.DoPrepare(psDefaultParameterValues);
myInfo.ExpectingInput = commandProcessor.IsPipelineInputExpected();
commandProcessor.DoPrepare(psDefaultParameterValues);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe it's a correct change.

@daxian-dbw

Copy link
Copy Markdown
Member Author

I have ran all tests on Linux, there were 2 failures, and they are related to Get-PackageProvider and Register-PackageSource. This was because I didn't specify -PSModuleRestore when building and thus I don't have PackageManagement and PowerShellGet modules installed.

So Linux test run was good.

image

@vors vors merged commit 20a0ba0 into PowerShell:master Nov 22, 2016
@daxian-dbw daxian-dbw deleted the nativecommand branch November 22, 2016 00:26
@daxian-dbw

Copy link
Copy Markdown
Member Author

OK, finally finished running all tests on windows and analyzing the failures. There are 8 test failures:

  1. SSH Remoting API Tests.SSHConnectionInfo should throw file not found exception for invalid key file path -- ssh.exe is not available on my test machine
  2. 4 failures in Get-ComputerInfo tests for properties: DeviceGuardAvailableSecurityProperties, DeviceGuardRequiredSecurityProperties, DeviceGuardSecurityServicesConfigured, DeviceGuardSecurityServicesRunning -- somehow there is no value for them on my test VM, which is WS2016.
  3. "Set-Date should be able to set the date in an elevated context" -- Set-Date fails even in elevated session because NativeMethods.SetLocalTime(ref systemTime) fails in SetDateCommand.ProcessRecord. It may be due to the test machine is a VM. Set-Date doesn't work well with VMs.
  4. Register-PackageSource and Get-PackageProvider cannot be found -- PackageManagement related.

image

None of the failures is related to swapping the order of DoPrepare and assigning myInfo.ExpectingInput. So Windows test run was good.

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.

NativeCommandProcessor not TTY compatible

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.