modules.cmdmod: handle windows environ better

[mattp-]

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.

[twangboy]

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]

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-]

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

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

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

[mattp-]

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

[Ch3LL]

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-]

@Ch3LL yep closing thanks