Opened 3 years ago

Closed 3 years ago

Last modified 2 years 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, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

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 3 years ago.
py3.patch​ (1.3 KB) - added by dimpase 3 years ago.

Download all attachments as: .zip

Change History (22)

Changed 3 years ago by dimpase

Changed 3 years ago by dimpase

comment:1 Changed 3 years 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 3 years ago by dimpase

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

comment:3 Changed 3 years ago by dimpase

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

comment:4 follow-up: Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:16 in reply to: ↑ 13 Changed 3 years 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 3 years 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 3 years 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 3 years 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 2 years ago by mkoeppe

  • Commit 99d842d7a8aa34afd14fb575258550d6e6ac15df deleted

Follow-up: #29019

Note: See TracTickets for help on using tickets.