Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove pointless rpaths on macOS; make sage-env polite when run on systems with no toolchain #37886

Merged
merged 17 commits into from
May 25, 2024
Merged

Remove pointless rpaths on macOS; make sage-env polite when run on systems with no toolchain #37886

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3d5fc6c
Update python3.11, make Makefile honest, clean up macOS rpath snafu.
culler Apr 27, 2024
4f969ee
typos
culler Apr 27, 2024
17672de
Fix primecountpy, which needs an rpath.
culler Apr 27, 2024
f3df0fb
Merge branch 'sagemath:develop' into develop
culler Apr 27, 2024
02ad303
Avoid overwriting user LDFLAGS
culler Apr 28, 2024
e7338cd
Put /lib in front of user's LDFLAGS
culler Apr 28, 2024
24f2545
Add rpath to LDFLAGS for symengine_py and bliss
culler Apr 29, 2024
64b11b8
Cleaner fix for printing make rules without setting an apparently unr…
culler May 6, 2024
8e72038
Update python3.11, make Makefile honest, clean up macOS rpath snafu.
culler Apr 27, 2024
a30a5e3
typos
culler Apr 27, 2024
e0ad573
Fix primecountpy, which needs an rpath.
culler Apr 27, 2024
38273e6
Avoid overwriting user LDFLAGS
culler Apr 28, 2024
c086cc4
Put /lib in front of user's LDFLAGS
culler Apr 28, 2024
e790efc
Add rpath to LDFLAGS for symengine_py and bliss
culler Apr 29, 2024
2fbd159
Cleaner fix for printing make rules without setting an apparently unr…
culler May 6, 2024
0b9a7dd
Merge branch 'develop' of github.com:culler/sage into develop
culler May 13, 2024
3f1894e
Fix broken logic in sage-env; add missing rpath to primecount spkg
culler May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/make/Makefile.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
# obscure the substance of the actual rules, this file can be debugged by
# running:
#
# $ make -f build/make/Makefile -n DEBUG_RULES=1
# $ make -f build/make/Makefile -n DEBUG_RULES=1 SAGE_PKGCONFIG=1
jhpalmieri marked this conversation as resolved.
Show resolved Hide resolved
#
# This will not actually run any rules (the -n flag) but will print all the
# rules generated from the templates.
Expand Down
5 changes: 5 additions & 0 deletions build/pkgs/primecountpy/spkg-install.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
if [ "$UNAME" = "Darwin" ]; then
LDFLAGS="${LDFLAGS} -Wl,-rpath,${SAGE_LOCAL}/lib"
export LDFLAGS
fi

cd src
sdh_pip_install .
6 changes: 3 additions & 3 deletions build/pkgs/python3/checksums.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
tarball=Python-VERSION.tar.xz
sha1=89ee31611b73dc0c32c178d15aa208734b462c5a
md5=4efe92adf28875c77d3b9b2e8d3bc44a
cksum=2916176597
sha1=a368aeed7a3325e47b55168452c356a8eb27ab50
md5=b353b8433e560e1af2b130f56dfbd973
cksum=534846730
upstream_url=https://www.python.org/ftp/python/VERSION/Python-VERSION.tar.xz
2 changes: 1 addition & 1 deletion build/pkgs/python3/package-version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.11.1
3.11.8
47 changes: 0 additions & 47 deletions build/pkgs/python3/patches/2.6.5-FD_SETSIZE.patch
View file
Open in desktop

This file was deleted.

15 changes: 0 additions & 15 deletions build/pkgs/python3/patches/2.7.3-dylib.patch
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions build/pkgs/python3/patches/3.2.6-no-enable-new-dtags.patch
View file
Open in desktop

This file was deleted.

25 changes: 13 additions & 12 deletions build/pkgs/python3/spkg-build.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,25 +73,26 @@ sdh_configure --enable-shared $PYTHON_CONFIGURE
# we need to provide paths into $SAGE_LOCAL, so that setup.py finds
# the libraries needed for the extension modules - in particular sqlite3.
# (The search code there does not know about CPATH and LIBRARY_PATH.)
make_LDFLAGS="-L. -Wl,-rpath,. -L$SAGE_LOCAL/lib $LDFLAGS"
make_CPPFLAGS="-I$SAGE_LOCAL/include $CPPFLAGS"

# Also, we need to add an rpath, like we do for SAGE_LOCAL in src/bin/sage-env.
# SAGE_INST_LOCAL is the installation hierarchy for the current package
# -- for python3, this is SAGE_VENV.
make_LDFLAGS="-Wl,-rpath,$SAGE_INST_LOCAL/lib $make_LDFLAGS"
if [ "$UNAME" = "Darwin" ]; then
make_LDFLAGS="-L$SAGE_LOCAL/lib -L$SAGE_INST_LOCAL/lib $LDFLAGS"
# This enables python to find libpython
export DYLD_LIBRARY_PATH="."
fi
if [ "$UNAME" = "Linux" ]; then
make_LDFLAGS="-L. -Wl,-rpath,. -L$SAGE_LOCAL/lib $LDFLAGS"
make_CPPFLAGS="-I$SAGE_LOCAL/include $CPPFLAGS"
# Also, we need to add an rpath, like we do for SAGE_LOCAL in src/bin/sage-env.
# SAGE_INST_LOCAL is the installation hierarchy for the current package
# -- for python3, this is SAGE_VENV.
make_LDFLAGS="-Wl,-rpath,$SAGE_INST_LOCAL/lib $make_LDFLAGS"
make_LDFLAGS="-Wl,-rpath-link,$SAGE_INST_LOCAL/lib $make_LDFLAGS"
# This enables python to find libpython
export LD_LIBRARY_PATH="."
fi

sdh_make LDFLAGS="$make_LDFLAGS" CPPFLAGS="$make_CPPFLAGS"

if [ "$UNAME" = "Darwin" ]; then
export DYLD_LIBRARY_PATH="."
else
export LD_LIBRARY_PATH="."
fi

# When building on a case-insensitive filesystem (on any OS, not just Windows)
# the Python executable is output to the build directory as 'python.exe'
if [ -f "python.exe" ]; then
Expand Down
39 changes: 25 additions & 14 deletions src/bin/sage-env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,20 +370,31 @@ if [ -n "$PYTHONHOME" ]; then
fi

if [ -n "$SAGE_LOCAL" ]; then
# On OS X, test whether "ld-classic" is present in the installed
# version of the command-line tools. If so, we add "-ld_classic"
# to LD_FLAGS. See #36599.
# When the full XCode is installed and in use, for example after
# "sudo xcode-select -s /Applications/Xcode.app", then "xcode-select -p"
# gives "/Applications/Xcode.app/Contents/Developer", but "ld-classic"
# is not in the subdirectory "usr/bin/" but rather in the subdirectory
# "Toolchains/XcodeDefault.xctoolchain/usr/bin/". See #37237.
# However, if LD is set explicitly, as it is within conda on macOS,
# do not not do this.
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
LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-ld_classic,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
else
LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replacing -rpath...

Copy link
Contributor Author

@culler culler May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained above, the rpath option is different for MACH binaries than for ELF binaries and there is no purpose in setting an rpath for a MACH binary if it does not have the @rpath string in any of its load paths. This option causes Sage to build MACH binaries with multiple identical unused LC_RPATH load commands. And duplicate LC_RPATH load commands are deprecated by Apple's linker.

Would you please explain why you are adding that rpath option back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading it right, the change at #37982 proposes restoring -rpath in the Linux case, not the Darwin case: essentially restoring line 386. Better to review #37982 than continue the discussion here, though.

Copy link
Contributor Author

@culler culler May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. I thought the suggestion proposed removing the entire red passage, which leaves the current rpaths for linux unchanged but removes the rpaths for Darwin.

I didn't really understand how this PR, which has not been merged, could have caused breakage that another PR would need to correct. And certainly this PR, which makes no changes to linux, could not cause a linux breakage even if it had been merged. Maybe the cross-reference to this PR was a typo.

In any case, I will be happy as long as the global rpath option is removed for macOS (and macOS only) and replaced by adding local rpath options in the few packages that actually need them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I see this on GitHub, I see vbraun, as part of his review process, quoting a change that you made. He is not proposing any changes here. It looks like your original change to sage-env removed an entire "if ... else" block (the old lines 383-386), and he is saying that the "else" part needs to be kept when using Linux: your changes remove these lines:

    # However, if LD is set explicitly, as it is within conda on macOS,
    # do not not do this.
    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
        LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-ld_classic,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
    else
        LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"

and he essentially wants to restore the last line when using Linux.

As far as how it could cause breakage, my guess is that since it had a positive review, he tried to merge it for the next release and saw that broke things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, John. That makes sense. I am certainly capable of having made a mistake like that. I will look for the mistake and commit a fixed version of sage-env.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that I definitely did screw up the logic in sage-env. I committed a fix in which I attempted to separate the linux and Darwin LDFLAGS constructions extremely clearly, so the next person who looks at that file won't get confused the way that I did. I also found that an rpath was needed in the primecount spkg in addition to the one which had already added to primecountpy.

I then built the PR on Ubuntu 22.04 and macOS 14.1.2. The linux build finished cleanly. The macOS build almost worked but failed near the end on the fflas_ffpack-2.5.0 spkg with a linker error reporting one undefined symbol:

[spkg-install] Undefined symbols for architecture x86_64:
[spkg-install]   "Givaro::Integer::operator%(unsigned long long) const", referenced from:
[spkg-install]       Givaro::Modular<double, double, void>::init(double&, Givaro::Integer const&) const in fflas_lvl1.o
[spkg-install]       Givaro::Modular<double, double, void>::init(double&, Givaro::Integer const&) const in fflas_lvl2.o
[spkg-install]       Givaro::Modular<double, double, void>::init(double&, Givaro::Integer const&) const in fflas_lvl3.o

I don't see how that error could be related to this PR, so I think it is ready to go (again).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing givaro symbols is #38002

# On OS X, test whether "ld-classic" is present in the selected XCode
# toolchain. If so, add "-Wl,-ld_classic" to LDFLAGS (see #36599) unless
# LD is already set, as it will be with conda on macOS. When the selected
# toolchain is in the Xcode app the output of "xcode-select -p" is
# "/Applications/Xcode.app/Contents/Developer", but "ld-classic" is not in
# the subdirectory "usr/bin/" but rather in the subdirectory
# "Toolchains/XcodeDefault.xctoolchain/usr/bin/". (See #37237.)
LDFLAGS="-L$SAGE_LOCAL/lib"
mkoeppe marked this conversation as resolved.
Show resolved Hide resolved
if [ "$UNAME" = "Darwin" ] && [ -z "$LD" ]; then
# Running xcode-select on a system with no toolchain writes an
# error message to stderr, so redirect stderr to /dev/null.
XCODE_PATH=$(/usr/bin/xcode-select -p 2> /dev/null)
if [ -n $XCODE_PATH ]; then
if [ -x "$XCODE_PATH/usr/bin/ld-classic" -o \
-x "$XCODE_PATH/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld-classic" ]; then
LDFLAGS="$LDFLAGS -Wl,-ld_classic"
fi
else
# On a macOS system with no toolchain we don't want this script
# to call gcc because that will also print an error message to
# stderr. We can avoid this by setting AS and LD to their
# default values.
AS=as
LD=ld
fi
fi
if [ "$UNAME" = "Linux" ]; then
LDFLAGS="-Wl,-rpath-link,$SAGE_LOCAL/lib $LDFLAGS"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...with rpath-link breaks various programs, see 37982

Copy link
Contributor Author

@culler culler May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...with rpath-link breaks various programs, see 37982

The lines 394-397 shown in the change request above appear in a block like:

if [ "$UNAME" = "Darwin" ] && [ -z "$LD" ]; then
...
fi

So they would not have any effect when building on Fedora, which is what PR #37982 is about. Can you give a specific example of something that breaks on macOS?

Expand Down
Loading