Skip to content

Fix build for OS X#1887

Merged
ctb merged 1 commit into
masterfrom
osx-build
Mar 13, 2019
Merged

Fix build for OS X#1887
ctb merged 1 commit into
masterfrom
osx-build

Conversation

@standage

Copy link
Copy Markdown
Member

This PR updates the min OS X build target. 10.7 is very old and the setuptools/distutils build config for 10.7 uses outdated compiler options. Latest OS X release is 10.14, but even just updating to 10.9 as the min build version enables a proper build on recent versions.

@codecov

codecov Bot commented Feb 21, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1887 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1887   +/-   ##
======================================
  Coverage    0.01%   0.01%           
======================================
  Files          78      78           
  Lines       33208   33208           
  Branches      434     434           
======================================
  Hits            5       5           
  Misses      33203   33203

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ec12a...d982f54. Read the comment docs.

@standage

Copy link
Copy Markdown
Member Author

Ready for review and merge!

Comment thread setup.py
'-stdlib=libc++'])
EXTRA_LINK_ARGS.append('-mmacosx-version-min=10.7')
EXTRA_COMPILE_ARGS.extend(['-arch', 'x86_64', '-mmacosx-version-min=10.9'])
EXTRA_LINK_ARGS.append('-mmacosx-version-min=10.9')

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the fix.

Comment thread setup.py
'-fno-omit-frame-pointer']
EXTRA_LINK_ARGS = ['-fno-omit-frame-pointer']
EXTRA_COMPILE_ARGS = ['-O3', '-std=c++11']
EXTRA_LINK_ARGS = []

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Some collateral damage from my debugging, but after reading up a bit it's probably better we didn't enable those flags unless we have a compelling reason to, in which case, documenting it would be great!

@ctb ctb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, and works on my Mac.

@ctb ctb merged commit fb65d21 into master Mar 13, 2019
@ctb ctb deleted the osx-build branch March 13, 2019 13:42
@standage

Copy link
Copy Markdown
Member Author

Thanks @ctb!

@ghost

ghost commented Aug 6, 2022

Copy link
Copy Markdown

pisahkan

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