Remove certain CLI args from setup.py
#3345
Conversation
Instead of passing `--enable-X` or `--disable-Y` or `--without-Z` to `setup.py`, it makes more sense to just directly use the CMake variables. The following are now set as env variables in `build_wheels.bash`: - NRN_ENABLE_PYTHON_DYNAMIC - NRN_RX3D_OPT_LEVEL - NRN_ENABLE_RX3D - NRN_ENABLE_INTERVIEWS - NRN_ENABLE_MPI - NRN_ENABLE_MUSIC - NRN_ENABLE_CORENEURON This eliminates the need for the following args: - without-nrnpython - rx3d-opt-level - disable-rx3d - disable-iv - disable-mpi - enable-music - enable-coreneuron This is useful since we want to replace calls of `python setup.py [args]` with `pip install`, i.e. towards conformance with PEP 517 (see also: #2696). It also refactors `build_wheels.bash` to make it more modular and using the "don't repeat yourself" (DRY) principle. The user-facing changes are as follows: - `build_wheels.bash` can now be ran without _any_ parameters because the platform is automatically detected (using `uname -s`) - the possible values of the first argument to `build_wheels.bash` were changed from `linux` to `Linux`, and `osx` to `Darwin` to conform to the output to `uname -s`. The special value `CI` remains valid.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3345 +/- ##
=======================================
Coverage 68.31% 68.31%
=======================================
Files 681 681
Lines 116420 116420
=======================================
Hits 79527 79527
Misses 36893 36893 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
✔️ a965a63 -> Azure artifacts URL |
|
✔️ 28170d8 -> Azure artifacts URL |
|
✔️ 7c0eb0a -> Azure artifacts URL |
| NRN_ENABLE_PYTHON_DYNAMIC : ${{ matrix.config.python_dynamic || 'OFF' }} | ||
| NRN_ENABLE_MUSIC: ${{ matrix.config.music || 'OFF' }} |
There was a problem hiding this comment.
This is needed because otherwise setup.py's strtobool throws an error (as any GitHub Action config option not explicitly specified defaults to the empty string, '').
| PLATFORM_LINUX="Linux" | ||
| PLATFORM_MACOS="Darwin" |
There was a problem hiding this comment.
The default identifiers that uname -s returns (on any "normal" Linux, and all MacOS versions).
| pip check | ||
| configure_macos() { | ||
| echo " - Installing build requirements for MacOS" | ||
| pip install -U 'delocate<=0.13.0,>=0.13.0' |
There was a problem hiding this comment.
Recent version of delocate fixed this bug: matthew-brett/delocate#153
JCGoran
changed the title
setup.pysetup.py
|
✔️ 5bc2ce4 -> Azure artifacts URL |
|
|
✔️ e3dc73f -> Azure artifacts URL |
Changes
Instead of passing
--enable-Xor--disable-Yor--without-Ztosetup.py, it makes more sense to just directly use the CMake variables. The following are now set as env variables inbuild_wheels.bash(with exactly the same defaults as before when building a wheel) that are then passed tosetup.py:This eliminates the need for the following CLI args to
setup.py:The docs have been updated to reflect the above.
It also refactors
build_wheels.bashto make it more modular and using the "don't repeat yourself" (DRY) principle.Context
This is useful since we want to replace calls to
python setup.py [args]withpip install, i.e. towards conformance with PEP 517 (see also: #2696 and the issues linked in that discussion), for which we shouldn't have any args on the command-line.User-facing changes
build_wheels.bashwere changed fromlinuxtoLinux, andosxtoDarwinto conform to the output touname -s. The special valueCIremains valid.--disable-mpiand so on, one should specifyNRN_ENABLE_MPI=ONand others as env variables when runningpython setup.py. The remaining valid CLI args are--cmake-build-dirand--cmake-defs, which are scheduled for later removal