-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[bindgen] Include Version in the export name if needed #7656
[bindgen] Include Version in the export name if needed #7656
Conversation
786d8d6
to
6494fa2
Compare
This has the side affect of creating crazy names like the following and is why CI is failing (local tests worked on generation project)
Since this is not super common (at least so far), if there is only one export for that pacakage/interface combination then don't include the version (same as it is today). For situations where there are two exports and the long name isn't desired we could have an override like: // can provide overrides
wasmtime::component::bindgen!({
exports: {
"test:dep/test@0.1.0": "test_dep_test",
"test:dep/test@0.2.0": "test_dep_test_02",
} |
319c983
to
62c143e
Compare
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! Could you clarify though? You mention that CI is failing but it looks passing here. This additionally implements what I would expect which is that versions are dropped unless they're required, so this looks good to go to me modulo a suggestion below to use a preexisting helper, but I wanted to confirm I wasn't missing anything.
Sorry for the confusion, you are not missing anything. The ci is passing now. I made that comment before I fixed it with the latest commit, which drops the version if there is only one.
Should this be applied across all bindgen projects? I recently made a change to the c# wit-bindgen to add the version to the namespaces bytecodealliance/wit-bindgen#781. It adds the version if present. |
Personally at least, yes, I think all bindings generators should drop versions unless there's an ambiguity. Helps promotes the use of versions I think without causing all the generated bindings to look terrible ideally. I haven't gotten around to implementing this much elsewhere other than the Wasmtime host and Rust guest generators though. |
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
62c143e
to
7ed8ef2
Compare
When exporting multiple versions of the same package the version name needs to be included.
Before this change the generator would produce:
now it will produce:
Found in https://github.com/bytecodealliance/wit-bindgen/pull/787/files#r1416724258
Discussed in https://bytecodealliance.zulipchat.com/#narrow/stream/327223-wit-bindgen/topic/Host.20Exports.20across.20versions