Skip to content

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 ?

platform dependant check
@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.

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.

3 participants