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

Add $ORIGIN/_fastjet_core/lib to RPATH for PyPI wheels. #65

Merged
merged 29 commits into from
Aug 18, 2021

Conversation

jpivarski
Copy link
Member

@jpivarski jpivarski commented Aug 16, 2021

Summary of this PR:

  • All ldd "Not Found" errors were resolved by linking with -rpath=$ORIGIN/_fastjet_core/lib, with sufficient quoting on the "$ORIGIN". 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".
  • Dropped PxCone because libgfortran is not found. According to @lgray, "PxCone is from 2006 and was most recently cited in a Herwig benchmarking paper for completeness in 2016." It's probably safe to ignore PxCone.
  • Intermediate commits were attempts to get it to work on MacOS. Last attempt in ci.yml, and last attempt in wheels.yml, which are both from bb4afab.
  • After all that, I tried making release v3.3.4.0rc7, but this failed because we weren't actually running auditwheel.
  • Restored the cibuildwheel infrastructure to its original state, with pypa/cibuildwheel@v2.0.1, following @henryiii's suggestion.
  • FastJet's ./configure, when it runs inside cibuildwheel, puts the SWIG-generated Python files in a different location. The only way I see to get it is to extract it directly from the pyinterface/Makefile. (A banner asking users to set PYTHONPATH tells us which variables hold the directory locations.)
  • At the time of writing (just after 1065e43), wheels have been uploaded to PyPI and successfully tested with pip install --pre fastjet!
  • However, the shared libraries have identical files named libxyz.so, libxyz.so.0, and libxyz.so.0.0.0. Presumably, they were originally symbolic links, but now they're duplicates. Removing the two excess copies would reduce the total wheel size by 1/3. (Uncompressed, the extra two copies add up to 52 MB out of 156 MB.) Perhaps the .0 and .0.0.0 versions can be dropped in MANIFEST.in?

@jpivarski jpivarski changed the title Add /_fastjet_core/lib to RPATH for PyPI wheels. Add $ORIGIN/_fastjet_core/lib to RPATH for PyPI wheels. Aug 16, 2021
jpivarski and others added 2 commits August 16, 2021 19:13
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
@jpivarski
Copy link
Member Author

For this

Binary wheel 'fastjet-3.3.4.0rc7-cp36-cp36m-linux_x86_64.whl' has an unsupported platform tag 'linux_x86_64'.

upon PyPI upload: it seems that we're missing an auditwheel repair step. It looks like wheels.yml never had one?

auditwheel repair --plat manylinux2010_x86_64 fastjet-3.3.4.0rc7-cp38-cp38-linux_x86_64.whl
INFO:auditwheel.main_repair:Repairing fastjet-3.3.4.0rc7-cp38-cp38-linux_x86_64.whl
usage: auditwheel [-h] [-V] [-v] command ...
auditwheel: error: cannot repair "fastjet-3.3.4.0rc7-cp38-cp38-linux_x86_64.whl" to "manylinux2010_x86_64" ABI because of the presence of too-recent versioned symbols. You'll need to compile the wheel on an older toolchain.

So the whole build process had to run in the quay.io/pypa/manylinux2010_x86_64 Docker image, apparently? I thought cibuildwheel would have done this.

@jpivarski
Copy link
Member Author

Is it a problem that the cibuildwheel configuration is overriding the default manylinux images?

fastjet/pyproject.toml

Lines 14 to 15 in ea2b058

manylinux-x86_64-image = "manylinux_2_24"
manylinux-i686-image = "manylinux_2_24"

I'm going to try removing those lines.

@jpivarski
Copy link
Member Author

This is what happened to the repair command: 558bbf2.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I'd recommend checking for any duplication and for unhidden symbols - these wheels are 40MB each, which might be expected, but also might be due to something fixable.

@jpivarski
Copy link
Member Author

I'd recommend checking for any duplication and for unhidden symbols - these wheels are 40MB each, which might be expected, but also might be due to something fixable.

I was thinking about doing something to fix the libfastjet.so, libfastjet.so.0, libfastjet.so.0.0.0 duplication (same for all the other libraries), which would reduce 40 MB → 25 MB, before merging.

Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
@jpivarski
Copy link
Member Author

jpivarski commented Aug 17, 2021

Formatting (by hand).

Edit: that was supposed to be the commit message, but I pressed "comment" instead of "commit." First time trying out github.dev.

setup.py Outdated Show resolved Hide resolved
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
@aryan26roy aryan26roy self-requested a review August 18, 2021 11:44
@jpivarski
Copy link
Member Author

I'd recommend checking for any duplication and for unhidden symbols - these wheels are 40MB each, which might be expected, but also might be due to something fixable.

I was thinking about doing something to fix the libfastjet.so, libfastjet.so.0, libfastjet.so.0.0.0 duplication (same for all the other libraries), which would reduce 40 MB → 25 MB, before merging.

I (or one of us) will remove those duplicates some other time. I'll merge this PR now.

@jpivarski jpivarski merged commit 4fb2d3d into main Aug 18, 2021
@jpivarski jpivarski deleted the jpivarski/fix-rpath-for-pypi branch August 18, 2021 12:37
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