Wasmtime GitHub notifications bot (Dec 08 2023 at 02:54): Wasmtime GitHub notifications bot (Dec 08 2023 at 02:54):jsturtevant opened PR #7656 from jsturtevant:support-versioned-exports to bytecodealliance:main:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->When exporting multiple versions of the same package the version name needs to be included.
Before this change the generator would produce:
pub fn test_dep_test(&self) -> &exports::test::dep0_1_0::test::Test { &self.interface1 } pub fn test_dep_test(&self) -> &exports::test::dep0_2_0::test::Test { &self.interface2 }now it will produce:
pub fn test_dep_0_1_0_test(&self) -> &exports::test::dep0_1_0::test::Test { &self.interface1 } pub fn test_dep_0_2_0_test(&self) -> &exports::test::dep0_2_0::test::Test { &self.interface2 }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
Wasmtime GitHub notifications bot (Dec 08 2023 at 02:54):jsturtevant requested pchickey for a review on PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 02:54):jsturtevant requested wasmtime-core-reviewers for a review on PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 02:56):jsturtevant updated PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 04:59):jsturtevant updated PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 05:01):jsturtevant commented on PR #7656:
This has the side affect of creating crazy names like the following and is why CI is failing (local tests worked on generation project)
proxy .wasi_http_0_2_0_rc_2023_12_05_incoming_handler() .call_handle(store, req, out)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", }
Wasmtime GitHub notifications bot (Dec 08 2023 at 05:01):jsturtevant updated PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 05:06):jsturtevant submitted PR review.
Wasmtime GitHub notifications bot (Dec 08 2023 at 05:06):jsturtevant created PR review comment:
I don't think this is exactly right. Was having a hard time mapping interfaces to packages in an effective way but this proved to be a way to get it to build.
looking for betters ways...
Wasmtime GitHub notifications bot (Dec 08 2023 at 16:44):alexcrichton submitted PR review:
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.
Wasmtime GitHub notifications bot (Dec 08 2023 at 16:44):alexcrichton submitted PR review:
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.
Wasmtime GitHub notifications bot (Dec 08 2023 at 16:44):alexcrichton created PR review comment:
Coudl this perhaps go through
name_package_moduleto leverage the preexisting logic to drop the version?
Wasmtime GitHub notifications bot (Dec 08 2023 at 17:09):jsturtevant commented on PR #7656:
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.
This additionally implements what I would expect which is that versions are dropped unless they're required,
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 https://github.com/bytecodealliance/wit-bindgen/pull/781. It adds the version if present.
Wasmtime GitHub notifications bot (Dec 08 2023 at 17:18):alexcrichton commented on PR #7656:
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.
Wasmtime GitHub notifications bot (Dec 08 2023 at 21:25):jsturtevant updated PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 21:25):jsturtevant edited PR review comment.
Wasmtime GitHub notifications bot (Dec 08 2023 at 21:25):jsturtevant submitted PR review.
Wasmtime GitHub notifications bot (Dec 08 2023 at 21:25):jsturtevant created PR review comment:
That was exactly what I needed and it was right in front of me :laughing:
Wasmtime GitHub notifications bot (Dec 08 2023 at 21:29):jsturtevant edited PR #7656.
Wasmtime GitHub notifications bot (Dec 08 2023 at 22:53):jsturtevant requested alexcrichton for a review on PR #7656.
Wasmtime GitHub notifications bot (Dec 11 2023 at 15:17):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 11 2023 at 15:50):alexcrichton merged PR #7656.
Last updated: Nov 22 2024 at 16:03 UTC