-
Notifications
You must be signed in to change notification settings - Fork 518
[WIP] only mangle CC/etc if on mac and not told not to #175
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
Conversation
(Rebased onto #176 to avoid the flake8 error.) |
setup.py
Outdated
if sys.platform.startswith('darwin'): | ||
if sys.platform.startswith('darwin') and not os.environ.get('POT_LEAVE_CC', ''): | ||
os.environ["CC"] = "g++" | ||
os.environ["CXX"] = "g++" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to do this in the first place? POT does not compile with clang or icc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for me not the right fix.
I typically do this when using clang on my mac:
CFLAGS='-stdlib=libc++' pip install -e .
and not need for a hack in setup.py but maybe there is a simpler way.
… |
I don't know why this would ever be generally needed; my worry is that the current behavior is over-reacting to one user's broken build system. But @ncourty can clarify maybe. |
Hello all. Yes, I do not think changing the CC flag is needed anymore. I remember it was an old solution to OSX installation problems before clang becomes the main c++ compiler on OSX. Clearly it is a reminiscence from the past, I guess we can remov 8000 e it now ? |
I propose to remove it and see if CIs break. If not we can remove
|
good for me |
OK now this PR is just removing the flags and it still works in CI. Since @djsutherland basically had to remove it for it to build for conda I guess we should merge it if everyone is OK with it. |
ok with this ! |
For the record, I don't love the |
Can you maybe make a new release that includes this change? It's been more than a year. |
Or how about a |
As mentioned in conda-forge/pot-feedstock#9 (comment) and following up on #130 – always setting
CC=g++
is the wrong thing to do, and also always doing theCFLAGS
mangling on Mac, regardless of what compilers you're doing/etc, is also the wrong thing to do. This only setsCC
on Mac, and allows you to bail out of doing any of this (as e.g. conda-forge will want) by settingPOT_LEAVE_CC
.CC @ncourty