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 ability to pass wildcard arguments to --exclude #508

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

Conversation

KyleFromNVIDIA
Copy link

Fixes #500

@emmatyping-nv
Copy link

Could you add a test verifying that sequences work? e.g. libfoo.[1,2] matches libfoo.1/libfoo.2, but not libfoo.3?

@oraluben
Copy link

oraluben commented Nov 25, 2024

Hi @mayeut, I’d like to draw your attention to this PR for it can support wildcard exclude, which can support to exclude specific libs without knowing the detailed filename (e.g. to exclude /usr/local/cuda*, intentionally). This feature can reduce a large amount of my work, please let me know if you think it's a good idea to merge this so I can try to find some other way.

Thanks!

@nickjbrowning
Copy link

I also have a similar issue, where for example I need to exclude: libcudart-8774224f.so and libnvToolsExt-847d78f2.so from my wheels. Wildcards would certainly help here for building wheels across different docker containers.

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.04%. Comparing base (9394d48) to head (67ef499).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
- Coverage   92.35%   92.04%   -0.31%     
==========================================
  Files          20       20              
  Lines        1268     1270       +2     
  Branches      244      244              
==========================================
- Hits         1171     1169       -2     
- Misses         56       57       +1     
- Partials       41       44       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KyleFromNVIDIA
Copy link
Author

KyleFromNVIDIA commented Dec 9, 2024

Could you add a test verifying that sequences work? e.g. libfoo.[1,2] matches libfoo.1/libfoo.2, but not libfoo.3?

@emmatyping-nv I had previously abandoned this PR because #411 had given me the impression that the PyPA team didn't want this change, but since there now appears to be activity on it from the maintainers, I went ahead and added this test.

@mayeut
Copy link
Member

mayeut commented Dec 14, 2024

I had previously abandoned this PR because #411 had given me the impression that the PyPA team didn't want this change, but since there now appears to be activity on it from the maintainers, I went ahead and added this test.

I won't have time to re-review this properly before the end of the year, maybe @lkollar will ?
Given the high demand for this feature, we should find a middle-ground to allow wildcard while emphasizing that it should be used with caution (c.f. the comment in #411 where the feature would allow not to reflect dependencies properly in metadata).
One way would be to have a toggle to enable fnmatch with its help mentioning that proper metadata is the packager responsibility, possibly redirecting to #411.

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.

Allow wildcards in --exclude
5 participants