BLD: Avoid using os.spawnve in favor of os.spawnv in exec_command

[ales-erjavec]

The env modifying spawn* function are not thread safe on windows (python issue6476) so avoid using them unless so requested by the caller.

This is a minimally intrusive fix for the segfaults reported in #7607

[charris]

This isn't my area. @njsmith @juliantaylor Thoughts?

[njsmith]

My concern is that this only makes _exec_command safe in some cases (no env), while adding more spaghetti to the code. Switching to subprocess or even to the thread-safe spawnve posted in the linked issue would simplify the code while making it actually threadsafe in all cases.

[ales-erjavec]

Switching to subprocess or even to the thread-safe spawnve posted in the linked issue would simplify the code while making it actually threadsafe in all cases.

Note that subprocess is not completely thread safe either, it just has different failure modes

• http://bugs.python.org/issue2320
• http://bugs.python.org/issue1336

The situation is greatly improved in Python >= 3.2 and there does exist a backported subprocess32.

I did start to work on replacing the exec_command, with subprocess, but considering that that would (probably) cause different bugs, I opted to make this simpler change (that can also be backported) first.

[njsmith]

I just audited the numpy codebase and there appear to be no uses of the env-setting feature of exec_command. @charris: what do you think about just making someone attempting to pass envvars to exec_command into an error. (The motivation is that the PR here fixes the non-env case but leaves the env case buggy, at the cost of a bunch of extra switches to special case env-versus-non-env; all that goes away if we rip out the env support.)

[charris]

@njsmith I'm not opposed in principle. The function itself dates to 2003, but it is exposed. I don't see any uses of it in scipy. The documentation says

Implements exec_command function that is (almost) equivalent to
commands.getstatusoutput function but on NT, DOS systems the
returned status is actually correct (though, the returned status
values may be different by a factor). In addition, exec_command
takes keyword arguments for (re-)defining environment variables.

So adding the ability to modify the evironment was intended, but that doesn't mean it was a good idea ;) Note that the getstatusoutput function has been moved to the subprocess module in python 3 and the commands module was deprecated in 2.6. The function looks like it was originally a workaround and is perhaps due a change.

A backward compatible option would be to define a new function to used in its place and deprecate the current function.

[ales-erjavec]

A backward compatible option would be to define a new function to used in its place and deprecate the current function.

I think subprocess.run introduced in Python 3.5 would be a candidate (interface-wise) to replace exec_command. It is a simple use of Popen's interface and matches the existing exec_command use pattern. It can be 'backported' to support older Python versions (like this)

[njsmith]

Meh, this is a mess, in code that's already a mess, and whose long term trajectory is probably to be removed rather than fixed, so I think I'm changing my vote to just merging and moving on :-). Anyone object?

[charris]

@njsmith No, be my guest.

[njsmith]

Let's do this

[njsmith]

Thanks @ales-erjavec