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

Jump to bottom

feature: allow specifying additional library paths (fixes: #373) #374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −0
Open

feature: allow specifying additional library paths (fixes: #373) #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions src/auditwheel/main_repair.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import argparse
import logging
import os
from os.path import abspath, basename, exists, isfile

from auditwheel.patcher import Patchelf
Expand Down Expand Up @@ -92,6 +93,12 @@ def configure_parser(sub_parsers):
help="Do not check for higher policy compatibility",
default=False,
)
p.add_argument(
"--add-path",
dest="PATHS",
default="",
help=f"Additional path(s) to search for libraries, {os.pathsep!r}-delimited",
)
Comment on lines +96 to +101
Copy link
Member

Choose a reason for hiding this comment

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

I understand that you wanted to use the same name as delvewheel which seems like a good thing however, it feels more like -L/--library-path or -rpath-link to mimic a bit more what feels like ld would be doing. I'd probably go with --library-path, allowed to be specified multiple times. Unfortunately, -L is already used for something else...

I'd be in favor of having the option specified multiple times with a single path each time rather than a single string option.

@lkollar, any thoughts on the name ? (Once it gets in, we won't be able to change it without breaking users).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that passing multiple paths is nicer with --library-path specified multiple times. However, overall I don't fully understand why this feature is necessary. What this PR does is functionally equivalent with running LD_LIBRARY_PATH=<path> audithweel repair. Doing this in cibuildwheel would need something like CIBW_REPAIR_WHEEL_COMMAND='LD_LIBRARY_PATH=<path> audithweel repair -w {dest_dir} {wheel}'. Maybe I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

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

@lkollar you're not missing anything, this is accurate, it can indeed be achieved by manipulating LD_LIBRARY_PATH. I hadn't realized that the "repair wheel command" was invoked in a shell, making it possible to alter environment variables. Now I'm not sure whether we actually want this option..

Copy link

Choose a reason for hiding this comment

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

@jlaine I guess too many people do not realize that LD_LIBRARY_PATH is a solution for ValueError: Cannot repair wheel, because required library "xxx.so" could not be located, and that's exactly the reason why this PR is needed.

Unfortunately, -L is already used for something else...

@mayeut and I can't say I didn't try it, because there was no other option.

--add-path is good for parity with delvewheel https://github.com/adang1345/delvewheel#additional-options

I would also add the pointer to it the error message, so that instead of plain ValueError there is a hint what to do next.

Copy link

Choose a reason for hiding this comment

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

Actually the proper fix is to propagate the option values to lddtree here. It already parses LD_LIBRARY_PATH, so no need to handle that.

auditwheel/src/auditwheel/wheel_abi.py

Line 83 in 55bc5ee

elftree = lddtree(fn)

Copy link

Choose a reason for hiding this comment

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

Appears it is not that easy. If ldpaths to lddtree() is supplied, then LD_LIBRARY_PATH won't be parsed. Don't know if it is a good thing or not. Also my knowledge is not enough to understand the dict params.

auditwheel/src/auditwheel/lddtree.py

Lines 315 to 318 in 55bc5ee

ldpaths
dict containing library paths to search; should have the keys:
conf, env, interp. If not supplied, the function ``load_ld_paths``
will be called.

p.set_defaults(func=execute)


Expand All @@ -106,6 +113,12 @@ def execute(args, p):

logger.info("Repairing %s", basename(args.WHEEL_FILE))

if args.PATHS:
library_path = args.PATHS
if "LD_LIBRARY_PATH" in os.environ:
library_path += os.pathsep + os.environ["LD_LIBRARY_PATH"]
os.environ["LD_LIBRARY_PATH"] = library_path

if not exists(args.WHEEL_DIR):
os.makedirs(args.WHEEL_DIR)

Expand Down
1 change: 1 addition & 0 deletions tests/integration/test_bundled_wheels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def test_wheel_source_date_epoch(tmp_path, monkeypatch):
args = Namespace(
LIB_SDIR=".libs",
ONLY_PLAT=False,
PATHS="",
PLAT="manylinux_2_5_x86_64",
STRIP=False,
UPDATE_TAGS=True,
Expand Down