Skip to content
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

Set-Location: Match $env:PWD & $PWD on UNIX #8901

Closed
wants to merge 5 commits into from

Conversation

lucyllewy
Copy link
Contributor

@lucyllewy lucyllewy commented Feb 15, 2019

PR Summary

Update the UNIX Environment variable PWD to the current location.

PR Context

Fixes #7388

Set the UNIX Environment variable $env:PWD to the full filesystem path
for the current working directory upon Set-Location. This also copes
when the current location is set to a PSDrive. In such circumstances the
environment variable is expanded to the full FileSystem provider path.

PR Checklist

Set the UNIX Environment variable $env:PWD to the full filesystem path
for the current working directory upon Set-Location. This also copes
when the current location is set to a PSDrive. In such circumstances the
environment variable is expanded to the full FileSystem provider path.

Fixes PowerShell#7388

Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
Replace positional parameters with named equivalents.

Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
Copy link
Contributor

@kvprasoon kvprasoon left a comment

Choose a reason for hiding this comment

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

Casing can be changed .

$path --> $Path and $location --> $Location . Please do the change for all occurrences. Once again, these are not mandatory for the functionality and PR to get merged.

Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
@lucyllewy
Copy link
Contributor Author

Thanks for the thorough review, @kvprasoon, and for sticking with me while I work through your suggestions :-)

@iSazonov
Copy link
Collaborator

I think we can not accept the PR because PowerShell can create many runspaces each of which has its own value of $PWD. Note that .Net also has "pwd" which doesn't correlate with PowerShell $PWD.

@vexx32
Copy link
Collaborator

vexx32 commented Feb 22, 2019

@iSazonov it would be better to set $env:PWD for the current process only, I guess? Is that possible on Unix platforms?

@iSazonov
Copy link
Collaborator

for the current process only

@vexx32 PowerShell can create many runspaces/threads in the same process and everyone will export $env:PWD - it is a mess.

@vexx32
Copy link
Collaborator

vexx32 commented Feb 22, 2019

@iSazonov how do other threaded Unix applications handle this issue? Are there any existing applications we could model behaviour from here?

@iSazonov
Copy link
Collaborator

@vexx32 $env:PWD is common resource. Let's replace it with Set-Date - what result do you get if many process/threads will modify system time in the same time? I guess Unix uses another scenario - a process track PWD for itself or inform subprocess about work directory.

@lucyllewy
Copy link
Contributor Author

Using Environment.SetEnvironmentVariable on UNIX will only change the value for the current process and those beneath it in the process tree. You mention multiple run-spaces... how do I start a second run-space within the current pwsh process to test the scenario? (I'm not very familiar with PowerShell yet.)

@vexx32
Copy link
Collaborator

vexx32 commented Feb 22, 2019

An function written in PowerShell that I wrote utilising runspaces can be found here.

The runspace pool is created on line #566, and the separate instances are created and invoked on lines #594-#627

This is a bit of a more complex example as I'm batching the tasks to make the best use of it, but it should illustrate the overall idea. 😄

There may be some already-built functions designed to help test these types of cases in amongst some of the existing test files, but I'm not sure of where to look for those.

@lucyllewy
Copy link
Contributor Author

OK, run-spaces definitely share the PWD environment, so this PR as it stands is not good-to-go :-)

@lucyllewy
Copy link
Contributor Author

This needs rethinking, so I'm closing the PR.

@lucyllewy lucyllewy closed this Mar 5, 2019
@lucyllewy lucyllewy deleted the unix-pwd-envvar branch February 15, 2020 01:53
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.

Unix utilities expect $env:PWD to reflect the current directory, as per POSIX
4 participants