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.

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

fix: Update RPATH and fully guard ORIGIN parameter expansion #277

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Feb 19, 2024

Requires PR #276 to go in first. (Splitting these to keep the commits atomic)

  • Use single quotes and $$ to properly guard ORIGIN when LDFLAGS are injected into Makefiles. e.g. '$$ORIGIN/_fastjet_core/lib' Without this guard, when Make evaluates the variable $ORIGIN will get evaluated as RIGIN. c.f. fix: Use ORIGIN in RPATH inside of Makefiles lgray/staged-recipes#5 for an example.
  • Set RPATH to support both local and cibuildwheel wheel builds.
  • Set LDFLAGS for to be written into the Makefiles generated by both
    the calls to ./autogen.sh and ./configure.

(Wrong commit message from before)

Use of $$ for the parameter expansion of ORIGIN results in a failure to properly resolve $ORIGIN and for LDFLAGS to be set improperly. Use $ORIGIN instead.

c.f. lgray/staged-recipes#3 for this patch being required for a build to succeed for conda-forge/staged-recipes#21052.

The use of $$ was added by @jpivarski in PR #65, so maybe he can elaborate on the need there and if this PR is missing something. A quick scan of the PR makes it seem like

Different parts of the configure/make procedure evaluate it at different levels, so just protecting the $ is not enough: I had to define an environment variable so that $ORIGIN would resolve to "$ORIGIN".

would be the most relevant bit, but that seems to not matter here.

@matthewfeickert matthewfeickert self-assigned this Feb 19, 2024
* Use of '$$' for the parameter expansion of ORIGIN results in
  a failure to properly resolve $ORIGIN and for LDFLAGS to be set
  improperly. Use '$ORIGIN' instead.
@matthewfeickert matthewfeickert force-pushed the fix/change-origin-par-expansion branch from 8499538 to 641f2ac Compare February 20, 2024 04:26
@matthewfeickert matthewfeickert marked this pull request as ready for review February 20, 2024 04:27
Copy link
Collaborator

@chrispap95 chrispap95 left a comment

Choose a reason for hiding this comment

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

I believe the double $$ was there to prevent accidental evaluation. I am not sure if needed but since the tests build fine I am okay with removing it if causes problems elsewhere.

@matthewfeickert matthewfeickert marked this pull request as draft February 21, 2024 06:05
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 21, 2024

I believe the double $$ was there to prevent accidental evaluation

Yeah, I'm now seeing this when it gets injected into Makefiles. I think $$ORIGIN is right but the paths following are wrong and it needs to be

'$$ORIGIN/../_fastjet_core/lib'

as ORIGIN is the path of the executable or shared library itself and not the directory that houses it. So as ORIGIN for fastjet is the _ext.cpython*.so shared library under site-packages (e.g. site-packages/fastjet/_ext.cpython-311-x86_64-linux-gnu.so) then to get to the path of the fastjet shared libraries that are at site-packages/fastjet/_fastjet_core/lib/ then the RPATH needs to be '$$ORIGIN/../_fastjet_core/lib'.

edit: Nope, I'm all wrong! ORIGIN is NOT the path to the executable or shared libary, but the path to the directory that contains the executable or shared library. So an RPATH of

'$$ORIGIN/_fastjet_core/lib:$$ORIGIN'

is correct and my notes to myself in the past were wrong.

(

'$$ORIGIN/_fastjet_core/lib'

picks up the libraries under /_fastjet_core/lib/ needed for site-packages/fastjet/_ext.cpython-3*.so

and the

'$$ORIGIN'

picks up the other /_fastjet_core/lib/ libraries that are needed for shared libraries in there.
)

c.f. conda-forge/staged-recipes#21052 (comment) also

I had forgotten how useful using ldd and objdump -x <path to shared library> | grep ORIGIN are for getting information out of shared library objects.

* Set RPATH to support both local and cibuildwheel wheel builds.
* Set LDFLAGS for to be written into the Makefiles generated by both
  the calls to ./autogen.sh and ./configure.
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 21, 2024

So one thing that I've noticed is that the build instructions aren't fully transparent as it seems that a local pip install doesn't do a good job of actually building and installing a wheel as we can see in this demo that _fastjet_core/lib/python3.11/site-packages/_fastjet.so.0 is linked against the shared libraries in the source directory and not installed versions under site-packages!

$ docker run --rm -ti python:3.11 /bin/bash
root@305165ec2faa:/# python -m venv venv && . venv/bin/activate
(venv) root@305165ec2faa:/# python -m pip --quiet install --upgrade pip setuptools wheel
(venv) root@305165ec2faa:/# apt-get update && apt-get install -y libboost-dev libmpfr-dev swig autoconf libtool
(venv) root@305165ec2faa:/# git clone --recursive https://github.com/scikit-hep/fastjet.git --branch fix/change-origin-par-expansion
(venv) root@305165ec2faa:/# cd fastjet/
(venv) root@305165ec2faa:/fastjet# python -m pip install --upgrade --verbose .
(venv) root@305165ec2faa:/fastjet# find /venv/lib/python3.11/site-packages/ -maxdepth 1 -iname "fastjet*"
/venv/lib/python3.11/site-packages/fastjet
/venv/lib/python3.11/site-packages/fastjet-3.4.2.0.dist-info
(venv) root@305165ec2faa:/fastjet# find /venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0"
/venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/_fastjet.so.0
(venv) root@305165ec2faa:/fastjet# ldd $(find /venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0")
	linux-vdso.so.1 (0x00007ffe17e7b000)
	libfastjet.so.0 => /fastjet/src/fastjet/_fastjet_core/lib/libfastjet.so.0 (0x00007f6e283a5000)
	libfastjettools.so.0 => /fastjet/src/fastjet/_fastjet_core/lib/libfastjettools.so.0 (0x00007f6e2835a000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f6e2813a000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f6e2805b000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f6e27e78000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f6e27e58000)
	libgmp.so.10 => /lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f6e27dd7000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f6e285eb000)
(venv) root@305165ec2faa:/fastjet# ls -l /fastjet/src/fastjet/_fastjet_core/lib/libfastjet.so.0
lrwxrwxrwx 1 root root 19 Feb 21 21:30 /fastjet/src/fastjet/_fastjet_core/lib/libfastjet.so.0 -> libfastjet.so.0.0.0
(venv) root@305165ec2faa:/fastjet# ls -l /fastjet/src/fastjet/_fastjet_core/lib/libfastjet.so.0.0.0  # this is the wrong file to link against 
-rwxr-xr-x 1 root root 16193200 Feb 21 21:30 /fastjet/src/fastjet/_fastjet_core/lib/libfastjet.so.0.0.0
(venv) root@305165ec2faa:/fastjet# find /venv/ -type f -iname "libfastjet.so.0"
/venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/libfastjet.so.0
(venv) root@305165ec2faa:/fastjet# ls -l /venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/libfastjet.so.0  # should be linking against this
-rwxr-xr-x 1 root root 16193200 Feb 21 21:31 /venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/libfastjet.so.0

Though if we download a wheel from this PR which was built with cibuildwheel (https://github.com/scikit-hep/fastjet/actions/runs/7995766598/artifacts/1264455642) (here I'm going to copy it into the Docker container as resolving the full URL in advance is a bit annoying) we see that cibuildwheel is doing additional things that change the package layout as well (e.g. site-packages/fastjet.libs/ and linking to shared libraries in it):

(venv) root@305165ec2faa:/fastjet# deactivate 
root@305165ec2faa:/fastjet# python -m venv /cibuildwheel-venv && . /cibuildwheel-venv/bin/activate
(cibuildwheel-venv) root@305165ec2faa:/fastjet# cd /tmp/
(cibuildwheel-venv) root@305165ec2faa:/tmp# unzip cibw-wheels-ubuntu-latest-311-auto64.zip
Archive:  cibw-wheels-ubuntu-latest-311-auto64.zip
  inflating: fastjet-3.4.2.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
(cibuildwheel-venv) root@305165ec2faa:/tmp# python -m pip --no-cache-dir install --upgrade ./fastjet-3.4.2.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
(cibuildwheel-venv) root@305165ec2faa:/tmp# find /cibuildwheel-venv/lib/python3.11/site-packages/ -maxdepth 1 -iname "fastjet*"  # we now have fastjet.libs too
/cibuildwheel-venv/lib/python3.11/site-packages/fastjet
/cibuildwheel-venv/lib/python3.11/site-packages/fastjet-3.4.2.0.dist-info
/cibuildwheel-venv/lib/python3.11/site-packages/fastjet.libs
(cibuildwheel-venv) root@305165ec2faa:/tmp# find /cibuildwheel-venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0"
/cibuildwheel-venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/_fastjet.so.0
(cibuildwheel-venv) root@305165ec2faa:/tmp# ldd $(find /cibuildwheel-venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0")
	linux-vdso.so.1 (0x00007fffe296a000)
	libfastjettools-ec77b8d8.so.0.0.0 => /cibuildwheel-venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/../../../../../fastjet.libs/libfastjettools-ec77b8d8.so.0.0.0 (0x00007fd49aab9000)
	libfastjet-8223762e.so.0.0.0 => /cibuildwheel-venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/../../../../../fastjet.libs/libfastjet-8223762e.so.0.0.0 (0x00007fd49a981000)
	libgmp-afec2dd4.so.10.2.0 => /cibuildwheel-venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/../../../../../fastjet.libs/libgmp-afec2dd4.so.10.2.0 (0x00007fd49a600000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fd49a3e6000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fd49a89a000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fd49a205000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fd49a1e5000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fd49accb000)
(cibuildwheel-venv) root@305165ec2faa:/tmp#

This matters especially for conda-forge/staged-recipes#21052 as there things are built with python -m pip install . -vv and no cibuildwheel. So getting the linking correct is imporant.

@chrispap95 @lgray @jpivarski can you comment on this and where to go?

edit: Note that while the file structure is different than the cibuildwheel wheel, if you first use build to build a wheel and then install from it the linking is at least correct:

$ docker run --rm -ti python:3.11 /bin/bash
root@88ae83b3563a:/# python -m venv venv && . venv/bin/activate
(venv) root@88ae83b3563a:/# python -m pip --quiet install --upgrade pip setuptools wheel
(venv) root@88ae83b3563a:/# apt-get update && apt-get install -y libboost-dev libmpfr-dev swig autoconf libtool
(venv) root@88ae83b3563a:/# git clone --recursive https://github.com/scikit-hep/fastjet.git --branch fix/change-origin-par-expansion
(venv) root@305165ec2faa:/# cd fastjet/
(venv) root@88ae83b3563a:/fastjet# python -m pip install build
(venv) root@88ae83b3563a:/fastjet# python -m build .
(venv) root@88ae83b3563a:/fastjet# python -m pip install --upgrade ./dist/fastjet-3.4.2.0-*.whl
(venv) root@88ae83b3563a:/fastjet# find /venv/lib/python3.11/site-packages/ -maxdepth 1 -iname "fastjet*"
/venv/lib/python3.11/site-packages/fastjet
/venv/lib/python3.11/site-packages/fastjet-3.4.2.0.dist-info
(venv) root@88ae83b3563a:/fastjet# find /venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0"
/venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/_fastjet.so.0
(venv) root@88ae83b3563a:/fastjet# ldd $(find /venv/lib/python3.11/site-packages/ -type f -iname "_fastjet.so.0")
	linux-vdso.so.1 (0x00007ffe9c51a000)
	libfastjet.so.0 => /venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/../../libfastjet.so.0 (0x00007fbe62379000)
	libfastjettools.so.0 => /venv/lib/python3.11/site-packages/fastjet/_fastjet_core/lib/python3.11/site-packages/../../libfastjettools.so.0 (0x00007fbe6232e000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fbe6210e000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fbe6202f000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fbe61e4c000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fbe61e2c000)
	libgmp.so.10 => /lib/x86_64-linux-gnu/libgmp.so.10 (0x00007fbe61dab000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fbe625bf000)
(venv) root@88ae83b3563a:/fastjet# 

@matthewfeickert matthewfeickert changed the title fix: Use single '$' for ORIGIN parameter expansion fix: Update RPATH and fully guard ORIGIN parameter expansion Feb 21, 2024
@lgray lgray marked this pull request as ready for review February 22, 2024 21:50
@lgray lgray merged commit 0791aa9 into main Feb 22, 2024
13 checks passed
@lgray lgray deleted the fix/change-origin-par-expansion branch February 22, 2024 21:50
@matthewfeickert
Copy link
Member Author

Oh, I was going to rebase this to clean up the commit messages before this got merged (that's why I had this in draft). For future reference, the commit log of 0791aa9 is WRONG and what is in this PR body is what was actually done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants