-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
fix abi3 interpreter discovery on Windows #2333
Conversation
Looks like this is working well. Without
... and the same if I enable the
|
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (2)
src/build_options.rs:413
- The capitalization of 'automatically' has been corrected.
eprintln!("🐍 Not using a specific python interpreter (automatically generating windows import library)");
src/build_context.rs:237
- The
to_string
method forinterp
does not include the major and minor versions, which could lead to incorrect output. Consider usingformat!("{}{}{}", interp, interp.major, interp.minor)
to ensure the correct format.
.map(|interp| interp.to_string())
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!
Tested in |
Great! |
But does setting just I'm not using this configuration myself since |
Oof, good spot, I think you might be right. I'll try to circle back here and cover with some tests at the same time. |
This is an attempt to fix #2325 (comment)
(Not tested yet, I wanted to push and see how this runs on CI as well as test locally.)