Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLD: compiler recognition on MacOS #3109

Closed
tylerjereddy opened this issue Jan 25, 2021 · 6 comments · Fixed by #3234
Closed

BLD: compiler recognition on MacOS #3109

tylerjereddy opened this issue Jan 25, 2021 · 6 comments · Fixed by #3234

Comments

@tylerjereddy
Copy link
Member

Expected behavior

Build system automagically handles both clang and "genuine" gcc on MacOS. This command should work with spack built gcc compiler toolchain loaded on macOS 10.15.7: python setup.py develop

Actual behavior

The build system thinks it is using clang but it is actually using gcc built with clang by spack (not the "alias" you would normally have on a "normal" mac machine).

gcc: error: unrecognized command line option '-stdlib=libc++'
error: command '/Users/treddy/github_projects/spack/opt/spack/darwin-catalina-x86_64/clang-11.0.0-apple/gcc-8.3.0-fxj4m5svuktydrjgtfkbxyifibwf6wsi/bin/gcc' failed with exit status 1

Code to reproduce the behavior

python setup.py develop

Python 3.7.4, though not likely relevant. Latest develop branch.

Hacky work around, that hints at part of code involved:

--- a/package/setup.py
+++ b/package/setup.py
@@ -286,9 +286,9 @@ def extensions(config):
     cpp_extra_link_args=[]
     # needed to specify c++ runtime library on OSX
     if platform.system() == 'Darwin' and using_clang():
-        cpp_extra_compile_args.append('-stdlib=libc++')
+        #cpp_extra_compile_args.append('-stdlib=libc++')
         cpp_extra_compile_args.append('-mmacosx-version-min=10.9')
-        cpp_extra_link_args.append('-stdlib=libc++')
+        #cpp_extra_link_args.append('-stdlib=libc++')
         cpp_extra_link_args.append('-mmacosx-version-min=10.7')
@IAlibay
Copy link
Member

IAlibay commented Jan 25, 2021

@tylerjereddy I think I'm misunderstanding the issue. Is it that somehow the spack built gcc is triggering a return of True for using_clang()?

If so, out of curiosity what's the output of gcc -v?

@tylerjereddy
Copy link
Member Author

tylerjereddy commented Jan 25, 2021

@IAlibay Yes, that's what happens--I'd say that ideally the mechanism of compiler detection should not rely on string analysis alone--for example, we just do return 'clang' in compiler_ver in that function, which is obviously true because gcc was built from source using clang and spack will include the string clang in some of the metadata, etc.

Here's a small sample of gcc -v output; just this first line is sufficient to trick our build system already:

Reading specs from /Users/treddy/github_projects/spack/opt/spack/darwin-catalina-x86_64/clang-11.0.0-apple/gcc-8.3.0-fxj4m5svuktydrjgtfkbxyifibwf6wsi/lib/gcc/x86_64-apple-darwin19.3.0/8.3.0/specs

@tylerjereddy
Copy link
Member Author

I wonder if NumPy distutils has something we can borrow for this, probably

@IAlibay
Copy link
Member

IAlibay commented Jan 25, 2021

So probably a very naive question (especially since we go through the trouble of using -v currently), but why aren't we just using the string from compiler.compiler in our distutils compiler check?

I realise folks could technically alias gcc or clang to whatever string value they want, but surely we could be taking the name provided at face value right? If "clang" in compiler.compiler[0] returns True then we should assume intent by naming?

I can see a case of someone calling your above case gcc-clang though...

@tylerjereddy
Copy link
Member Author

I think 'gcc' more often means clang than gcc on MacOS, so we can't really take the name too literally. The string clang will almost always be in the description we get out there because it is basically an alias of sorts to clang. So, I don't think this is just "folks aliasing it," but rather that's the default case on MacOS if you don't change anything so our current code path would likely cover 90%+ of end users/devs.

Indeed, it is me who has changed something by using genuine gcc instead of the "default alias to clang." That should be fair game though.

I'm pretty sure most robust solutions to the problem involve a try/except style approach to seeing what flags can be supported and deciding off that (I think CMake commonly does small try_compile type things, probably NumPy distutils too).

@hmacdope
Copy link
Member

hmacdope commented Jan 26, 2021

Sorry to chime in a bit here but down the road at distopia we use something like:

  #ifdef __clang_major__
    #define DISTOPIA_CLANG 1
  #endif

  #ifdef __INTEL_COMPILER
    #define DISTOPIA_ICC 1
  #endif
  
  #if defined(__GNUC__) && !defined(DISTOPIA_CLANG) && !defined(DISTOPIA_ICC)
    #define DISTOPIA_GCC 1
  #endif

  #if defined(_MSC_VER) && !defined(DISTOPIA_ICC)
    #define DISTOPIA_MSVC 1
  #endif

you could then combine with something like:

static_assert(DISTOPIA_CLANG != 1, "compiler is not clang");

which will fail to compile if not on clang if you want to go for the try_compile idea.

IAlibay pushed a commit that referenced this issue Apr 24, 2021
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
lilyminium pushed a commit to lilyminium/mdanalysis that referenced this issue Apr 28, 2021
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
jbarnoud added a commit that referenced this issue May 3, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants