Conversation
* gracefully handle the case where `gcc` toolchain in use on MacOS has been built from source using `clang` by `spack` (so it really is `gcc` in use, not `clang`) * we could try to add regression testing, but a few problems: - `using_clang()` is inside `setup.py`, which probably can't be safely imported because it has unguarded statements/ code blocks that run right away - testing build issues is typically tricky with mocking, etc. (though in this case, probably just need to move `using_clang()` somewhere else and then test it against a variety of compiler metadata strings
Codecov Report
@@ Coverage Diff @@
## develop #3234 +/- ##
==========================================
Coverage ? 92.81%
==========================================
Files ? 170
Lines ? 22801
Branches ? 3239
==========================================
Hits ? 21162
Misses ? 1591
Partials ? 48 Continue to review full report at Codecov.
|
| customize_compiler(compiler) | ||
| compiler_ver = getoutput("{0} -v".format(compiler.compiler[0])) | ||
| return 'clang' in compiler_ver | ||
| if 'Spack GCC' in compiler_ver: |
There was a problem hiding this comment.
So I have no issues with the setup file as-is, but I do wonder if this is opening a pandora's box in supporting the peculiarities for various build tools (for example, does EasyBuild also need handling differently?).
Is this the same way numpy/scipy is handling compiler identification @tylerjereddy ?
There was a problem hiding this comment.
No, SciPy uses scipy/_build_utils/compiler_helper.py, which is a module dedicated to compiler handling. As discussed in the matching issue, they implement a small try_compile() function to only allow flags to get appended to the compile line if the compiler genuinely supports them.
We could redesign to use something like that--that may be biting off a bit more than I was hoping to chew here, though we could probably even copy-paste a decent bit of the code.
Here's an example of how it works:
if sys.platform == 'darwin':
# Set min macOS version
min_macos_flag = '-mmacosx-version-min=10.9'
if has_flag(cc, min_macos_flag):
args.append(min_macos_flag)
ext.extra_link_args.append(min_macos_flag)That has_flag() function checks that the compile flag works "for real" with the compiler:
def has_flag(compiler, flag, ext=None):
"""Returns True if the compiler supports the given flag"""
return try_compile(compiler, flags=[flag], ext=ext)There was a problem hiding this comment.
Is this starting to re-implement what automake and cmake build systems do?
Yeah, basically--this discussion is also relevant: scipy/scipy#13615 I wonder if the Python ecosystem will stabilize on a common solution eventually. |
IAlibay
left a comment
There was a problem hiding this comment.
I probably should have just approved in my first review 😅
Thanks for the insights @tylerjereddy, maybe we should open a separate issue to target a refresh of the build system based on what scipy does? (if it somehow fixes #3176 at the same time, then even better!).
|
Sounds good, your skepticism is justified--this isn't exactly elegant either. And we have in-house expertise with CMake, so some of our dystopia project assembly-level folks may share my irritation with having to learn meson on top of CMake, but anyway.. |
Fixes MDAnalysis#3109 ## Work done in this PR * gracefully handle the case where `gcc` toolchain in use on MacOS has been built from source using `clang` by `spack` (so it really is `gcc` in use, not `clang`) ## Notes * we could try to add regression testing, but a few problems: - `using_clang()` is inside `setup.py`, which probably can't be safely imported because it has unguarded statements/ code blocks that run right away - testing build issues is typically tricky with mocking, etc. (though in this case, probably just need to move `using_clang()` somewhere else and then test it against a variety of compiler metadata strings
* added get_connections * modified tests for atoms.bonds/angles/dihedrals etc * modified parsers and things to use get_connections or bonds * updated CHANGELOG * pep8 * undo half of PR 3160 * add intra stuff * Update package/MDAnalysis/core/groups.py Co-authored-by: Jonathan Barnoud <jonathan@barnoud.net> * tighten up base class checking * update docstring * suppres warnings * Use absolute file paths in ITPParser (#3108) Fixes #3037 Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com> * Adds aromaticity and Gasteiger charges guessers (#2926) Towards #2468 ## Work done in this PR * Add aromaticity and Gasteiger charges guessers which work via the RDKIT converter. * BLD: handle gcc on MacOS (#3234) Fixes #3109 ## Work done in this PR * gracefully handle the case where `gcc` toolchain in use on MacOS has been built from source using `clang` by `spack` (so it really is `gcc` in use, not `clang`) ## Notes * we could try to add regression testing, but a few problems: - `using_clang()` is inside `setup.py`, which probably can't be safely imported because it has unguarded statements/ code blocks that run right away - testing build issues is typically tricky with mocking, etc. (though in this case, probably just need to move `using_clang()` somewhere else and then test it against a variety of compiler metadata strings * Remove ParmEd Timestep writing "support" (#3240) Fixes #3031 * Adding py3.9 to gh actions CI matrix (#3245) * Fixes #2974 * Python 3.9 officially supported * Add Python 3.9 to testing matrix * Adds macOS CI entry, formalises 3.9 support * fix changelog * special metaclass * move function down * tidy code Co-authored-by: Jonathan Barnoud <jonathan@barnoud.net> Co-authored-by: Aditya Kamath <48089312+aditya-kamath@users.noreply.github.com> Co-authored-by: Cédric Bouysset <bouysset.cedric@gmail.com> Co-authored-by: Tyler Reddy <tyler.je.reddy@gmail.com> Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
gracefully handle the case where
gcctoolchainin use on MacOS has been built from source using
clangby
spack(so it really isgccin use, notclang)we could try to add regression testing, but a few problems:
using_clang()is insidesetup.py, which probablycan't be safely imported because it has unguarded statements/
code blocks that run right away
(though in this case, probably just need to move
using_clang()somewhere else and then test it against a variety of compiler
metadata strings)
Fixes #3109
PR Checklist