Skip to content

Conversation

@JamesWTruher
Copy link
Collaborator

@JamesWTruher JamesWTruher commented Jul 6, 2018

PR Summary

Fix #5752.

If you attempt to start pwsh in a directory like /tmp/[T]est the shell will default to $PSHOME

The 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

@JamesWTruher
Copy link
Collaborator Author

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.

Copy link
Collaborator

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
}

Copy link
Collaborator Author

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"

Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

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, maybe use

$result = & $powershell -c '([Environment]::CurrentDirectory)'

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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)?

Copy link

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?

Copy link
Collaborator Author

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

Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

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, maybe use

$result = & $powershell -c '([Environment]::CurrentDirectory)'

Copy link
Collaborator

@BrucePay BrucePay left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@PaulHigin PaulHigin left a 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.

@JamesWTruher
Copy link
Collaborator Author

@adityapatwardhan using the API approach will fail in some circumstances where ${TESTDRIVE}/... in PowerShell is /tmp/<guid>/... but the API may return /private/tmp/<guid>/... as it follows the symlink. Testing the name of the directory is the easiest way to avoid the necessity of recognizing/constructing the "real" path. What I've done proves the behavior well enough.

@adityapatwardhan
Copy link
Member

@JamesWTruher Is the thread jobs test fix intended to be in this PR? If so could you update the title and description.

@JamesWTruher JamesWTruher dismissed adityapatwardhan’s stale review July 11, 2018 16:03

get currentdirectory will include the resolution of symbolic links which will cause the test to fail

@anmenaga
Copy link

@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
@daxian-dbw daxian-dbw merged commit d61ea03 into PowerShell:master Jul 12, 2018
@ZeppLu
Copy link

ZeppLu commented Dec 10, 2018

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.

@vexx32
Copy link
Collaborator

vexx32 commented Dec 10, 2018

@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. 😄

@KillyMXI
Copy link

This fix was included into the version v6.1.0-preview.4 - 2018-07-19.
The question is, when the fresh enough version of PowerShell will be shipped with Windows.

@ZeppLu
Copy link

ZeppLu commented Dec 11, 2018

@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.

@TravisEz13 TravisEz13 added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Powershell does not open in current directory if its name contains a left square bracket (wildcard)