Skip to content

Wasm registration fn names now based on crate name + index #1205

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

0x53A
Copy link
Contributor

@0x53A 0x53A commented Jun 15, 2025

fixes #1202

@GodotRust
Copy link

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

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, great simplification! 👍

Is the libc dependency still necessary after this?

@Bromeon Bromeon added c: wasm WebAssembly export target bug labels Jun 15, 2025
@Bromeon Bromeon changed the title [macros] generate '__godot_rust_registrant_' name based on crate name and index Wasm registration fn names now based on crate name + index Jun 15, 2025
@0x53A 0x53A force-pushed the dev-fix-godot_rust_registrant branch from d1e47eb to e5ac8cc Compare June 15, 2025 21:16
@PgBiel
Copy link
Contributor

PgBiel commented Jun 15, 2025

In principle this should be fine, though we may want to go back to the hash idea if we hit some name length limit. Let's wait and see...

@0x53A
Copy link
Contributor Author

0x53A commented Jun 15, 2025

image

It works in my simple case


if we hit some name length limit

I don't think there is a limit for identifier names, and if there is, it should be large enough

@0x53A 0x53A force-pushed the dev-fix-godot_rust_registrant branch from 3afea1b to 9a4d537 Compare June 15, 2025 22:07
@PgBiel
Copy link
Contributor

PgBiel commented Jun 15, 2025

Yeah, it's just that emscripten is now a bit picky about identifiers: #438 (comment)

But it's causing Rust to not work altogether, so this is probably not a concern for us, but for them. :p

@0x53A
Copy link
Contributor Author

0x53A commented Jun 17, 2025

This is ready from my side

@PgBiel
Copy link
Contributor

PgBiel commented Jun 18, 2025

On my limited testing on hello-gdext-wasm, seems to work fine too.

I'll just note that there are some concerns noted here: https://users.rust-lang.org/t/simple-state-in-procedural-macro/68204/2

In particular, proc macro caching and other details could potentially lead to problems.
But, I suppose, the previous approach wasn't any better than the current one, if the seed was stored in the process memory anyway... So I'd vote to ship this as an incremental improvement and consider changing further in the future should problems arise.

That thread also suggested using UUIDs, but I'm fairly sure they are generated using the current time?
Though I suppose that's not a problem for reproducible builds since they can control the current time in the sandbox... So we could eventually consider something like that, but let's go one step at a time.

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.

I'll just note that there are some concerns noted here: https://users.rust-lang.org/t/simple-state-in-procedural-macro/68204/2

Very good point 👍

@0x53A looks good for now. Could you apply my suggestion on the static comment (verbatim), so we're not oblivious to this issue, should it cause trouble at some point?

add comment

Co-authored-by: Jan Haller <[email protected]>
@0x53A
Copy link
Contributor Author

0x53A commented Jun 18, 2025

if it comes to that we can pass through an identifier so it would become "__godot_rust_registrant__{crate_name}__v{crate_version}__{user_type_name}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: wasm WebAssembly export target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rand is not seeded, causing duplicated symbol names if two crates use gdext
4 participants