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

Recognize Windows cmd or powershell environment in updateProcessEnv #15165

Merged
merged 2 commits into from Apr 25, 2018

Conversation

Projects
None yet
5 participants
@segevfiner
Contributor

segevfiner commented Aug 4, 2017

Description of the Change

Windows doesn't have PWD in the environment passed to processes from the shell. cmd has PROMPT and PowerShell has PSModulePath, so also check for those to determine if we were launched from proper shell environment. At least that's what I think this condition is meant to check for.

Alternate Designs

Maybe a different check is more appropriate.

Why Should This Be In Core?

It's a bug in the core functionality of updating the environment from the shell on Windows.

Benefits

The environment will be updated when using the atom command from the shell like it does on other operating systems.

Possible Drawbacks

If this starts updating the environment on cases that it shouldn't.

Applicable Issues

Fixes #14986

@segevfiner segevfiner changed the title from :bug: Recognize Windows cmd or powershell environment in updateProcessEnv to Recognize Windows cmd or powershell environment in updateProcessEnv Aug 4, 2017

@50Wliu 50Wliu added the needs-testing label Aug 4, 2017

@segevfiner

This comment has been minimized.

Contributor

segevfiner commented Jan 23, 2018

ping @50Wliu

This PR has been sitting here for quite a while...

@rsese

This comment has been minimized.

Member

rsese commented Feb 21, 2018

Sorry for the delay @segevfiner - the team talked about this one and were wondering even though it's a small change if you think there's any test that could be added?

@segevfiner

This comment has been minimized.

Contributor

segevfiner commented Feb 22, 2018

Added some specs. They are unit tests, so someone might want to verify that this actually fixes the problem when you are really launching Atom from cmd or powershell.

@daviwil

Looks good to me, thanks a lot @segevfiner!

@daviwil daviwil merged commit 5b485dc into atom:master Apr 25, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@segevfiner segevfiner deleted the segevfiner:sf-windows-update-process-env branch Nov 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment