Fix inference conflict with serde_json through schemars feature
#733
Conversation
| @@ -45,6 +45,7 @@ vello_shaders = { version = "0.3.0", path = "vello_shaders" } | |||
| bytemuck = { version = "1.18.0", features = ["derive"] } | |||
| skrifa = "0.22.3" | |||
| peniko = "0.2.0" | |||
| kurbo = "0.11.1" | |||
There was a problem hiding this comment.
It is required by the addtion of kurbo = { workspace = true, optional = true } in with_winit/Cargo.toml
without that it gets the following:
error: process didn't exit successfully: `~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo metadata --format-version=1 --no-deps` (exit status: 101)
--- stderr
error: failed to load manifest for workspace member `~/git/linebender/vello/examples/with_winit`
referenced by workspace at `~/git/linebender/vello/Cargo.toml`
Caused by:
failed to parse manifest at `~/git/linebender/vello/examples/with_winit/Cargo.toml`
Caused by:
error inheriting `kurbo` from workspace root manifest's `workspace.dependencies.kurbo`
error: process didn't exit successfully: `~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo metadata --format-version=1 --no-deps` (exit status: 101)
--- stderr
error: failed to load manifest for workspace member `~/git/linebender/vello/examples/with_winit`
referenced by workspace at `~/git/linebender/vello/Cargo.toml`
Caused by:
failed to parse manifest at `~/git/linebender/vello/examples/with_winit/Cargo.toml`
Caused by:
error inheriting `kurbo` from workspace root manifest's `workspace.dependencies.kurbo`
Caused by:
`dependency.kurbo` was not found in `workspace.dependencies`
We could I suppose specify 0.11.1 in with_winit rather than picking it up from the workspace,
but the two versions (the one inherited through peniko, and the one specified explicitly) need to be kept in sync
to trigger the error (at least that is my belief, until a time where we have the feature supported in peniko itself).
So a bit oddly neither i with_winit or in the root Cargo.toml is perhaps an ideal place for it.
There was a problem hiding this comment.
Yeah, this is just my mistake. I misunderstood the context of this change
There was a problem hiding this comment.
Perhaps at the very least we could add a comment specifying that the kurbo line can be removed once peniko supports the schemars feature and needs to be kept in sync with the version of kurbo used by peniko?
Edit: Added in 049892c
There was a problem hiding this comment.
It might also work to use a "*" version, but I don't have a good intuition about whether that will do what you want; it might be worth having a #[test] when the hidden feature is enabled which validates that the Peniko and Kurbo versions are compatible.
There was a problem hiding this comment.
Adding a #[test] definitely sounds like a good idea, will do.
I don't have a good intuition about whether a "*" version will do the right thing either though.
There was a problem hiding this comment.
I added that #[test] in ac97f1e, I don't think it will automagically run during ci.
but at the very least you can do so manually with cargo test --features _ci_dep_features_to_test
That said I did try to take care that the test doesn't actually require initializing gpu, etc.
There was a problem hiding this comment.
Hmm would it not be automatically run in CI? It seems to be in your latest commit:
https://github.com/linebender/vello/actions/runs/11730274415/job/32677779520#step:7:159
There was a problem hiding this comment.
Hmm would it not be automatically run in CI? It seems to be in your latest commit: https://github.com/linebender/vello/actions/runs/11730274415/job/32677779520#step:7:159
Well, I guess the following is enough for this test, since all we need to test is that it compiles.
cargo clippy --tests --benches --examples --locked --no-default-features --features _ci_dep_features_to_test -- -D warnings
But these appear to be running running clippy, not actually running any test target, that said I think it is sufficient? I had originally missed that it was actually running with the --tests, so had assumed it wasn't compiling them either.
serde_json through schemars feature
|
Is anything blocking this now? @ratmice you should have permission to merge this if it's finished. |
|
No there was nothing really blocking, I just am always hesitant to pull the trigger when things have been approved, but i've made subsequent changes since then, given that I've made more changes, I'll give it a few days at least unless anything further comes up. |
| @@ -45,6 +45,7 @@ vello_shaders = { version = "0.3.0", path = "vello_shaders" } | |||
| bytemuck = { version = "1.18.0", features = ["derive"] } | |||
| skrifa = "0.22.3" | |||
| peniko = "0.2.0" | |||
| kurbo = "0.11.1" | |||
There was a problem hiding this comment.
Hmm would it not be automatically run in CI? It seems to be in your latest commit:
https://github.com/linebender/vello/actions/runs/11730274415/job/32677779520#step:7:159
Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
|
Thanks for your reviews Daniel, will merge this tomorrow unless anything else comes up. At the very least though this does not feel as fragile as it was initially. |
Initially marking as a draft, because this currently just fiddles with Cargo.toml to ensure that CI tests the feature.
this shouldn't actually require any changes to our CI scripts to trigger, and as Daniel suggested, this uses a feature on with_winit rather than vello itself.
Will follow up with the actual fix once I've confirmed CI fails.