Fix cross build from macos to msvc #1361
Conversation
|
Thanks for the contribution! Please add a PR description, containing not just what it fixes but also how it does so, and elaborating design choices and trade-offs you made on the way. The diff contains several changes that seem unrelated to the original stated problem, and I'm not sure they are necessary. Why is there logic to locate Why is version 4.5 hardcoded? Same regarding |
|
Got it, I will try to find a better approach, the root cause is the #[cfg(windows)] and #[cfg(target_os = "macos")] in load_gdextension_header_rs() are evaluated at compile time for the HOST platform (since it's a build dependency), not the TARGET platform. It also affect android x32 platform. |
There was a problem hiding this comment.
Thanks for the update!
I made some remarks. In many cases there is vague terminology and I'm not sure the comments accurately reflect what's happening. Things like "bindings file" aren't obvious in our context -- there is the C header, there is the generated header for Rust (through bindgen), there are the JSON files...
Please also note our Contribution Policy on AI. Use of AI tools is OK, but you as PR author are responsible for its content, meaning you need to fully understand what's happening and the comments need to be precise.
I also asked for this earlier, could you edit your initial message? There's probably some overlap with the issue description, but in the future it will help to immediately recognize how things are implemented 🙂
Please add a PR description, containing not just what it fixes but also how it does so, and elaborating design choices and trade-offs you made on the way.
godot-bindings/src/import.rs
Outdated
| let target_family = std::env::var("CARGO_CFG_TARGET_FAMILY").ok(); | ||
| let target_os = std::env::var("CARGO_CFG_TARGET_OS").ok(); | ||
|
|
||
| // Select platform suffix matching gdextension-api's file naming |
There was a problem hiding this comment.
What does this mean? Which file naming?
godot-bindings/src/import.rs
Outdated
| /// Since godot-bindings is a build-dependency, it's compiled for HOST, but we need bindings for TARGET. | ||
| /// We detect the target platform using CARGO_CFG_TARGET_* environment variables available during | ||
| /// build script execution, then read the corresponding file from the gdextension-api dependency. |
There was a problem hiding this comment.
Also here, what "corresponding file"?
godot-bindings/src/import.rs
Outdated
| // Get the Godot version string from prebuilt module | ||
| let version = super::prebuilt::GODOT_VERSION_STRING; |
There was a problem hiding this comment.
You don't need to comment trivial variable assignments.
godot-bindings/src/import.rs
Outdated
| // The 4.x branches have files directly in res/, not versions/{version}/res/ | ||
| let file_path = PathBuf::from(dep_root) | ||
| .join("res") | ||
| .join(format!("gdextension_interface_{}.rs", platform)); |
There was a problem hiding this comment.
I don't think that comment is true, see https://github.com/godot-rust/godot4-prebuilt/tree/release-v0.3 directory structure.
What does the comment mean?
godot-bindings/src/import.rs
Outdated
| Err(e) => panic!( | ||
| "Failed to load platform-specific bindings for platform '{}'.\n\ | ||
| Tried to read: {}\n\ | ||
| Error: {}\n\ | ||
| \n\ | ||
| This is likely a cross-compilation issue or the gdextension-api version doesn't support this platform.", | ||
| platform, file_path.display(), e | ||
| ), |
There was a problem hiding this comment.
Please use inline string interpolation where possible (for platform + e)
godot-bindings/src/lib.rs
Outdated
| // CROSS-COMPILATION FIX: | ||
| // Since godot-bindings is a build-dependency, it and gdextension-api are compiled for the HOST platform. | ||
| // The #[cfg] attributes in gdextension-api::load_gdextension_header_rs() evaluate for HOST, not TARGET. | ||
| // We read CARGO_CFG_TARGET_* environment variables to select the correct bindings at runtime. |
There was a problem hiding this comment.
| // CROSS-COMPILATION FIX: | |
| // Since godot-bindings is a build-dependency, it and gdextension-api are compiled for the HOST platform. | |
| // The #[cfg] attributes in gdextension-api::load_gdextension_header_rs() evaluate for HOST, not TARGET. | |
| // We read CARGO_CFG_TARGET_* environment variables to select the correct bindings at runtime. | |
| // Cross-compilation support: | |
| // Since godot-bindings is a build-dependency, it and the gdextension-api crate are compiled for the HOST platform. | |
| // The #[cfg] attributes in gdextension_api::load_gdextension_header_rs() evaluate for HOST, not TARGET. | |
| // We read CARGO_CFG_TARGET_* environment variables to select the correct bindings at runtime. |
|
Thanks for your detailed review feedback, I've updated the code and comment accordantly. |
Our policy explicitly mentions "Do not generate the PR description with AI." If you fully understand your code, I don't think it's too much to ask for a summary of its inner workings in your own words. I'd rather have less text but written by a human, it doesn't need to be excessively long. Because I can just feed the diff into an AI myself otherwise -- the interesting parts are why you chose to do things in a certain way, and other thoughts you had on the way. |
I wrote the PR description in Chinese, and use AI to translate to proper English, that's why you sense it is written by AI, this time I request AI to not re-organize my writing, do direct translate, hope this time you think it's qualified to the PR standard. |
2 participants
Cross-compiling gdext from macOS to Windows was blowing up because build scripts were pulling macOS bindings at build time. #[cfg(target_os = ...)] runs for the host, not the target, so macOS pthread types slipped into Windows builds and caused size/overflow errors.
The fix is simple: during the build script, detect the target at runtime and load the right prebuilt bindings file for that target. We read Cargo’s CARGO_CFG_TARGET_* env vars, map them to a platform name (windows → windows, macos/ios → macos, everything else → linux), and then load res/gdextension_interface_{platform}.rs directly from gdextension-api using DEP_GDEXTENSION_API_ROOT. That logic lives in prebuilt_platform::load_gdextension_header_rs_for_target(), and write_gdextension_headers() now uses it so gdextension_interface.rs always matches the target.
Why this route? It keeps platform detection in one place, uses Cargo’s standard DEP_* env vars, doesn’t force API changes in gdextension-api, and adds basically no overhead (it reads one file). The trade-off is we need a small companion change in godot4-prebuilt (godot-rust/godot4-prebuilt#1) to expose the crate root, and there’s a bit of file I/O—nothing unusual for builds.
Alternatives we skipped: adding per-platform loader functions to gdextension-api (too invasive, more to maintain) and creating a special build-time variant of godot-bindings (duplication and dependency bloat).
What you can expect: native builds still work, macOS → Windows/MSVC cross-compiles now pass, and custom API modes (api-custom, api-custom-json) are unaffected. This only touches the default “prebuilt” mode. For any non-Windows and non-macOS/iOS target, we default to “linux.” If a platform file is missing, the error message tells you exactly what went wrong.