more consistent running of external commands

[mwichmann]

Improvement/maintenance task: SCons has a lot of places where it wants to run external commands. A huge number of these are in tests, as well as Tools - which typically are external-command runners of some sort, and often need to first run the command to determine its version and maybe some compatibility info. How this is done is all over the place, in fact every method mentioned in the Python subprocess module documentation (https://docs.python.org/3/library/subprocess.html#replacing-older-functions-with-the-subprocess-module) as being appropriate for transition to subprocess is used in the scons codebase: os.system, os.popen, os.popen2, os.popen3, os.exec*, popen2, "backtick", as well as subprocess. It would be nice to transition all of this to a consistent approach for better maintainability.

There has been an effort already to implement a "wrapped" subprocess, so some of the common stuff that has to happen is in one place instead of repeated in each caller (arg handling, environment setup, creating the Popen object). This wrapper is SCons.Action/_subproc. It is used in about 20 places, but many more do not use it. The difficulty with _subproc at the moment is that the way it is implemented prevents the use of subprocess.Popen in one of its most valuable ways - as a context manager, which really simplifies the work of cleaning up after using it. A context manager counts on being able to understand the context in question (which has to be within a single function/method), and perform the designated cleanups when it is exited. But because _subproc instantiates the Popen object and returns it to the caller, the context is lost. So one idea for making _subproc more useful is to turn it itself into a context manager. If that works well, then a lot of the other external-process calls can be transitioned over to use it also.

[mwichmann]

see discussion at https://www.mail-archive.com/scons-dev@scons.org/msg04684.html

[bdbaddog]

I'd create another bug just for spawn. I think you can set up bug dependencies to make this the wrapper bug for all subprocess.Popen() outstanding issues.

[mwichmann]

You can't (set up issue dependencies). I've just been looking, it's an extremely wanted feature, but github's minimalist approach has no space for it at the moment, apparently. Sigh, every other bug tracker in the world that I'm familiar with (not a massive list TBH) has this ability. However, that doesn't prevent making a separat issue for win32/spawn.

[bdbaddog]

So I read a suggestion to just create a checklist with one item per dependent bug.. Not the full functionality, but could serve the purpose?

Or we can create a new tag "Popen" or spawn?

[techtonik]

Without context manager gives a lot of warnings about unclosed file handlers:

...
17/1188 (1.43%) /home/travis/virtualenv/python3.6.3/bin/python -tt src/engine/SCons/SConfTests.py
/home/travis/build/SCons/scons/src/engine/SCons/Tool/gcc.py:66: ResourceWarning: unclosed file <_io.BufferedReader name=6>
  return detect_version(env, env.Detect(env.get('CC', compilers)))
/home/travis/build/SCons/scons/src/engine/SCons/Tool/gxx.py:75: ResourceWarning: unclosed file <_io.BufferedReader name=6>
  return gcc.detect_version(env, env.Detect(env.get('CXX', compilers)))
...

https://travis-ci.org/SCons/scons/jobs/475516895#L2817

Created here:

scons/src/engine/SCons/Action.py

Lines 771 to 779 in 08877ad

	io = kw.get('stdin')
	if is_String(io) and io == 'devnull':
	kw['stdin'] = open(os.devnull)
	io = kw.get('stdout')
	if is_String(io) and io == 'devnull':
	kw['stdout'] = open(os.devnull, 'w')
	io = kw.get('stderr')
	if is_String(io) and io == 'devnull':
	kw['stderr'] = open(os.devnull, 'w')

[mwichmann]

The difficulty is the context is kind of spread out... some things happen here, but the context should still be active back in the caller once control returns from here. I think need to turn this whole function into a context manager.

[techtonik]

Making a wrapper with contextlib is probably the easiest way to go.

[mwichmann]

I have a fix for majority of the unclosed file ResourceWarnings, which is independent of what we're talking about here - actually has to do with using the (Popen object).stdout as a context manager (rather than the broader Popen object itself). So that won't really close this issue, but will make things a lot quieter. Not sure there's a fix for the complaints relating to the sqlite file(s), those are intended to stay open far away from the place they're opened.