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

add -tdlib=libc++ argument #72

Merged
merged 3 commits into from
Oct 23, 2018
Merged

add -tdlib=libc++ argument #72

merged 3 commits into from
Oct 23, 2018

Conversation

ncourty
Copy link
Collaborator

@ncourty ncourty commented Oct 19, 2018

No description provided.

@rflamary
Copy link
Collaborator

OK so now we know this parameter breaks the linux version ;)

So now we need to have a test about if Macosx and if Mojave (or above?) in the setup.py I guess.

You ok with this @ncourty ?

@ncourty
Copy link
Collaborator Author

ncourty commented Oct 19, 2018

Added a platform dependant test in order to solve permanently #71

@ncourty ncourty requested a review from rflamary October 19, 2018 23:22
opt_arg=["-O3"]
import platform
if platform.system()=='Darwin':
if platform.release()=='18.0.0':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you use >= instead, i don't think this option will be necessary on just of release of MacSOX

Suggested change
if platform.release()=='18.0.0':
if platform.release()>='18.0.0':

PS trying the suggestion thing it's nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about the suggestion. In its form I think the bug is partly due to the fact that python Extension module has trouble to get the correct path from the new locations of clang compiler on the new OSX. I hope this will be corrected in the future. Clearly it is a temporary patch here.

@@ -24,6 +24,12 @@
with open(os.path.join(ROOT, 'README.md'), encoding="utf-8") as f:
README = f.read()

# add platform dependant optional compilation argument
opt_arg=["-O3"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@rflamary rflamary merged commit c48be43 into master Oct 23, 2018
@rflamary rflamary deleted the osx-issue branch December 5, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants