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
| bail!(offset, "invalid interface version: {e}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this validation move up to the above method? I think it'd be a bit clearer to perform this validation when implements is specified rather than deferring it to happen later here. That'd probably require validating the version_suffix field before implements and reorderin the above if statements, but that should be fine to do.
There was a problem hiding this comment.
Done. Neither way is perfect. We validate version_suffix with ComponentName here. Ideally, we would also validate the suffix with implements here. But moving up does make a smaller diff.
|
|
||
| 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.
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.