Exempt wasm-bindgen-related crates from version requirements#1683
Exempt wasm-bindgen-related crates from version requirements#1683LaurenzV wants to merge 6 commits into
wasm-bindgen-related crates from version requirements#1683Conversation
|
If the CLI needs to be the exact same What about Similarly, do the other two |
Yes, unfortunately they depend on And they are also pinned to exact versions, so they need to be in sync: https://github.com/wasm-bindgen/wasm-bindgen/blob/0621e40fc716b89a6f558dc844fc9419d06b7d85/crates/web-sys/Cargo.toml#L26-L27 Same for wasm-bindgen-futures and wasm-bindgen-test, so it likely applies to all crates in that family. |
|
What a mess. I am sympathetic to the goal of being able to use the same CLI version across revisions. The problem is that this will create the exact blindspot that the general policy is designed to avoid. Which is that Maybe that is an ok risk for |
|
We can still make sure that the latest version is used in our lock files right? I can revert the change to the lock file. But we just shouldn’t be too strict in the Cargo.toml file itself, but as long as we’ve tested that vello works with the minimum version we provided it should be fine, right? |
cc12604 to
55bf830
Compare
|
This policy of upgrading in
Can't this can happen even with our policy if a newer version of a crate is published after we publish? |
There was a problem hiding this comment.
My position would be:
- We (should!) pin a specific, lower version of wasm-bindgen, so the issue that we only test with newer deps doesn't apply.
- Any user who needs this feature should already be pinning their own version of wasm-bindgen. As such, I'm happy to say that any breakage resulting from a wasm-bindgen upgrade is the user's responsibility, as they will be explicitly upgrading the external dependency. This is similar to our position with rustc; we only guarantee we work on the rustc we've tested with, and that by upgrading rustc, you're taking on the risk that Rust broke us (but we'll fix any issues caused by new stables when we upgrade Rustc).
To spell it out, I do believe that wasm-bindgen specifically can be an exception to our policy, and so agree with this PR.
That being said, this can't land as-is for the reason explained in my PR comment.
I don't like that js-sys/web-sys have to be included here, but they do unfortunately pin a specific wasm-bindgen version, and likely have synced releases with wasm-bindgen.
| web-time = { workspace = true } | ||
| # If updating, also update in .github/workflows/web-demo.yml | ||
| wasm-bindgen = "=0.2.121" | ||
| wasm-bindgen = { workspace = true } |
There was a problem hiding this comment.
I'm not certain that this one is right; we still need to pin an exact version, so as to avoid a benign Cargo.lock bump breaking CI.
(That is, we specifically build this crate for WASM, so it needs to pin the version it uses. The same applies for all the example crates, I believe)
There was a problem hiding this comment.
As a piece of evidence here, consider the insane situation that this PR hasn't needed to change the version in the CI file. That's definitely wrong.
Am I understanding correctly that you are talking about |
|
Yes, we need to pin for examples, but not for the library crates. |
bc9f3b4 to
98aeb25
Compare
98aeb25 to
35d5c71
Compare
|
Okay so:
I hope that resolves the concerns that you had! |
We have a policy to 1) update all dependencies before a new release and 2) also specify the newest patch release that we use explicitly in the Cargo.toml. I think this makes sense in general, but I would like to propose that we except wasm-bindgen and the related crates from the second rule.
The reason for this is that
wasm-bindgenis special, in the sense that there is an additional CLI tool you need to use and the version of the CLI tool must exactly match the one used by the crate. Therefore, by bumping wasm-bindgen with each version (which will become even more common assuming we start doing more regular releases), we create a lot of very unnecessary churn for consumers to update their wasm-bindgen CLI version, even though we don't really need any of the new features from a patch release. For example, I try to sync up my own repo vello_bench2 every once in a while with the newest git version. I also have a tendency to check out and run older revisions so I can test whether there has been any regression in a specific time frame. Because of this, I created an alias which allows me to easily switch between multiple versions, and while that works fine, I think it would be much better if we didn't force this kind of churn from the vello side in the first place. Curious to hear your thoughts. 🙏Therefore, in this PR I have