Always generate the same output structure with bindgen!
#8721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently with the `bindgen!` macro when the `with` key is used then the generated bindings are different than if the `with` key was not used. Not only for replacement purposes but the generated bindings are missing two key pieces: * In the generated `add_to_linker` functions bounds and invocations of `with`-overridden interfaces are all missing. This means that the generated `add_to_linker` functions don't actually represent the full world. * The generated module hierarchy has "holes" for all the modules that are overridden. While it's mostly a minor inconvenience it's also easy enough to generate everything via `pub use` to have everything hooked up correctly. After this PR it means that each `bindgen!` macro should, in isolation, work for any other `bindgen!` macro invocation. It shouldn't be necessary to weave things together and remember how each macro was invoked along the way. This is primarily to unblock bytecodealliance#8715 which is running into a case where tcp/udp are generated as sync but their dependency, `wasi:sockets/network`, is used from an upstream async version. The generated `add_to_linker` does not compile because tcp/udp depend on `wasi:sockets/network` isn't added to the linker. After fixing that it then required more modules to be generated, hence this PR.
alexcrichton
commented
May 31, 2024
| @@ -136,9 +136,10 @@ pub mod sync { | |||
| async: false, | |||
| with: { | |||
| "wasi:http": crate::bindings::http, // http is in this crate | |||
| "wasi:io": wasmtime_wasi::bindings::sync, // io is sync | |||
| "wasi:io": wasmtime_wasi::bindings::sync::io, // io is sync | |||
There was a problem hiding this comment.
This was a bug from before this PR and probably shouldn't have ever worked...
lann
approved these changes
May 31, 2024
github-actions
bot
added
wasi
pchickey
approved these changes
May 31, 2024
There was a problem hiding this comment.
cc @MendyBerger can you see if this improves any of the code we looked at together earlier this week?
|
@pchickey I'm unable to get the code to compile when I rebase on top of this git rev. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently with the
bindgen!macro when thewithkey is used then the generated bindings are different than if thewithkey was not used. Not only for replacement purposes but the generated bindings are missing two key pieces:In the generated
add_to_linkerfunctions bounds and invocations ofwith-overridden interfaces are all missing. This means that the generatedadd_to_linkerfunctions don't actually represent the full world.The generated module hierarchy has "holes" for all the modules that are overridden. While it's mostly a minor inconvenience it's also easy enough to generate everything via
pub useto have everything hooked up correctly.After this PR it means that each
bindgen!macro should, in isolation, work for any otherbindgen!macro invocation. It shouldn't be necessary to weave things together and remember how each macro was invoked along the way. This is primarily to unblock #8715 which is running into a case where tcp/udp are generated as sync but their dependency,wasi:sockets/network, is used from an upstream async version. The generatedadd_to_linkerdoes not compile because tcp/udp depend onwasi:sockets/networkisn't added to the linker. After fixing that it then required more modules to be generated, hence this PR.