Subproc wrapper part2

[nerdvegas]

-added new Popen class (subclassed subprocess.Popen) to allow for use as context in py2/3. This fixes a ResourceWarning in py3.
-removed old open and py23.subprocess_Popen functions
-moved a bunch of subproc/script- related functions to new rez.utils.execution module (want to deprecate rez.util at some point)
-fixed DEV_NULL file object left open in some cases

Closes #761

[JeanChristopheMorinPerso]

I'm not sure I get the point of subclassing Popen if it's just to not call communicate... The cost of _, _ = p.communicate() is the same as p.wait(), but having this py2/3 wrapper cost (maintenance) more than calling communicate IMO. The current implementation of the __exit__ for py2 also doesn't match 100% what's done in the stdlib in py3.

Note that I'm not against the subclass for the stdin and the text thing (well I find the text thing limit, but I can live with it).

The way I see things, the less py2/3 hacks we have, the better the health of the project will be.

[nerdvegas]

I don't think there's anything wrong with subclassing Popen as a means of adding the support for 'text', and the extra stuff we do wrt stdin. And I don't see any specific need for py2's Popen.__exit__ to match py3 - it's just enabling use as a context in both cases. Wrt 'text', that was done because it's really just a renaming of universal_newlines in py3, which they did because it's become misleading in py3 (it isn't intuitive that 'universal_newlines=True' would change the return type). I can see that calling communicate() avoids the problem, but I don't like that because it's misleading - communicate is there to use when you want to get the output of a process. What I don't understand is why use of wait() in py3 would be causing a ResourceWarning when that must surely be a very common pattern. Given all of the above, I opted for adding support for, and using, Popen as a context where necessary, to avoid the leak. It fixes the issue and keep the intent of the code clear. I also think it's generally cleaner to use contexts for temporary resources anyway, so why not move towards that in all cases where Popen is used?

…

On Tue, Oct 15, 2019 at 12:16 PM Jean-Christophe Morin < ***@***.***> wrote: I'm not sure I get the point of subclassing Popen if it's just to not call communicate... The cost of _, _ = p.communicate() is the same as p.wait(), but having this py2/3 wrapper cost (maintenance) more than calling communicate IMO. The current implementation of the __exit__ for py2 also doesn't match 100% what's done in the stdlib in py3. Note that I'm not against the subclass for the stdin and the text thing (well I find the text thing limit, but I can live with it). The way I see things, the less py2/3 hacks we have, the better the health of the project will be. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#762>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSSCRLTEMTFUGON7OCDQOUKXRANCNFSM4JAVLSGA> .

[maxnbk]

testing on cent with py-2.7, 3.6, 3.7, 3.8 has succeeded for me with no ResourceWarning.