internal: [codex] try fix mingw libname#6164
Conversation
usually mingw version of python are shipped with `lib` prefix, so we need to add it for proper linking
|
@ognevny can you try in this Git branch? |
chirizxc
changed the title
|
IIUC you just let the agent fix the things? I'll try to cherry-pick some good changes if I find them |
yes |
|
this time ai was smarter than me. I just forgot about dot separating major and minor version! that could be an issue... |
|
your branch didn't work too... https://github.com/msys2/MINGW-packages/actions/runs/28259458928/job/83730859317?pr=29942 |
|
maybe this? |
|
сodex went a bit overboard, but I hope it will help clarify the problem itself |
|
it worked! but the latest change seem to be a workaround... anyway I'll give up with my branch |
You can add fix to your branch; the main issue is that we need to test these builds in CI |
I closed my PR. anyway my was different only in a kind of style, so I'll let maintainers review your change |
|
oh I didn't see you wrote comments on the same lines 😅 |
|
@ognevny сould you take another look? I'm not sure about some of comments, since I haven't worked with MINGW |
|
fine with the comments, but you seem to duplicate them in one place |
|
I saw this PR, If anyone would like to help add MSYS2 to CI, please submit a PR to branch linked in this PR |
| // GNU-family Windows targets still need an import library for any non-Rust | ||
| // objects participating in the final link, such as CFFI-generated C code. | ||
| println!("cargo:rustc-link-lib={import_lib_name}"); |
There was a problem hiding this comment.
Is there some test / CI evidence to this?
There was a problem hiding this comment.
I asked Codex to create a test in the test repository that checks whether this branch of code is present or not; it seems that MINGW32 doesn't work without it
There was a problem hiding this comment.
There was a problem hiding this comment.
So far, I've only found this:
- Problems with
raw-dyliboni686-pc-windows-gnu: Wrong symbols are referenced for raw-dylib on i686-pc-windows-gnu rust-lang/rust#138963
There was a problem hiding this comment.
wait, but it didn't work for any arch, not only i686
oh I see that's different kind of workaround
Merging this PR will degrade performance by 12.61%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
| Ensure that the target Python library directory is \ | ||
| in the rustc native library search path." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Maybe we could use target-lexicon as a build-dependency for pyo3-ffi as well; I think that would make the code a little more readable:
fn emit_link_config(build_config: &BuildConfig) -> Result<()> {
let interpreter_config = &build_config.interpreter_config;
let target = target_triple_from_env();
let lib_name = interpreter_config
.lib_name()
.ok_or("attempted to link to Python shared library but config does not contain lib_name")?;
if target.operating_system == target_lexicon::OperatingSystem::Windows {
// Use raw-dylib linking: emit a cfg so that `extern_libpython!` picks the
// right `#[link(name = "...", kind = "raw-dylib")]` attribute at compile time.
// This eliminates the need for import libraries (.lib files) entirely.
//
// Note: raw-dylib is inherently dynamic linking. Static embedding of the
// Python interpreter on Windows is not supported by this path (and is not
// officially supported by CPython on Windows).
println!("cargo:rustc-cfg=pyo3_dll=\"{lib_name}\"");
let is_i686_pc_windows_gnu = target.environment == target_lexicon::Environment::Gnu
&& matches!(target.architecture, target_lexicon::Architecture::X86_32(_));
3 participants
No description provided.