-
Notifications
You must be signed in to change notification settings - Fork 59
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 --exclude option to delocate-wheel #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Some thoughts. Can you think of a test?
delocate/cmd/delocate_wheel.py
Outdated
def copy_filt(name): | ||
if not exclude: | ||
return filter_system_libs(name) | ||
return filter_system_libs(name) and not _is_in_list(name, exclude) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are doing a substring search here? Is that what you intended? Or did you mean
and not name in exclude
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The substring search by rossant is intentional. The main alternative is to allow RegEx, which is usually overkill.
delocate/cmd/delocate_wheel.py
Outdated
@@ -37,6 +45,8 @@ def main(): | |||
action="store_true", | |||
help="Only analyze files with known dynamic library " | |||
"extensions"), | |||
Option("-e", "--exclude", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or allow multiple --exclude
options with action="append"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten the code to use action="append"
but I haven't tested it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 94.93% 95.11% +0.18%
==========================================
Files 14 15 +1
Lines 1125 1167 +42
==========================================
+ Hits 1068 1110 +42
Misses 57 57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good - but it still needs some tests - maybe in delocate/tests/test_scripts.py
?
Maybe. I wish the script running tests were using a standard tool rather than |
Yes, I wrote my own script testing framework around 2016, before there were others available - or at least - before I knew there were others available. |
Any progress? |
Won't merge until tests are made which is a reasonable request. The new tests can use |
Can we merge this please ? |
I took a few minutes to write something that should do ok as a test : @pytest.mark.xfail(sys.platform != "darwin", reason="Needs macOS linkage.")
def test_fix_wheel_with_excluded_dylibs():
with InTemporaryDirectory() as tmpdir:
fixed_wheel, stray_lib = _fixed_wheel(tmpdir)
_rename_module(fixed_wheel, "module.other", "test.whl")
shutil.copyfile("test.whl", "test2.whl")
# We exclude the stray library so it shouldn't be present in the wheel
code, stdout, stderr = run_command(["delocate-wheel", "-e", "extfunc", "test.whl"])
with InWheel("test.whl"):
dylibs = pjoin("plat_pkg", "fakepkg1", ".dylibs")
assert_equal(os.listdir(dylibs), [])
# We exclude a library that does not exist so we should have a normal behavior
code, stdout, stderr = run_command(["delocate-wheel", "-e", "doesnotexist", "test2.whl"])
_check_wheel("test2.whl", ".dylibs") |
Co-authored-by: rossant <[email protected]>
Co-authored-by: Philip Garnero <[email protected]>
I've added the test and cleaned up the commits. It could be better but now I think it's good enough to merge. |
Adds new exlucde argument to delocate-path Makes verbosity work the same among all commands
Moved some CLI logic to a shared module so that any new code can apply to both |
Hey @HexDecimal, I think the changes in this PR may have resulted in breaking the delocate CLI extending a glob-style paths arg: #71 (comment). |
Same motivation as pypa/auditwheel#310
I need this to remove libvulkan from the repaired wheel.