bpo-39763: distutils.spawn now uses subprocess

[vstinner]

Reimplement distutils.spawn.spawn() function with the subprocess
module.

setup.py now uses a basic implementation of the subprocess module if
the subprocess module is not available: before required C extension
modules are built.

https://bugs.python.org/issue39763

[vstinner]

Now to fix https://bugs.python.org/issue39763, parallelism must be disable when the pure Python implementation is used:

diff --git a/setup.py b/setup.py
index c9f3c97238..c89e2cb8ad 100644
--- a/setup.py
+++ b/setup.py
@@ -358,7 +358,10 @@ class PyBuildExt(build_ext):
         self.failed = []
         self.failed_on_import = []
         self.missing = []
-        if '-j' in os.environ.get('MAKEFLAGS', ''):
+        # Don't use parallel build if the subprocess is bootstrapped:
+        # the pure Python implementation is unsafe ([bpo-39763](https://bugs.python.org/issue39763)).
+        if ('-j' in os.environ.get('MAKEFLAGS', '')
+           and not SUBPROCESS_BOOTSTRAP):
             self.parallel = True
 
     def add(self, ext):

I would prefer to push this change in a separated PR.

[vstinner]

cc @pitrou

[pitrou]

parallelism must be disable when the pure Python implementation is used

Or you can use posix_spawn in the pure Python fallback.

[vstinner]

Or you can use posix_spawn in the pure Python fallback.

It's not available on all platforms. I don't think that it's interesting to optimize the setup.py implementation of subprocess.

subprocess.py only uses os.posix_spawn() on a few specfic platforms: macOS and Linux with glibc 2.24 or newer.

[vstinner]

Even when subprocess is available, os.posix_spawn() is not used by subprocess (called by distutils) because distutils.spawn uses close_fds default value which is True. If you want to experiment os.posix_spawn() in distutils, try this patch locally:

diff --git a/Lib/distutils/spawn.py b/Lib/distutils/spawn.py
index aad277b0ca..086d5cc3b7 100644
--- a/Lib/distutils/spawn.py
+++ b/Lib/distutils/spawn.py
@@ -71,7 +71,7 @@ def spawn(cmd, search_path=1, verbose=0, dry_run=0):
             env = dict(os.environ,
                        MACOSX_DEPLOYMENT_TARGET=cur_target)
 
-    proc = subprocess.Popen(cmd, env=env)
+    proc = subprocess.Popen(cmd, env=env, close_fds=False)
     proc.wait()
     exitcode = proc.returncode
 

[codecov]

Codecov Report

Merging #18743 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #18743     +/-   ##
=========================================
  Coverage   82.14%   82.14%             
=========================================
  Files        1956     1955      -1     
  Lines      590140   584725   -5415     
  Branches    44488    44475     -13     
=========================================
- Hits       484743   480337   -4406     
+ Misses      95745    94742   -1003     
+ Partials     9652     9646      -6     

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	30.00% <0.00%> (-65.00%)	⬇️
Lib/distutils/command/bdist_rpm.py	7.63% <0.00%> (-56.88%)	⬇️
Modules/_decimal/libmpdec/umodarith.h	80.76% <0.00%> (-19.24%)	⬇️
Lib/test/test_urllib2net.py	76.92% <0.00%> (-13.85%)	⬇️
Lib/test/test_smtpnet.py	78.57% <0.00%> (-7.15%)	⬇️
Lib/ftplib.py	63.85% <0.00%> (-6.06%)	⬇️
Lib/test/test_ftplib.py	87.11% <0.00%> (-4.72%)	⬇️
Tools/scripts/db2pickle.py	17.82% <0.00%> (-3.97%)	⬇️
Tools/scripts/pickle2db.py	16.98% <0.00%> (-3.78%)	⬇️
Lib/test/test_socket.py	71.94% <0.00%> (-3.77%)	⬇️
... and 333 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28d0bca...e7d0e98. Read the comment docs.

[vstinner]

@pganssle @pablogsal @tirkarthi @pitrou @nanjekyejoannah: Would you mind to review this change?

This change basically continues the same code to spawn subprocesses when building Python from scratch. The difference is that incremental build of Python, or obviously building any third party C extension module, will now use subprocess, rather than fork+exec implemented in pure Python.

The subprocess module uses _posixsubprocess C extension module to reduce the risk of race conditions.

[pitrou]

+1

[jaraco]

This change appears to be implicated in a regression where in some cases spaces in filenames aren't quoted so libraries fail to link. See pypa/setuptools#2417.