Skip to content

feat: Integrate wheel repairer #1009

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Mar 4, 2025

The goal of this is:

  • Get the linked libraries from the configure step
  • Check if the paths start with the current build environment's sysconfig.get_path("platlib")
    (pip fake venv is really annoying here)
  • If so patch the RPATH to be relative paths
  • Patch existing RPATHs of the current wheel files, e.g. from CMAKE_INSTALL_RPATH_USE_LINK_PATH
  • For windows systems, add dlls to Scripts path.
    Maybe would be better to use the existing paths instead.
  • Do some other magic that delvewheel does
    Afaict, the only special thing it does is inject os.add_dll_directory. Cannot support subprocess.run right now, but we could do it if we generate the wrappers. Will make a subsequent PR for that.

To discuss:

  • Should this be separated as a standalone plugin and instead expose an entry-point scikit-build.repair-wheel?
  • Initially I was considering using auditwheel/delocate/delvewheel directly, but these do not handle differently the packages in site-package vs external
  • Do we extend the interface to pick up system packages as well? I was thinking that the repairwheel trio can do all the hard work generally, but maybe the user needs more nuanced configuration, e.g. with Qt/PySide the requirement is to bundle the Qt framework uniquely, compared to dlevewheel which creates separate dlls for each wheel.

Depends-on: #1022 #1016

@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch 4 times, most recently from 45d04fd to 04d9881 Compare March 10, 2025 12:05
@LecrisUT

This comment was marked as resolved.

@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch 25 times, most recently from 81f9407 to 9cc8ea9 Compare March 17, 2025 14:13
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Mar 18, 2025

Ready for review for this initial proposal. Further proposals that I want to build up from this

  • Expand the wheel.repair to accept more complex repairs. Done this now
  • Make the repair-wheel plugin-able, allowing to specify either a script (PEP723 looks really nice for this) or a entry-point, to allow more fine-grained repairs
  • Add an option to bundle a glob list of libraries. Done this now

@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch 3 times, most recently from eb9a769 to a1506bd Compare March 26, 2025 10:51
@LecrisUT
Copy link
Collaborator Author

I have dropped the documentation until #1022 is resolved. Implementation wise, this is the overall picture I had in mind for this feature (I couldn't figure how to get .dll files from the CMake build so I can't do the bundling on Windows). I will keep it in this state for review.

@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch 2 times, most recently from 5bcd1d3 to b9b5450 Compare March 26, 2025 11:50
@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch from b9b5450 to da7c156 Compare April 1, 2025 10:27
henryiii added a commit that referenced this pull request Apr 15, 2025
I split the general documentation from #1009

---------

Signed-off-by: Cristian Le <git@lecris.dev>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
LecrisUT added 17 commits April 15, 2025 13:32
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
`auditwheel` pacher forces `DT_RPATH`. Here instead we use `DT_RUNPATH` so that it can be overwritten by the user

Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
This is needed because the last delocate package that supports python 3.8 does not have _get_rpaths

Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT LecrisUT force-pushed the feat/repair-wheel branch from da7c156 to 36e8c26 Compare April 15, 2025 14:04
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.

None yet

1 participant