-
Notifications
You must be signed in to change notification settings - Fork 293
Make the {project} placeholder available to repair-wheel-command
#2589
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
Make the {project} placeholder available to repair-wheel-command
#2589
Conversation
agriyakhetarpal
left a comment
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.
Thanks, @arcondello! This looks good to me. Could you also mark #1931 to be closed with this?
|
Oh wow, totally missed that one. Updated |
|
I'm a little surprised we don't run the command from the project directory on Linux. |
This is what's confusing me also... I'm 90% sure that we do...! But I think this can still be useful so I'm happy to merge it. On testing, it appears that the previous test did actually assert that the cwd was the project directory previously. Could we make sure that we don't lose that too? It might be best to revert the test change and add another test, perhaps on just a single python version, that runs the script with the |
Sure, I can do that |
|
As an alternative, if the intent is always to run the |
Yes, that is the intention. I doubt that will change - the other commands like before-build etc. also run from the project dir (which is the cwd when cibuildwheel starts). Logic with relative paths gets pretty strange if we change this rule! Happy to see this noted in the docs. Also, I think it might be nice for consistency for |
Will do
Sure, I can do that. Likely will be able to update the PR tomorrow |
|
Ok updated, sorry for the delay. Though I fear my pattern matching for the tests is falling short and that something more sophisticated would be better. |
|
I dont think the readthedocs error is related to this PR? but if it is let me know |
7b82536 to
58e0a1d
Compare
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.
Pull Request Overview
This PR adds support for the {project} and {package} placeholders in the repair-wheel-command configuration option, allowing users to reference the project root and package directory in their custom repair wheel commands.
- Adds
projectandpackageparameters toprepare_commandcalls forrepair_commandacross all platforms - Updates documentation to mention the new placeholder support
- Adds a parameterized test to verify the placeholders work correctly
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cibuildwheel/platforms/windows.py | Adds package and project parameters to repair command preparation |
| cibuildwheel/platforms/macos.py | Adds package and project parameters to repair command preparation |
| cibuildwheel/platforms/linux.py | Adds package and project parameters to repair command preparation using container paths |
| cibuildwheel/platforms/pyodide.py | Adds package and project parameters to repair command preparation |
| cibuildwheel/platforms/android.py | Adds package and project parameters to repair command preparation |
| docs/options.md | Documents the availability of {package} and {project} placeholders for repair-wheel-command |
| test/test_custom_repair_wheel.py | Adds parameterized test to verify placeholder functionality in repair wheel commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #2588
Closes #1931
Let me know if you'd prefer a different approach for testing, the change I made amounts to a smoke test.