#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: |
Description (last modified by )
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)
Change History (22)
Changed 2 years ago by
Changed 2 years ago by
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
- Branch set to u/dimpase/packages/pythonOSXfix
- Commit set to 7dbd59b9a41d096e3f91e269925a80817eb641bc
comment:3 Changed 2 years ago by
- Cc embray vbraun jhpalmieri added
- Description modified (diff)
- Status changed from new to needs_review
comment:4 follow-up: ↓ 6 Changed 2 years ago by
- 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 2 years ago by
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: ↓ 7 Changed 2 years ago by
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 callxcrun
again, given that this is a Python package why not just usesysconfig.get_config_var('Py_MACOS_SYSROOT')
as is done in thesetup.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 2 years ago by
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 callxcrun
again, given that this is a Python package why not just usesysconfig.get_config_var('Py_MACOS_SYSROOT')
as is done in thesetup.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: ↓ 9 Changed 2 years ago by
- Commit changed from 7dbd59b9a41d096e3f91e269925a80817eb641bc to 461d82ca69387491419166421c89829f9edb59e4
Branch pushed to git repo; I updated commit sha1. New commits:
461d82c | use already configured location of OSX headers
|
comment:9 in reply to: ↑ 8 Changed 2 years ago by
comment:10 follow-up: ↓ 12 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
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, andmake 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: ↓ 16 ↓ 18 Changed 2 years ago by
- 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 2 years ago by
- Commit changed from 461d82ca69387491419166421c89829f9edb59e4 to 99d842d7a8aa34afd14fb575258550d6e6ac15df
Branch pushed to git repo; I updated commit sha1. New commits:
99d842d | bump up pillow version
|
comment:15 Changed 2 years ago by
- Status changed from needs_review to positive_review
comment:16 in reply to: ↑ 13 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
Replying to dimpase:
I'll make an upstream PR, no problem.
comment:19 Changed 2 years ago by
- Branch changed from u/dimpase/packages/pythonOSXfix to 99d842d7a8aa34afd14fb575258550d6e6ac15df
- Resolution set to fixed
- Status changed from positive_review to closed
comment:20 Changed 15 months ago by
- Commit 99d842d7a8aa34afd14fb575258550d6e6ac15df deleted
Follow-up: #29019
One also has to do something to pillow: