Opened 11 months ago

Closed 10 months ago

Last modified 6 weeks ago

#27631 closed defect (fixed)

patch pythons to allow MacOS builds without /usr/include

Reported by: dimpase Owned by:
Priority: critical Milestone: sage-8.8
Component: build Keywords:
Cc: embray, vbraun, jhpalmieri Merged in:
Authors: Dima Pasechnik Reviewers: Erik Bray
Report Upstream: Not yet reported upstream; Will do shortly. Work issues:
Branch: 99d842d (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

this is a followup to #26899, where a stop-gap was implemented to let Sage build on a /usr/include-less OSX.

It turns out it is pretty easy to provide such a fix.

Attachments (2)

py2.patch​ (1.6 KB) - added by dimpase 11 months ago.
py3.patch​ (1.3 KB) - added by dimpase 11 months ago.

Download all attachments as: .zip

Change History (22)

Changed 11 months ago by dimpase

Changed 11 months ago by dimpase

comment:1 Changed 11 months ago by dimpase

One also has to do something to pillow: 

The headers or library files could not be found for zlib,
a required dependency when compiling Pillow from source.

comment:2 Changed 11 months ago by dimpase

  • Branch set to u/dimpase/packages/pythonOSXfix
  • Commit set to 7dbd59b9a41d096e3f91e269925a80817eb641bc

comment:3 Changed 11 months ago by dimpase

  • Cc embray vbraun jhpalmieri added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 follow-up: Changed 11 months ago by embray

  • Reviewers set to Erik Bray

This looks excellent, thank you. Positive review from me in principle, but we should also get some feedback from people who are better situated to actually test it.

One thing you might consider (and maybe you did and I'm missing something), in the ​patch to pillow, rather than use subprocess to call xcrun again, given that this is a Python package why not just use sysconfig.get_config_var('Py_MACOS_SYSROOT') as is done in the setup.py for Python itself? That way you ensure it's using the same MACOS_SYSROOT as the Python you're installing it on (in case for some odd reason it since changed).

comment:5 Changed 11 months ago by dimpase

I tested this on an OSX 10.14 with command-line tools-only installation of Xcode. It would be great to have this tested on OSX with full-blown Xcode.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 11 months ago by embray

Replying to embray:

One thing you might consider (and maybe you did and I'm missing something), in the ​patch to pillow, rather than use subprocess to call xcrun again, given that this is a Python package why not just use sysconfig.get_config_var('Py_MACOS_SYSROOT') as is done in the setup.py for Python itself? That way you ensure it's using the same MACOS_SYSROOT as the Python you're installing it on (in case for some odd reason it since changed).

And this?

comment:7 in reply to: ↑ 6 Changed 11 months ago by dimpase

Replying to embray:

Replying to embray:

One thing you might consider (and maybe you did and I'm missing something), in the ​patch to pillow, rather than use subprocess to call xcrun again, given that this is a Python package why not just use sysconfig.get_config_var('Py_MACOS_SYSROOT') as is done in the setup.py for Python itself? That way you ensure it's using the same MACOS_SYSROOT as the Python you're installing it on (in case for some odd reason it since changed).

Right, I forgot that it’s a normal Python call. I will change this as soon as I am back to the keyboard

And this?

comment:8 follow-up: Changed 11 months ago by git

  • Commit changed from 7dbd59b9a41d096e3f91e269925a80817eb641bc to 461d82ca69387491419166421c89829f9edb59e4

Branch pushed to git repo; I updated commit sha1. New commits:

​461d82cuse already configured location of OSX headers

comment:9 in reply to: ↑ 8 Changed 11 months ago by dimpase

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

​461d82cuse already configured location of OSX headers

this addresses comment:6

comment:10 follow-up: Changed 11 months ago by jhpalmieri

This works for me on an OS X 10.14.4 system for which the change at #26899 is otherwise necessary. I have a full Xcode installation plus my own copy of gfortran. With the branch here, all tests pass with a Python 2 build, and make ptest-python3 passes its tests also.

What are the chances that the Python 3.x patch will be accepted? Is there any reason to wait for progress on ​https://bugs.python.org/issue36231?

comment:11 Changed 11 months ago by embray

I guess one last thing to add would be to bump the patch level in the pillow package-version.txt.

Although rebuilding python might force a rebuild of pillow, it would still be good to be explicit. Then you can set to positive review as far as I'm concerned.

comment:12 in reply to: ↑ 10 Changed 11 months ago by embray

Replying to jhpalmieri:

This works for me on an OS X 10.14.4 system for which the change at #26899 is otherwise necessary. I have a full Xcode installation plus my own copy of gfortran. With the branch here, all tests pass with a Python 2 build, and make ptest-python3 passes its tests also.

What are the chances that the Python 3.x patch will be accepted? Is there any reason to wait for progress on ​https://bugs.python.org/issue36231?

I haven't noticed any action on this upstream, but I (or Dima) should make a PR for his patch. I think it's quite good and has a chance of being accepted; or at the very least making a PR might help spur action upstream.

In the meantime I don't think we should wait. This is a problem for us now, and I have no problem with patching build system stuff.

comment:13 follow-ups: Changed 11 months ago by dimpase

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

I'll make an upstream PR, no problem.

comment:14 Changed 11 months ago by git

  • Commit changed from 461d82ca69387491419166421c89829f9edb59e4 to 99d842d7a8aa34afd14fb575258550d6e6ac15df

Branch pushed to git repo; I updated commit sha1. New commits:

​99d842dbump up pillow version

comment:15 Changed 11 months ago by dimpase

  • Status changed from needs_review to positive_review

comment:16 in reply to: ↑ 13 Changed 11 months ago by dimpase

Replying to dimpase:

I'll make an upstream PR, no problem.

There is a new fun OSX bug, breaking testsuite on 10.14.4: ​https://bugs.python.org/issue36432

(I also got it after trying to make a PR for upstream: ​https://github.com/dimpase/cpython/commit/41617bd6cd957995ec9e1f89558fbdce5f78bc79)

comment:17 Changed 11 months ago by jhpalmieri

Joke's on them. We haven't run the Python test suite in years because it always fails.

comment:18 in reply to: ↑ 13 Changed 10 months ago by dimpase

Replying to dimpase:

I'll make an upstream PR, no problem.

see ​https://github.com/python/cpython/pull/12825

comment:19 Changed 10 months ago by vbraun

  • Branch changed from u/dimpase/packages/pythonOSXfix to 99d842d7a8aa34afd14fb575258550d6e6ac15df
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 Changed 6 weeks ago by mkoeppe

  • Commit 99d842d7a8aa34afd14fb575258550d6e6ac15df deleted

Follow-up: #29019

Note: See TracTickets for help on using tickets.