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

modules.cmdmod: handle windows environ better #52472

Closed
wants to merge 6 commits into from

Conversation

@mattp-
Copy link
Contributor

commented Apr 10, 2019

What does this PR do?

python exposes an nt.environ for case insensitive environment behavior
that is native to windows; so it makes sense to use this instead of
os.environ to avoid enexpected behavior and failure.

further detail: https://bugs.python.org/issue28824

What issues does this PR fix or reference?

environ handling on windows

Tests written?

Yes

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

python exposes an nt.environ for case insensitive environment behavior
that is native to windows; so it makes sense to use this instead of
os.environ to avoid enexpected behavior and failure.

further detail: https://bugs.python.org/issue28824
@mattp- mattp- force-pushed the bloomberg:win_cmd branch from d02fa15 to ebf6df4 Apr 10, 2019
dwoz added a commit that referenced this pull request Apr 10, 2019
2018.3 backport #52472 modules.cmdmod: handle windows environ better
@twangboy

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I'm not sure I'm understanding the intent here. Do you want it to be case-sensitive or not case-sensitive. Here are my results using os.environ and nt.environ:

>>> 'tmp' in nt.environ
False
>>> 'TMP' in nt.environ
True
>>> 'TMP' in os.environ
True
>>> 'tmp' in os.environ
True

So, it looks to me like the dict returned by nt.environ is actually case-sensitive, where os.environ is not. The wording in your description:

python exposes an nt.environ for case insensitive environment behavior
that is native to windows; so it makes sense to use this instead of
os.environ to avoid enexpected behavior and failure.

would seem to indicate that you want a case-insensitive environment... which is not what I am getting with nt.environ.

@twangboy

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I also found nt.environ to be quite a bit slower than os.environ. If your intent is NOT case sensitive then I would recommend we stay with os.environ

If we go that route, we'll need to revert the PR that has been merged (#52476).

@mattp-

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@twangboy I think I may have worded that poorly. I'll describe what uncovered this: I am using salt to spawn chef-client converge (which in turn runs chocolatey installations). When spawning through cmd.run with os.environ being passed down to the popen, something was breaking with chocolatey that was making it think multiple versions of it were running/throwing 'chocolateyPending file in use' errors. In troubleshooting, I noticed a straight popen with no env being passed allowed things to work correctly, ie child proc is inheriting the parent env from salt. when cloning nt.environ, it still worked as expected, meaning that using nt.environ is equivalent to inheriting what is from the parent, but os.environ is not.
as to what is causing this difference, I'm not sure - but it seems to me that if nt.environ is equivalent to not passing an env kwarg to popen at all for direct inheritance, its probably what we want?

@twangboy

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

@mattp- OK. The environment returned by os and nt are also different. The dictionaries I got were differing lengths. I wonder if nt.environ is regenerating the environment, while os.environ is just getting what python already has loaded...

@mattp-

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@twangboy https://github.com/python/cpython/blob/master/Lib/os.py#L729 I think is where the creation of os.environ comes from, which is all uppercase keys, first wins if multiple exist I guess.
nt.environ, is actually from https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L14087
so its a static dict of the environment as it was received from exec. I still think it makes sense to use this instead of os.environ, since it seems a that it affects downstream behavior of other windows apps to not do so.. but this also means that any changes to the running environment of the minion over time won't be included. not really sure whats 'right' here, but I do know using nt.environ instead of os.environ solved my problem 🙂

dwoz added a commit that referenced this pull request Apr 11, 2019
2019.2 backport #52472 modules.cmdmod: handle windows environ better
@mattp-

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@twangboy this should be good to merge I think ? thanks 👍

twangboy added 2 commits May 14, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

hmm it looks like these changes have already been merged. im guessing what happened was it was merged here #52477 and then merged forward. @mattp- are you okay closing this?

@mattp-

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@Ch3LL yep closing thanks

@mattp- mattp- closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.