Skip to content

Conversation

@jsturtevant
Copy link
Contributor

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

@jsturtevant jsturtevant requested a review from a team as a code owner December 8, 2023 02:54
@jsturtevant jsturtevant requested review from pchickey and removed request for a team December 8, 2023 02:54
@jsturtevant jsturtevant force-pushed the support-versioned-exports branch from 786d8d6 to 6494fa2 Compare December 8, 2023 02:56
@jsturtevant
Copy link
Contributor Author

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",
    }

@jsturtevant jsturtevant force-pushed the support-versioned-exports branch from 319c983 to 62c143e Compare December 8, 2023 05:01
Copy link
Member

@alexcrichton alexcrichton left a 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.

@jsturtevant
Copy link
Contributor Author

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 bytecodealliance/wit-bindgen#781. It adds the version if present.

@alexcrichton
Copy link
Member

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>
@jsturtevant jsturtevant force-pushed the support-versioned-exports branch from 62c143e to 7ed8ef2 Compare December 8, 2023 21:25
@jsturtevant jsturtevant changed the title [bindgen] Include Version in the export name [bindgen] Include Version in the export name if needed Dec 8, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Dec 11, 2023
Merged via the queue into bytecodealliance:main with commit ef70686 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants