-
Notifications
You must be signed in to change notification settings - Fork 90
Interface version canonicalization #536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7b6bd7d
to
2f8eda8
Compare
For the binary encoding the most straightforward option from a quick review would seem to be adding variants of
I suppose if we wanted to optimize the binary a bit this extra field could contain just the part of the original version that got lopped off by canonicalization. On this field width:
https://semver.org/#does-semver-have-a-size-limit-on-the-version-string
🤷 |
@lann Thanks for starting this! For the binary encoding question: yes, taking over the
Is there a simplicity argument to be made that requiring the concatenation of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! A few drive-by comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oops, meant to "comment" not approve before it's even ready to review 🙃 )
For the binary encoding, here's another possible encoding:
maybe with affordances for rc/etc unsure. The basic idea though is that the actual import name would always be
For this I'd recommend using |
@alexcrichton Good idea; that cleanly answers some of the questions above. My only light concern is that tools might just treat the |
I agree yeah there's risk since the name in the binary format is "so simple", but yeah that's also where I'd hope that tests could weed things out. It'd be pretty simple in parser libraries I'd imagine to avoid exposing the full name as the import name if the discriminant was present. My thinking though was that the name always has a full and valid semver, as defined by semver itself. That way the discriminant says what the "real" import name is (e.g. chopping off other stuff) for linking/semantic purposes. Although I may be misunderstanding what you're thinking about how to drop the discriminant? |
I think @lukewagner is suggesting that the differences between 1/2/3 can be derived from the string itself. The algo would be something like:
|
Ah I see! So something like (as a transition to the future):
where in the future we'd drop 0x00 entirely (and possibly rename 0x01 to 0x00). The |
I think of the "semver-aware" options I prefer @lukewagner's 1 (extra) discriminant option; if you are parsing semver anyway then the logic is only marginally more complex than the 3 discriminant option. I'm more ambivalent on whether the parser should be semver-aware. I like the conceptual simplicity of "the name is the name" but this is a binary encoding and if we're going to require validation of semver then we're probably already committing to most of that code complexity anyway. |
We discussed this in a meeting today and decided to simplify a bit:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -399,7 +402,10 @@ Notes: | |||
`(result (own $R))`, where `$R` is the resource labeled `r`. | |||
* Validation of `[method]` names requires the first parameter of the function | |||
to be `(param "self" (borrow $R))`, where `$R` is the resource labeled `r`. | |||
* `<valid semver>` is as defined by [https://semver.org](https://semver.org/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<valid semver>
wasn't referenced in this file.
interfaceversion ::= '@' <valid semver> | ||
| '@' <canonversion> | ||
canonversion ::= [1-9] [0-9]* | ||
| '0.' [1-9] [0-9]* | ||
| '0.0.' [1-9] [0-9]* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some ambiguity here; '0.0.' [1-9] [0-9]*
matches a subset of <valid semver>
. I think this makes sense in the context of non-canonical versions getting phased out but could add more to clarify if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point and I think you're right; it's fine if we word the validation rules in terms of "matching canonversion
" as suggested above.
namespace ::= <words> ':' | ||
words ::= <word> | ||
| <words> '-' <word> | ||
projection ::= '/' <label> | ||
version ::= '@' <valid semver> | ||
# FIXME: surrounding alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO after final review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! Just a few small comments:
export ::= (export <id>? "<exportname>" <sortidx> <externdesc>?) | ||
import ::= (import "<importname>" <versionsuffix>? bind-id(<externdesc>)) | ||
export ::= (export <id>? "<exportname>" <versionsuffix>? <sortidx> <externdesc>?) | ||
versionsuffix ::= (versionsuffix "<semver suffix>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should <semver suffix>
be defined below (it doesn't seem to be on semver.org)? If so, then could it be semversuffix
(since we otherwise don't use spaces for production names defined by this doc).
interfaceversion ::= '@' <valid semver> | ||
| '@' <canonversion> | ||
canonversion ::= [1-9] [0-9]* | ||
| '0.' [1-9] [0-9]* | ||
| '0.0.' [1-9] [0-9]* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point and I think you're right; it's fine if we word the validation rules in terms of "matching canonversion
" as suggested above.
@@ -2539,6 +2547,46 @@ annotations. For example, the validation rules for `[constructor]foo` require | |||
for details. | |||
|
|||
|
|||
### Canonical Interface Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you find the place in the preceding "Import and export definitions" section where valid semver
is mentioned and update that paragraph to link down to this new section?
`wasi:http/[email protected]` and a host provides the (subtype-compatible) interface | ||
`wasi:http/[email protected]`, we'd like to make it easy for the host to link with the | ||
guest. The `canonversion` for both of these interfaces would be `0.2`, so this | ||
linking could be done by matching canonical interface names literally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also mention the more-nuanced "forward compatibility" case (a component imports the new version, but uses only the functions present in the hold version with compatible types)?
@@ -399,7 +402,10 @@ Notes: | |||
`(result (own $R))`, where `$R` is the resource labeled `r`. | |||
* Validation of `[method]` names requires the first parameter of the function | |||
to be `(param "self" (borrow $R))`, where `$R` is the resource labeled `r`. | |||
* `<valid semver>` is as defined by [https://semver.org](https://semver.org/) | |||
* Validation requires that `versionsuffix` is preceded by an `interfaceversion` | |||
with a `canonversion` and that the concatenation of the `canonversion` and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with a `canonversion` and that the concatenation of the `canonversion` and the | |
matches `canonversion` and that the concatenation of the `canonversion` and the |
See #534