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: for C extension builds on mac, target macOS 10.9 where possible #24274

Merged
merged 6 commits into from
Dec 28, 2018

Conversation

robbuckley
Copy link
Contributor

@robbuckley robbuckley commented Dec 13, 2018

For extension builds, distutils targets the same macOS version as python was built for, even if the host system is newer. This can cause problems when building on a 10.9+ system, with a python built for <10.9. Targetting macOS <10.9 sets the c++ standard library to stdlibc++, which was deprecated in 10.9 in favour of libc++. Starting with Xcode 10, stdlibc++ is no longer installed by default, so isn't guaranteed to be available. This can be an issue with the current 64/32b universal builds of python, which target macOS 10.7.

Another reason to do this is to move over to 10.9 / libc++ where possible, as the current 64-bit builds of python have done.

@pep8speaks
Copy link

Hello @robbuckley! Thanks for submitting the PR.

  • There are no PEP8 issues in the file setup.py !

@codecov
Copy link

codecov bot commented Dec 13, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24274   +/-   ##
=======================================
  Coverage   92.29%   92.29%           
=======================================
  Files         162      162           
  Lines       51841    51841           
=======================================
  Hits        47847    47847           
  Misses       3994     3994
Flag Coverage Δ
#multiple 90.7% <ø> (ø) ⬆️
#single 42.98% <ø> (ø) ⬆️

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 5d134ec...f8c89c2. Read the comment docs.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks for the research on this PR. I have some general code comments though I'm curious if you have thoughts nohow to manage such a change long term, i.e. how will we know if / when such a directive is no longer necessary.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@WillAyd WillAyd added Build Library building on various platforms OSX labels Dec 14, 2018
@TomAugspurger
Copy link
Contributor

Thanks for looking into this.

Do you know if this affects libraries like NumPy as well?

@robbuckley
Copy link
Contributor Author

robbuckley commented Dec 15, 2018

@TomAugspurger I'd expect anything that uses the c++ standard lib to be affected. I cloned numpy and it builds fine for me without any workaround for this issue. I guess they don't use the c++ standard lib. I tried to check this by running otool -L on all the .a and .so files to look for linkages to libc++ and there are none. The same check on my pandas branch gives 6 objects that link to libc++.1.dylib:

./build/lib.macosx-10.7-x86_64-3.7/pandas/io/msgpack/_packer.cpython-37m-darwin.so:
./build/lib.macosx-10.7-x86_64-3.7/pandas/io/msgpack/_unpacker.cpython-37m-darwin.so:
./build/lib.macosx-10.7-x86_64-3.7/pandas/_libs/window.cpython-37m-darwin.so:
./pandas/io/msgpack/_unpacker.cpython-36m-darwin.so:
./pandas/io/msgpack/_packer.cpython-36m-darwin.so:
./pandas/_libs/window.cpython-36m-darwin.so:

If there are other libs you'd like to me to check I'm happy to take a look

@robbuckley
Copy link
Contributor Author

@WillAyd on long term maintenance: once python targetting macOS 10.9+ becomes the norm, my change will stop having any effect. At some point after that, you could decide to drop support for sub-10.9 systems. 10.9 is >4 years old now, so maybe this isnt so far away? For example Chrome requires 10.10, Firefox 10.9

@robbuckley
Copy link
Contributor Author

BTW, for building wheels for distribution, I suppose you want to keep compatibility with older systems, at least for now. You may need to set MACOSX_DEPLOYMENT_TARGET in the script that builds the wheels

@TomAugspurger
Copy link
Contributor

Thanks for the explanation.

Anyone have concerns with this? (@jreback @chris-b1)?

@jreback
Copy link
Contributor

jreback commented Dec 16, 2018

let me look

@jreback
Copy link
Contributor

jreback commented Dec 17, 2018

so there is this section lowerdown

try:
    from wheel.bdist_wheel import bdist_wheel

    class BdistWheel(bdist_wheel):
        def get_tag(self):
            tag = bdist_wheel.get_tag(self)
            repl = 'macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64'
            if tag[2] == 'macosx_10_6_intel':
                tag = (tag[0], tag[1], repl)
            return tag
    cmdclass['bdist_wheel'] = BdistWheel
except ImportError:
    pass

I would say this is 'causing' this issue so likely should just fix this

@robbuckley
Copy link
Contributor Author

hi, I dont think the bdist_wheel stuff is directly related to the issue this PR is trying to address (#23424 ) , which happens with any kind of build -- for example python setup.py build_ext not just when building wheels.

@jreback
Copy link
Contributor

jreback commented Dec 18, 2018

so I would ideally like to test this with our wheel builder: https://travis-ci.org/MacPython/pandas-wheels. @robbuckley might be non-trivial to do this, basically you would fork this repo then update the hash to build from this PR. Alternatively we could merge this and make sure the wheels build, reverting if there is a problem.

@robbuckley
Copy link
Contributor Author

I’ll give the wheel build a try on the PR branch.

@jreback jreback added this to the 0.24.0 milestone Dec 19, 2018
@jreback
Copy link
Contributor

jreback commented Dec 19, 2018

@robbuckley pls also add a note in the whatsnew (Other section is fine)

@robbuckley
Copy link
Contributor Author

robbuckley commented Dec 20, 2018

@jreback ive pushed a whatsnew change, is it OK? I've forked the wheel building repo and will try to get this tested in the next day or 2.

@jreback
Copy link
Contributor

jreback commented Dec 25, 2018

@robbuckley pls indicate if the wheel building is successful.

@robbuckley
Copy link
Contributor Author

hi @jreback I forked https://github.com/MacPython/pandas-wheels, made a branch and updated the pandas submodule to point to the head of my PR branch. 4 out of 11 builds failed. I reran the fails, same result. See https://github.com/robbuckley/pandas-wheels/runs/43815545.

Build #2 ( macosx ) failed early, in the the multibuild scripts. It looks like the same failure is on the head of https://github.com/MacPython/pandas-wheels

The other fails aren't on the branch I forked from. They dont seem obviously related to my change: since they are all in linux builds, the code in my change shouldn't be active when building those targets.

My thoughts on how i would investigate: branch pandas-wheels from the last passing commit, rebase my changes onto the corresponding pandas commit.

Details of the other 3 fails:

Builds 3 and 4 (linux) failed later, with ERROR collecting tests/indexes/datetimes/test_tools.py

Build 5 failed with the following report.
E hypothesis.errors.Flaky: Hypothesis test_tick_add_sub(cls=Second, n=-492, m=492) produces unreliable results: Falsified on the first call but did not on a subsequent one /venv/lib/python3.5/site-packages/hypothesis/core.py:797: Flaky ---------------------------------- Hypothesis ---------------------------------- Falsifying example: test_tick_add_sub(cls=Second, n=-492, m=492) Unreliable test timings! On an initial run, this test took 1240.61ms, which exceeded the deadline of 500.00ms, but on a subsequent run it took 0.13 ms, which did not. If you expect this sort of variability in your test timings, consider turning deadlines off for this test by setting deadline=None.

@jreback
Copy link
Contributor

jreback commented Dec 26, 2018

should have mentioned
run on the daily branch

the natale branch only gets updated on release

@robbuckley
Copy link
Contributor Author

robbuckley commented Dec 26, 2018 via email

@robbuckley
Copy link
Contributor Author

hi, wheel building passed.

I rebased my changes onto 0.24.0.dev0+1370.g08c920eab which passed on the daily wheel builder yesterday (https://travis-ci.org/MacPython/pandas-wheels/builds/472537433). My rebased changes passed on my fork of the wheel builder (https://travis-ci.com/robbuckley/pandas-wheels/builds/95843192)

@jreback
Copy link
Contributor

jreback commented Dec 28, 2018

thanks for putting in all of this effort @robbuckley !

@jreback jreback merged commit 459ebb2 into pandas-dev:master Dec 28, 2018
thoo added a commit to thoo/pandas that referenced this pull request Dec 28, 2018
* upstream/master: (26 commits)
  DOC: Fixing doc upload (no such remote origin) (pandas-dev#24459)
  BLD: for C extension builds on mac, target macOS 10.9 where possible (pandas-dev#24274)
  POC: _eadata (pandas-dev#24394)
  DOC: Correct location (pandas-dev#24456)
  CI: Moving CircleCI build to Travis (pandas-dev#24449)
  BUG: Infer compression by default in read_fwf() (pandas-dev#22200)
  DOC: Fix minor typo in whatsnew (pandas-dev#24453)
  DOC: Add dateutil to intersphinx pandas-dev#24437 (pandas-dev#24443)
  DOC: Adding links to offset classes in timeseries.rst (pandas-dev#24448)
  DOC: Adding offsets to API ref (pandas-dev#24446)
  DOC: fix flake8 issue in groupby.rst (pandas-dev#24363)
  DOC: Fixing more doc warnings (pandas-dev#24438)
  API: Simplify repeat signature (pandas-dev#24447)
  BUG: to_datetime(Timestamp, utc=True) localizes to UTC (pandas-dev#24441)
  CLN: Cython Py2/3 Compatible Imports (pandas-dev#23940)
  DOC: Fixing more doc warnings (pandas-dev#24431)
  DOC: Removing old release.rst (pandas-dev#24427)
  BUG-24408 Series.dt does not maintain own copy of index (pandas-dev#24426)
  DOC: Fixing several doc warnings (pandas-dev#24430)
  ENH: fill_value argument for shift pandas-dev#15486 (pandas-dev#24128)
  ...
javabrett added a commit to javabrett/pyemd that referenced this pull request Jun 10, 2019
…10.9.

Modified setup.py to set env MACOSX_DEPLOYMENT_TARGET=10.9 when the build
MacOS is that or newer, but the default MACOSX_DEPLOYMENT_TARGET (when absent)
is <10.9, which happens when the running Python targets <10.9 min.

This happens for example when running a current Anaconda Python, which
has target 10.7.

This fix taken verbatim from similar in Pandas,
pandas-dev/pandas#24274 .

Fixed wmayner#39.
pquentin added a commit to clustree/noahong that referenced this pull request Apr 20, 2020
pquentin added a commit to clustree/noahong that referenced this pull request May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build failure with Xcode 10 - libstdc++ not supported anymore
5 participants