-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix for issue 5752: pwsh cannot start in a directory with a name that includes a wildcard #7240
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
Conversation
|
it looks like the test that failed is unrelated to my change. In reviewing the threadjob test failure, there looks to be a timing issue. I'll follow up with @PaulHigin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suppress output new-item -type Directory -path "${TESTDRIVE}/$d" > $null.
Please use common formatting style:
foreach ( $d in $dirnames ) {
new-item -type Directory -path "${TESTDRIVE}/$d" > $null
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, although this is slower than:
$null = New-Item -type Directory -path "${TESTDRIVE}/$d"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use cmdlet? Maybe better .Net method to get current directory or -LiteralPath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it really doesn't matter, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, maybe use
$result = & $powershell -c '([Environment]::CurrentDirectory)'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I want to check only the name, ason some systems, Environment.CurrentDirectory will include a symlink which is a problem because PS does not, I'm I need to call split-path, I can use this form just as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are testing PowerShell, it would makes sense to use PowerShell i.e. the way Jim has written it rather then calling .NET. Why do you think it should be a .NET call? Is there some reason not to use native PowerShell capabilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My misunderstanding is that we use Directory.GetCurrentDirectory() in the code and Set-Location and Get-Item in the test - do we work with different current directories (first in .Net, second in FileSystem provider)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(style) Maybe it's just my OCD, by why configuring providerContext inside try block rather than right after initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the two operations to be visually close, putting the assignment in the block did that for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We iterate over $dirname twice, once for new-item and once for generating test cases. Should be do it in a single loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, maybe use
$result = & $powershell -c '([Environment]::CurrentDirectory)'
BrucePay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fcddd78 to
2454649
Compare
PaulHigin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at these changes I realize that we really only need wait-forrunning function for the Job2 tests because they test job state and/or use job APIs with the assumption of the job running. And also for the couple of other tests that wait for job state to go to running (with throttle limit). Otherwise Wait-Job is used which will wait for an end state. I don't think it hurts anything but it is not needed.
2454649 to
8449d4d
Compare
|
@adityapatwardhan using the API approach will fail in some circumstances where |
|
@JamesWTruher Is the thread jobs test fix intended to be in this PR? If so could you update the title and description. |
get currentdirectory will include the resolution of symbolic links which will cause the test to fail
|
@JamesWTruher it looks like job test fix was moved to a separate PR #7270 (with some additions), so they can be excluded from this one. |
…rs in the name. There's really no reason why you should glob a path when you're attempting to go to Directory.GetCurrentDirectory()
Sometimes the job object will return with state Running before it is actually running
8d845ec to
7b3e866
Compare
|
Sorry to bother, but when will this fix be in Windows? I'm using PowerShell 5.1.17763.134 on Windows 1809, and issue 5752 still exists. |
|
@ZeppLu the PS team have mentioned in previous issues and PRs that no more fixes are being backported to Windows PowerShell. You are, however, free to download, install, and use PowerShell Core on Windows. 😄 |
|
This fix was included into the version v6.1.0-preview.4 - 2018-07-19. |
|
@vexx32 Installing PowerShell Core does not seem a good solution for me, I'll try out other workarounds. Anyway, thanks for your explanation and advice. |
PR Summary
Fix #5752.
If you attempt to start pwsh in a directory like
/tmp/[T]estthe shell will default to $PSHOMEThe issue was that we attempted to glob against the string returned by
Directory.GetCurrentDirectory()which we should not do.Technically, this is a breaking change - anyone that relies on starting in $PSHOME under these circumstances may be surprised by this behavior.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests