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

Added 32-bit support. #361

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Added 32-bit support. #361

merged 1 commit into from
Aug 1, 2023

Conversation

Esption
Copy link

@Esption Esption commented Jul 28, 2023

For issue #347

Main two things are causing it to pull and output both the 32 and 64 bit versions of float/double in the codegen. Unfortunately can't check target_pointer_width in codegen (it only sees your host width, not target) so that check has to be after codegen.

Also, need to change the align on Opaque for pointer casting. AFAIK this should be complete besides tests.

@Bromeon Bromeon added feature Adds functionality to the library c: tooling CI, automation, tools c: engine Godot classes (nodes, resources, ...) labels Jul 29, 2023
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-361

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Unfortunately can't check target_pointer_width in codegen (it only sees your host width, not target) so that check has to be after codegen.

Do you know why that is? godot-codegen is a normal Rust crate... the only special thing is that it's a build-dependency, not a regular one.

Also, which rustc target did you use? I wonder how we can best try to reproduce it in CI... 🤔

@@ -11,7 +11,8 @@
///
/// Note: due to `align(8)` and not `packed` repr, this type may be bigger than `N` bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment would need to be updated:

Suggested change
/// Note: due to `align(8)` and not `packed` repr, this type may be bigger than `N` bytes
/// Note: due to `align(4)`/ `align(8)` and not `packed` repr, this type may be bigger than `N` bytes

@Esption
Copy link
Author

Esption commented Jul 29, 2023

rust-lang/rust#42587

It's just a byproduct of how it works. It's unfortunate but the only other way is adding a feature cfg but that would feel clunky on the user side.

Comment on lines +248 to +259
let build_config: [&'static str; 2] = {
if cfg!(feature = "double-precision") {
["double_32", "double_64"]
} else {
["float_32", "float_64"]
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explanation!

Could you maybe add that link with a short explanation above this snippet? That way, the design decision is documented and people don't wonder why 32/64 is not already determined at this point (like me) 🙂

@Bromeon
Copy link
Member

Bromeon commented Jul 29, 2023

rustfmt would need to be fixed as well, otherwise looks good! Please amend your existing commit if possible.

I still need to think about how to run 32-bit config in CI. What's the name of your target (the triple format)?

@Esption
Copy link
Author

Esption commented Jul 30, 2023

Forgot to do the cargo fmt, so yeah oops. Also, I don't see any clippy things from anything I touched too tho there seem to be some floating around.

As for targets I'm just on windows atm so all I can really test is native windows or if it compiles in WSL. So, i686-pc-windows-msvc and i686-unknown-linux-gnu. It looks like the CI uses Ubuntu, which I don't believe installs 32-bit headers by default. Likely you'd need to install gcc-multilib and possibly g++-multilib.

@Bromeon
Copy link
Member

Bromeon commented Jul 30, 2023

Thanks! Small request:

Please amend your existing commit if possible.

Could you squash your commits into one?

@Bromeon
Copy link
Member

Bromeon commented Aug 1, 2023

Thank you! 🚀

@Bromeon Bromeon enabled auto-merge August 1, 2023 05:58
@Bromeon Bromeon added this pull request to the merge queue Aug 1, 2023
Merged via the queue into godot-rust:master with commit 157afcf Aug 1, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) c: tooling CI, automation, tools feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants