Implement canonical interface names in wit-component - #2622
Conversation
Tweak the string-splitting to more closely match the intent of versions found in tooling, specifically: * Document that build metadata (`+foo` in `0.0.1+foo`) is always split out and not part of the canonical name. * Don't split out pre-release information since pre-release versions are incompatible with all other versions. This additionally matches preexisting merging behavior in `wit-component` as discovered in bytecodealliance/wasm-tools#2622
|
|
||
| fn encode_interface_export( | ||
| &mut self, | ||
| export_name: &str, |
There was a problem hiding this comment.
Right now this name logic is now split between the caller and the end of this function, but perhaps this parameter could be changed to ComponentExternName directly to avoid this duplication and instead have the logic in just one place?
| external_id: resolve.external_id_value(key, item).map(|s| s.into()), | ||
| version_suffix: None, | ||
| let implements = resolve.implements_interface(key, item); | ||
| if canonical_names { |
There was a problem hiding this comment.
In a previous review I was curiuos if it would be possible to deduplicate the number of places that a canonical-names option was taken into account and a ComponentExternName were created. I count currently four different locations doing very similar things:
- here
- below in this file in
for interface in interfaces - in
encode_interface_importinencoding.rs - in
encode_interface_exportinencoding.rs(split across two functions)
Were you able to take a look and see if these locations could be unified? Is there perhaps one, or maybe two at most, helpers that could be used to construct these names?
There was a problem hiding this comment.
Will think about it tomorrow.
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Were there more changes you wanted to make as well? I think the main previously-mentioned-and-possibly-outstanding comment of mine is refactoring the construction fo ComponentExternName, but if you'd prefer that can be deferred to a future change too.
Otherwise though in reading over the tests again I'm thinking that the component model should probably disallow an empty versionsuffix field, which would then require some changes here too. Other than that though I think this is reasonable to land.
Thanks again for working on this!
| ) | ||
| ) | ||
| (import "wasi:io/poll@0.2.0-rc-2023-11-10" (instance (;0;) (type 0))) | ||
| (import "wasi:io/poll@0.2.0-rc-2023-11-10" (versionsuffix "") (instance (;0;) (type 0))) |
There was a problem hiding this comment.
I think we'll probably want to handle this differently with a few layers:
- In the abstract this is a wast of binary-encoding space so we wouldn't want to emit the
versionsuffix - Additionally though the component model should probably add a validation rule that the length of
versionsuffixis >0 since there's not much point in having a 0-length version suffix
Would you be up for adding the new validation rule, adding tests for that, and then adjusting the encoder to skip the suffix if it's empty?
There was a problem hiding this comment.
Eventually yes. During the transition period, is it better to always have a versionsuffix, so that tools can easily tell if the binary is using the canonical name or not? Or we can put something in the producer section, like processed-by wit-component --emit-canonical-names? Happy to implement this either way.
|
Thanks for the review! There may be some follow up PR to fix the decoder side, so that the decoder doesn't need to reconstruct the full version using the As for refactoring During the transition period, it's helpful to know if the binary is encoded using canonical-names or not. I was wondering the best way to do this, either we add something in the producer section, or we emit versionsuffix everywhere even if it's empty. Which one do you prefer? |
2 participants
Follow-up to #2556 and replaces #2602.
emit_canonical_namesinComponentEncoderto emit canonical version.merge_world_imports_based_on_semverthatimplementsversion is not updated during the merge.--emit-canonical-namestowasm-tools component new,embedandwit.During the transition period, upstream libraries, e.g., wac, can decode the original binary and re-encode with the
emit_canonical_namesflag enabled, so that we convert all binaries into their canonical encoding as a preprocessing step. Thenwaccan merge the interface via string matching directly.