sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

OS X Mojave without /usr/include: a range of issues #26899

Closed jhpalmieri closed 5 years ago

jhpalmieri commented 5 years ago

On OS X Mojave, it is possible to have Xcode installed but to have no directory /usr/include.

Upstream: Reported upstream. No feedback yet.

CC: @embray

Component: packages: standard

Author: Dima Pasechnik, Volker Braun

Branch: db50d09

Reviewer: Dima Pasechnik, Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/26899

embray commented 5 years ago
comment:50

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

dimpase commented 5 years ago
comment:51

Replying to @vbraun:

Or we can make build/pkgs/zlib/spkg-configure.m4 less clever. The cost for building zlib unnecessary is a few seconds walltime, and the cost for not building zlib when required is total failure.

what we see in comment 11 above is that spkg-configure.m4 already succeeds without even trying to look into any of /usr, /usr/local, etc.

So this is obviously a bug in Python that they resort to that kind of hackery without trying to check whether the compiler/linker already knows where to look.

dimpase commented 5 years ago
comment:52

Replying to @vbraun:

Python's setup.py checks for the presence (header + library) and version of zlib, and will silently skip zlib if it can't be found. This all happens after the configure run.

        zlib_inc = find_file('zlib.h', [], inc_dirs)
...

digging up shows that in find_file() they special-case OSX, and call macosx_sdk_root() to find the starting point, but the latter does something lame, instead of the latest Xcode magic spell xcrun --show-sdk-path

embray commented 5 years ago
comment:53

It sucks because this code has to live, currently, in Python's main source distribution, but something like xcode is such a moving target that it's easy for any special cases made for it can easily become out of sync with Python releases.

For that reason I think it's also perfectly acceptable to patch Python's setup.py in this case, if at least there is a reasonable patch that won't break backwards-compat.

dimpase commented 5 years ago
comment:54

xcrun has been there since OSX 10.9, so it's pretty safe to use, at least for SageMath. Note that Python's info on the headers on OSX is outdated. https://devguide.python.org/setup/#macos-and-os-x

Should we open a bug with them?

jhpalmieri commented 5 years ago
comment:55

Once again, why don't we just remove build/pkgs/zlib/spkg-configure.m4? Or make the check platform-dependent, so it always builds on OS X >= 10.14. On this computer, building zlib takes 10 seconds, so building it when not necessary is not a big deal.

I like the idea of being able to use system libraries, and if zlib were one of the last pieces, then we should try very hard to fix this. But for now, given a choice between building zlib when it may not be necessary vs. patching Python, I would prefer building zlib.

dimpase commented 5 years ago
comment:56

"just" removing build/pkgs/zlib/spkg-configure.m4 means that anything that depends upon zlib must be built as well.

Besides, I have a draft patch for Python(s) ready (this is diff to CPython master, thus probably needs a rebase for other Pythons)

diff --git a/setup.py b/setup.py
index b96961073b..e1dbd71bb4 100644
--- a/setup.py
+++ b/setup.py
@@ -134,7 +134,8 @@ def macosx_sdk_root():
     cflags = sysconfig.get_config_var('CFLAGS')
     m = re.search(r'-isysroot\s+(\S+)', cflags)
     if m is None:
-        sysroot = '/'
+        import subprocess
+        sysroot = subprocess.check_output(["xcrun", "--show-sdk-path"]).decode("utf-8").strip('\n')
     else:
         sysroot = m.group(1)
     return sysroot
@@ -146,6 +147,7 @@ def is_macosx_sdk_path(path):
     """
     return ( (path.startswith('/usr/') and not path.startswith('/usr/local'))
                 or path.startswith('/System/')
+                or path.startswith('/Applications/')
                 or path.startswith('/Library/') )

This allows building zlib module on CPython master. I guess I'll wrap it into try:...

jhpalmieri commented 5 years ago
comment:57

Replying to @dimpase:

"just" removing build/pkgs/zlib/spkg-configure.m4 means that anything that depends upon zlib must be built as well.

I'm curious: how many packages does that affect?

dimpase commented 5 years ago
comment:58

Replying to @jhpalmieri:

Replying to @dimpase:

"just" removing build/pkgs/zlib/spkg-configure.m4 means that anything that depends upon zlib must be built as well.

I'm curious: how many packages does that affect?

there are executables that certainly may use whatever zlib, so apologies for not being precise here, but still this leaves

libpng, iconv, libpng, glpk, cbc, and all of its dependent libraries, such as freetype, libgd, etc.

And it rules out using system-wide Python (this is far from being ready, but in principle not much should prevent one to build Sage in a virtual environment of a system-provided Python).

dimpase commented 5 years ago
comment:59

I've opened https://bugs.python.org/issue36231 to discuss this with the upstream.

dimpase commented 5 years ago

Upstream: Reported upstream. No feedback yet.

jhpalmieri commented 5 years ago
comment:60

With this patch, Python 2 and 3 both fail to build for me (this is on the machine with no /usr/include and for which I've been having to build Sage's zlib; to test this, I didn't build Sage's zlib).

Python 2:

running build
running build_ext
Traceback (most recent call last):
  File "./setup.py", line 2311, in <module>
    main()
  File "./setup.py", line 2306, in main
    'Lib/smtpd.py']
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/command/build.py", line 127, in run
    self.run_command(cmd_name)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/cmd.py", line 326, in run_command
    self.distribution.run_command(command)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/distutils/command/build_ext.py", line 340, in run
    self.build_extensions()
  File "./setup.py", line 181, in build_extensions
    missing = self.detect_modules()
  File "./setup.py", line 822, in detect_modules
    search_for_ssl_incs_in
  File "./setup.py", line 80, in find_file
    sysroot = macosx_sdk_root()
  File "./setup.py", line 50, in macosx_sdk_root
    import subprocess
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python2-2.7.15.p0/src/Lib/subprocess.py", line 72, in <module>
    import select
ImportError: No module named select

Python 3:

running build
running build_ext
Traceback (most recent call last):
  File "./setup.py", line 2404, in <module>
    main()
  File "./setup.py", line 2399, in main
    "Tools/scripts/2to3", "Tools/scripts/pyvenv"]
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/dist.py", line 955, in run_commands
    self.run_command(cmd)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/command/build.py", line 135, in run
    self.run_command(cmd_name)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/distutils/command/build_ext.py", line 339, in run
    self.build_extensions()
  File "./setup.py", line 230, in build_extensions
    missing = self.detect_modules()
  File "./setup.py", line 852, in detect_modules
    search_for_ssl_incs_in
  File "./setup.py", line 126, in find_file
    sysroot = macosx_sdk_root()
  File "./setup.py", line 96, in macosx_sdk_root
    import subprocess
  File "/Users/jpalmier/Desktop/Sage_stuff/sage_builds/TESTING/sage-8.7.beta6/local/var/tmp/sage/build/python3-3.6.6.p0/src/Lib/subprocess.py", line 136, in <module>
    import _posixsubprocess
ModuleNotFoundError: No module named '_posixsubprocess'

The problem seems to be the new line import subprocess in setup.py. I actually don't know how setup.py is supposed to work: how much of Python's functionality can you expect to be working when it is called?

dimpase commented 5 years ago
comment:61

Sorry, it's actually not a complete patch. See https://bugs.python.org/issue36231 for a complete one for Python 3 (although it might not cleanly apply to Sage's Python 3, as it's for Python's master).

OK, I must say I misunderstood the issue a bit - that's why on Python bug tracker I posted a patch which simply triggers building of _posixsubprocess and a bunch of other modules (some of them needed for just testing).

Surely using subprocess may be substituted by something available at this point, I just don't know without looking into it more what it should be.

jhpalmieri commented 5 years ago
comment:62

Okay, I see. I got Python 3 to build successfully, but when I tried something similar for Python 2, it builds but without the zlib module. (When I say "something similar", I mean that I patched Modules/Setup.dist until import subprocess in setup.py stopped giving errors.) Maybe I'm doing something wrong, and in any case, it's a good sign that it works for Python 3.

vbraun commented 5 years ago
comment:63

How likely is it that upstream will fix this for Python 2? My hunch is that they won't, so I would be fine with shipping whatever patch needed to make it work.

embray commented 5 years ago
comment:64

Moving all blocker/critical issues from 8.7 to 8.8.

embray commented 5 years ago
comment:65

Yep, backporting patches to Python 2.7 for build issues makes perfect sense.

embray commented 5 years ago
comment:66

Ned Deily wrote on the BPO issue that he's working on a potential fix but didn't really explain his approach; absent that I can look into it. I think something like Dima's patch would work, but it should be run at ./configure time. Just need to check for the xcrun program and use it if so; if it doesn't exist (e.g. on older OSX) it's fine, because there the issue is not relevant anyways.

embray commented 5 years ago
comment:67

One thing I'm concerned about is that I don't know exactly how to reproduce this issue, in particular given that the one OSX machine I currently have access to already has a /usr/include/zlib.h for some reason.

dimpase commented 5 years ago
comment:68

Replying to @embray:

One thing I'm concerned about is that I don't know exactly how to reproduce this issue, in particular given that the one OSX machine I currently have access to already has a /usr/include/zlib.h for some reason.

One can do

sudo mv /usr/include /usr/blah 
sudo mv /usr/local/include /usr/local/blah 
embray commented 5 years ago
comment:69

The machine I have access to is one of the Sage buildbots, so I don't want to break anything like that :)

vbraun commented 5 years ago

Branch: u/vbraun/os_x_mojave_without__usr_include__python_builds_without_zlib_support

vbraun commented 5 years ago

Commit: 3d49fc2

vbraun commented 5 years ago
comment:71

gfortran still dies with

The directory that should contain system headers does not exist:
  /usr/include

even after deleting build/pkgs/zlib/spkg-configure.m4 (actually I removed the corresponding section from ./configure since autotools are not installed)


New commits:

3d49fc2Delete the zlib spkg-configure.m4
dimpase commented 5 years ago
comment:72

huh? just fix it, don't delete it! It calls a crappy macro m4/ax_check_zlib.m4, basically lifted off the net, it can be improved to get rid of an explicit looping through directories.

dimpase commented 5 years ago
comment:73

By the way, ​https://bugs.python.org/issue36231 shows that it's perfectly possible to build Python on OSX with zlib from Xcode.

And see comment:58 why using system's zlib is a good idea.

jhpalmieri commented 5 years ago
comment:74

Any estimates on when a fix will be available? If it's going to take a while, I suggest using Volker's suggestion for now and then reimplementing build/pkgs/zlib/spkg-configure.m4 when the fix is ready.

dimpase commented 5 years ago
comment:75

I only have access to an extremely slow, 8 y.o. OSX machine, and it cannot run 10.14, only 10.13. And I neither use OSX for work nor am paid for porting Python on OSX...

I think Volker’s “fix” must not go through in this form. Make it conditional for this corner case of building for on a particularly bare OSX configuration.

dimpase commented 5 years ago
comment:76

Replying to @vbraun:

gfortran still dies with

The directory that should contain system headers does not exist:
  /usr/include

gcc has --with-native-system-header-dir= option (not shown in the output of ./configure -h, but present in docs and in configure itself. One can use already mentioned. xcrun --show-sdk-path call to compute the value to be passed.

dimpase commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
-On OS X Mojave, it is possible to have Xcode installed but to have no directory `/usr/include`. The configure check from #26286 finds zlib somewhere, but the Python build does not: Python reports that it has built without zlib support. Without zlib, pip will not build.
+On OS X Mojave, it is possible to have Xcode installed but to have no directory `/usr/include`. 
+
+* The configure check from #26286 finds zlib somewhere, but the Python build does not: Python reports that it has built without zlib support. Without zlib, pip will not build.
+
+* gcc/gfortran needs the location of headers to be explicitly passed via `--with-native-system-header-dir=` option. (untested)
vbraun commented 5 years ago
comment:78

PS: the osx buildbot worker had his /usr/include wiped in the last osx update

dimpase commented 5 years ago
comment:79

I'm seting up a remote paid-for (from the grant, hopefully) Mac mini with 10.14, hopefully I'd sort these issues out.

jhpalmieri commented 5 years ago
comment:80

Replying to @dimpase:

Replying to @vbraun:

gfortran still dies with

The directory that should contain system headers does not exist:
  /usr/include

gcc has --with-native-system-header-dir= option (not shown in the output of ./configure -h, but present in docs and in configure itself. One can use already mentioned. xcrun --show-sdk-path call to compute the value to be passed.

Using GCC_CONFIGURE="--with-sysroot=`xcrun --show-sdk-path` $GCC_CONFIGURE" worked for me. (I didn't see this earlier because I've installed my own gfortran on each of my OS X machines so I don't have to wait for Sage to build it. But when I hid my copy, I get the same error.)

dimpase commented 5 years ago
comment:81

Replying to @jhpalmieri:

Using GCC_CONFIGURE="--with-sysroot=`xcrun --show-sdk-path` $GCC_CONFIGURE" worked for me. (I didn't see this earlier because I've installed my own gfortran on each of my OS X machines so I don't have to wait for Sage to build it. But when I hid my copy, I get the same error.)

yes, this works (checking on a very bare MacOS 10.14, with nothing except Command Line Tools for Xcode 10.2 installed), thanks:

--- a/build/pkgs/gcc/build-gcc
+++ b/build/pkgs/gcc/build-gcc
@@ -25,6 +25,7 @@ if [ "$UNAME" = "Darwin" ]; then
     # files prior to comparison during bootstrap (broken by Xcode 6.3).
     # See #18156
     GCC_CONFIGURE="--with-build-config=bootstrap-debug $GCC_CONFIGURE"
+    GCC_CONFIGURE="--with-sysroot=`xcrun --show-sdk-path` $GCC_CONFIGURE"
 fi

 # Let Gfortran build on Raspberry Pi using hard floats.

I suppose it should be done conditionally, only if there is no /usr/include present.

vbraun commented 5 years ago
comment:82

I've added that to the branch, now gfortran builds...

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 3d49fc2 to 758e699

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

758e699Add Xcode sdk path
vbraun commented 5 years ago
comment:84

Passes so I'd suggest we merge this to get the OSX buildbot up and running again. Feel free to reintroduce the zlib configure in a later ticket, then we'll be able to test it against a buildbot without the extra headers installed.

dimpase commented 5 years ago
comment:85

If you are really in rush we can add a condition disabling spkg-configure.m4 for zlib in the case there is no /usr/include present. Why is this a problem, it's one if block, right?

I hopefully soon will have a Python fix ready that would make this not necessary.

dimpase commented 5 years ago

Reviewer: Dima Pasechnik

dimpase commented 5 years ago
comment:86

Instead of removing spkg-configure.m4, apply

--- a/build/pkgs/zlib/spkg-configure.m4
+++ b/build/pkgs/zlib/spkg-configure.m4
@@ -5,4 +5,5 @@ SAGE_SPKG_CONFIGURE([zlib], [
         AX_CHECK_ZLIB([], [zlib_cv_libz=no])
     ])
     AS_IF([test "x$zlib_cv_libz" != "xyes"], [sage_spkg_install_zlib=yes])
+    AC_CHECK_FILE([/usr/include/],[],[sage_spkg_install_zlib=yes])
 ])

this disables the usage of zlib from the system if /usr/include/ is absent. I just tested this on MacOS. With this change, please feel free to give it positive review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 758e699 to db50d09

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

25761b7Add Xcode sdk path
db50d09Disable the usage of zlib from the system if /usr/include/ is absent
vbraun commented 5 years ago
comment:88

You can push commits instead of pasting diffs to the chat, you know ;-)

New commits:

25761b7Add Xcode sdk path
db50d09Disable the usage of zlib from the system if /usr/include/ is absent
vbraun commented 5 years ago

Changed reviewer from Dima Pasechnik to Dima Pasechnik, Volker Braun

vbraun commented 5 years ago

Author: Dima Pasechnik, Volker Braun

vbraun commented 5 years ago

Changed branch from u/vbraun/os_x_mojave_without__usr_include__python_builds_without_zlib_support to db50d09

dimpase commented 5 years ago

Changed commit from db50d09 to none

dimpase commented 5 years ago
comment:91

Replying to @dimpase:

If you are really in rush we can add a condition disabling spkg-configure.m4 for zlib in the case there is no /usr/include present. Why is this a problem, it's one if block, right?

I hopefully soon will have a Python fix ready that would make this not necessary.

Patching Python 3 to do this is easy, Erik's suggestion on https://bugs.python.org/issue36231 really makes it a 1-line addition to Python's configure.ac and 2 more lines to setup.py, but backporting this to Python 2 is not straightforward, as it has more brain-dead setup.py, which seems to requires manual adjustments to a bunch of module configurations.

embray commented 5 years ago
comment:92

IMO this is kinda weak, albeit harmless for the time being (though the line added to spkg-configure.m4 should at least have had a comment, but at least we can git blame it easily...). I think Python should be patched instead.