Skip to content

Commit ecb65c4

Browse files
author
Release Manager
committed
sagemathgh-37886: Remove pointless rpaths on macOS; make sage-env polite when run on systems with no toolchain <!-- ^ Please provide a concise and informative title. --> This PR does the following (after splitting out the Python upgrade to [sagemath#37914](sagemath#37914)): 1. Fix a comment in the Makefile about how to use the DEBUG flag so it will actually work. 3. Edit sage-env so that it does not print errors to stderr and/or open a dialog whenever it is run on a system which has neither XCode nor command line tools installed. 4. Remove (almost) all uses of rpath on macOS (see below). 5. Fix the primecountpy spkg, which was the only one which actually needed an rpath in its python extension module. **About rpaths on macOS** Sage has been misusing rpaths on macOS for a very long time. As far as I can tell, this was an error based on the incorrect assumption that MACH binaries use rpath in the same way as ELF binaries. The rpath in an ELF binary provides a search path, beyond the default search path, where the loader looks for shared libraries needed by the binary. A MACH binary specifies the dynamic libraries it needs with load commands of type LC_LOAD_DYLIB. The value of a LC_LOAD_DYLIB load command is a path to a specific library. There is one such load command for each library that the binary needs. The path is an absolute path by default. However, the path is allowed to begin with the substring `@rpath`. When the binary is loaded that substring is replaced by the value of each LC_RPATH load command in the binary to produce a list of absolute paths (or relative paths if the value of the LC_RPATH begins with `@loader_path`). Each of the paths on that list is tried as a possible location of the needed dynamic library. An obvious consequence of this design is that if none of the LC_LOAD_DYLIB paths contains the string `@rpath` then there is no need for any LC_RPATH commands, since they won't be used. With one exception (primecountpy) every MACH binary produced when building Sage had absolute paths for its LC_LOAD_DYLIB load commands. Nonetheless Sage was setting the rpath to $SAGE_LOCAL/lib in all of these binaries. In fact, it was doing that multiple times, producing many duplicate rpaths all of which were useless. (And duplicate rpaths are now deprecated by Apple's linker.) This PR removes all of those bogus rpaths. **About xcode-select** The sage-env script was calling gcc to find the names of its linker and archiver in the unlikely case that either of those had a non-standard name, and it was also calling xcode-select as part of a check to see whether the selected XCode toolchain had an ld-classic executable. That executable was added in XCode 15 as a new name for the old linker when a new linker was added. Apple made it possible to use the old linker by passing the linker option -ld_classic to the gcc command. The new linker was buggy in the beginning and was unable to link openblas. So numpy and scipy use the old linker. Sage also wanted to use it. While the new linker is probably working well enough now as a linker, it generates a warning when it encounters duplicate libraries in a link command. It also appears that the new linker, or XCode 15 itself, somehow *creates* duplicate libraries. While the warnings should be innocuous, they were causing Sage doctest failures. So this PR provides code which adds -Wl,-ld_classic to LDFLAGS in sage-env to replace the analogous code which was already there. But the new code is careful to redirect the error messages to /dev/mull so they do not get printed on the users terminal whenever sage starts up on a machine which does not have any XCode toolchain at all. It also avoids calling gcc in that case since doing so posts a dialog on the user's screen asking if they would like to install command line tools. The dialog is a nice touch, but should only appear if the user is actually *using* gcc, not as part of every invocation of Sage, especially since XCode uses the standard names. <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37886 Reported by: Marc Culler Reviewer(s): John H. Palmieri, Marc Culler, Matthias Köppe, Volker Braun
2 parents 506ea81 + 3f1894e commit ecb65c4

File tree

6 files changed

+50
-14
lines changed

6 files changed

+50
-14
lines changed

build/make/Makefile.in

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
# Always use bash for make rules
1818
SHELL = @SHELL@
1919

20+
ifndef DEBUG_RULES
2021
# Check a variable that is only set in build/make/install, but not in sage-env, for example
2122
ifndef SAGE_PKGCONFIG
2223
# Set by build/bin/sage-sdist, which invokes the Makefile directly in
@@ -25,6 +26,7 @@ ifndef SAGE_SPKG_COPY_UPSTREAM
2526
$(error This Makefile needs to be invoked by build/make/install)
2627
endif
2728
endif
29+
endif
2830

2931
# Directory to keep track of which packages are installed - relative to installation prefix
3032
SPKG_INST_RELDIR = var/lib/sage/installed

build/pkgs/bliss/spkg-install.in

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
if [ "$UNAME" = "Darwin" ]; then
2+
LDFLAGS="${LDFLAGS} -Wl,-rpath,${SAGE_LOCAL}/lib"
3+
export LDFLAGS
4+
fi
15
cd src
26
sdh_cmake -DUSE_GMP=OFF -DCMAKE_VERBOSE_MAKEFILE=ON
37
sdh_make

build/pkgs/primecount/spkg-install.in

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
if [ "$UNAME" = "Darwin" ]; then
2+
LDFLAGS="${LDFLAGS} -Wl,-rpath,${SAGE_LOCAL}/lib"
3+
export LDFLAGS
4+
fi
15
cd src
26

37
# Issue #33054: primecount needs "-std=gnu++..."
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,7 @@
1+
if [ "$UNAME" = "Darwin" ]; then
2+
LDFLAGS="${LDFLAGS} -Wl,-rpath,${SAGE_LOCAL}/lib"
3+
export LDFLAGS
4+
fi
5+
16
cd src
27
sdh_pip_install .
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,7 @@
1+
if [ "$UNAME" = "Darwin" ]; then
2+
LDFLAGS="${LDFLAGS} -Wl,-rpath,${SAGE_LOCAL}/lib"
3+
export LDFLAGS
4+
fi
5+
16
cd src
27
sdh_pip_install .

src/bin/sage-env

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -370,22 +370,38 @@ if [ -n "$PYTHONHOME" ]; then
370370
fi
371371

372372
if [ -n "$SAGE_LOCAL" ]; then
373-
# On OS X, test whether "ld-classic" is present in the installed
374-
# version of the command-line tools. If so, we add "-ld_classic"
375-
# to LD_FLAGS. See #36599.
376-
# When the full XCode is installed and in use, for example after
377-
# "sudo xcode-select -s /Applications/Xcode.app", then "xcode-select -p"
378-
# gives "/Applications/Xcode.app/Contents/Developer", but "ld-classic"
379-
# is not in the subdirectory "usr/bin/" but rather in the subdirectory
380-
# "Toolchains/XcodeDefault.xctoolchain/usr/bin/". See #37237.
381-
# However, if LD is set explicitly, as it is within conda on macOS,
382-
# do not not do this.
383-
if [ "$UNAME" = "Darwin" ] && [ -z "$LD" ] && [ -x "$(xcode-select -p)/usr/bin/ld-classic" -o -x "$(xcode-select -p)/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld-classic" ] ; then
384-
LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-ld_classic,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
385-
else
386-
LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
373+
# Construct and export LDFLAGS
374+
if [ "$UNAME" = "Darwin" ]; then
375+
LDFLAGS="-L$SAGE_LOCAL/lib $LDFLAGS"
376+
# On OS X, use the old linker if it is available.
377+
# if "ld-classic" is present in the selected XCode
378+
# toolchain, add "-Wl,-ld_classic" to LDFLAGS (see #36599) unless
379+
# LD is already set, as it will be with conda on macOS. When the
380+
# selected toolchain is in the Xcode app the output of "xcode-select -p"
381+
# is "/Applications/Xcode.app/Contents/Developer", but "ld-classic" is
382+
# not in the subdirectory "usr/bin/" but rather in the subdirectory
383+
# "Toolchains/XcodeDefault.xctoolchain/usr/bin/". (See #37237.)
384+
if [ -z "$LD" ]; then
385+
# Running xcode-select on a system with no toolchain writes an
386+
# error message to stderr, so redirect stderr to /dev/null.
387+
XCODE_PATH=$(/usr/bin/xcode-select -p 2> /dev/null)
388+
if [ -n $XCODE_PATH ]; then
389+
if [ -x "$XCODE_PATH/usr/bin/ld-classic" -o \
390+
-x "$XCODE_PATH/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld-classic" ]; then
391+
LDFLAGS="$LDFLAGS -Wl,-ld_classic"
392+
fi
393+
else
394+
# On a macOS system with no toolchain we don't want this script
395+
# to call gcc because that will also print an error message to
396+
# stderr. We can avoid this by setting AS and LD to their
397+
# default values.
398+
AS=as
399+
LD=ld
400+
fi
401+
fi
387402
fi
388403
if [ "$UNAME" = "Linux" ]; then
404+
LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
389405
LDFLAGS="-Wl,-rpath-link,$SAGE_LOCAL/lib $LDFLAGS"
390406
fi
391407
export LDFLAGS

0 commit comments

Comments
 (0)