bpo-36146: Refactor setup.py #12068
bpo-36146: Refactor setup.py #12068
Conversation
The detect_modules() method of setup.py became longer and longer over the years. It is now 1128 lines long. It's way too long: it becomes very hard to track the lifetime of a variable and many variables are overriden on purpose or not. Shorter functions help to track the lifetime of variables, ease review and reduce the number of bugs. Changes: * Fix detect_ctypes(): it no longer modifies global inc_dirs on macOS * self.srcdir is now always an absolute path * Add MACOS and MS_WINDOWS constants * Add TEST_EXTENSIONS to allow to not build extensions used to test Python (ex: _testcapi) * Add MATH_LIBS which is the libm library Other refactoring changes: * Reorder imports * Replace "from distutils.errors import *" with "from distutils.errors import CCompilerError, DistutilsError" to be able to use static analyzers like pyflakes * Rename globals to upper case to better distinguish if a variable is global or locale: * Rename cross_compiling to CROSS_COMPILING * Rename host_platform to HOST_PLATFORM * Rename disabled_module_list to DISABLED_MODULE_LIST * Don't run code at module top level, but at the beginning of main() * Move 'missing' and 'srcdir' from detect_modules() local variable to PyBuildExt attributes * Remove exts local variable from detect_modules(): add add_ext() method which append directly the module to self.extensions. * _detect_openssl() and _detect_nis() now directly add the extension and have no return value. * Split very long detect_modules() method into subfunctions, add many "detect_xxx" methods.
|
@pablogsal @serhiy-storchaka @methane @gpshead @pxinwr: Would you mind to review this large refactoring change? I basically moved a lot of code, but I made some additional changes described in the commit message (and PR description). |
|
Since the change shouldn't modify the behavior of setup.py of any way, I decided to skip the bpo number and NEWS entry. |
|
@vstinner don't you prefer to use |
| def build_extensions(self): | ||
| self.srcdir = sysconfig.get_config_var('srcdir') |
There was a problem hiding this comment.
maybe you could use pathlib.Path ?
There was a problem hiding this comment.
setup.py is used to build Python. When setup.py is called, most Python modules are not available. I prefer to use the bare minimum from the stdlib. For example, "import subprocess" fails with "ModuleNotFoundError: No module named _posixsubprocess". That's why os.system() is used.
Moreover, I prefer to limit changes in this PR.
setup.py
Outdated
| @@ -425,7 +477,7 @@ def check_extension_import(self, ext): | |||
|
|
|||
| # Workaround for Cygwin: Cygwin currently has fork issues when many | |||
| # modules have been imported | |||
| if host_platform == 'cygwin': | |||
| if HOST_PLATFORM == 'cygwin': | |||
There was a problem hiding this comment.
add CYGWIN = (HOST_PLATFORM == 'cygwin') after MS_WINDOWS ?
There was a problem hiding this comment.
Yeah, I hesitated to add this one. Done, I added CYGWIN.
|
@vstinner maybe you prefer to keep the compatibility with |
|
@vstinner excepted the issues with ftp://www.pythontest.net your code could be merged. |
|
Yikes! Please do not merge this until I've had a chance to better review and test various build configurations. It would be a lot easier without so many gratuitous changes. |
I'm sorry, my intent wasn't to scare you :-) I agree that this PR modifies too many unrelated things at once, and so I close it in favor of shorter changes which would be easier to review. I created https://bugs.python.org/issue36146 to track this work and I started with PR #12093 to convert globals to upper case and add some new constants. This PR should be straightforward not controversial in any way :-) |
The detect_modules() method of setup.py became longer and longer over
the years. It is now 1128 lines long. It's way too long: it becomes
very hard to track the lifetime of a variable and many variables are
overriden on purpose or not. Shorter functions help to track the
lifetime of variables, ease review and reduce the number of bugs.
Changes:
macOS
Python (ex: _testcapi)
Other refactoring changes:
Reorder imports
Replace "from distutils.errors import *"
with "from distutils.errors import CCompilerError, DistutilsError"
to be able to use static analyzers like pyflakes
Rename globals to upper case to better distinguish if a variable is
global or locale:
Don't run code at module top level, but at the beginning of main()
Move 'missing' and 'srcdir' from detect_modules() local variable to
PyBuildExt attributes
Remove exts local variable from detect_modules(): add add_ext()
method which append directly the module to self.extensions.
_detect_openssl() and _detect_nis() now directly add the extension
and have no return value.
Split very long detect_modules() method into subfunctions, add
many "detect_xxx" methods.
https://bugs.python.org/issue36146