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!
| Some(implements) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
I think these changes may no longer be necessary
There was a problem hiding this comment.
FWIW the changes here seem fine, but for future PRs I the test here don't really have much to do with the feature being added here so I think it'd be fine to just hardcode this to either false or true
| ) | ||
| ) | ||
| (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?
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 newDuring 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.