Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix inference conflict with serde_json through schemars feature #733

Merged
merged 8 commits into from
Nov 9, 2024
Merged

Fix inference conflict with serde_json through schemars feature #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a6fc15a
Cause ci to trigger the kurbo/schemars feature in with_winit
ratmice Nov 4, 2024
3d09ae3
Add type annotations needed with kurbo schemars feature
ratmice Nov 4, 2024
9aa58a8
rustfmt
ratmice Nov 4, 2024
5bb5258
use u32::try_from instead
ratmice Nov 4, 2024
049892c
Add comment on kurbo version syncing in root Cargo.toml
ratmice Nov 4, 2024
ac97f1e
Add test for schemars
ratmice Nov 7, 2024
9d04706
Update examples/with_winit/src/lib.rs
ratmice Nov 8, 2024
ce4b8fe
Remove schemars dep, fix imports
ratmice Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ vello_encoding = { version = "0.3.0", path = "vello_encoding" }
vello_shaders = { version = "0.3.0", path = "vello_shaders" }
bytemuck = { version = "1.18.0", features = ["derive"] }
skrifa = "0.22.3"
# The version of kurbo used below should be kept in sync
# with the version of kurbo used by peniko.
peniko = "0.2.0"
# FIXME: This can be removed once peniko supports the schemars feature.
kurbo = "0.11.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used

Copy link
Contributor Author

@ratmice ratmice Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@DJMcNab DJMcNab Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just my mistake. I misunderstood the context of this change

Copy link
Contributor Author

@ratmice ratmice Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@DJMcNab DJMcNab Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ratmice ratmice Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

futures-intrusive = "0.5.0"
raw-window-handle = "0.6.2"
smallvec = "1.13.2"
Expand Down
4 changes: 4 additions & 0 deletions examples/with_winit/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ default = ["wgpu-profiler"]
# Enable the use of wgpu-profiler. This is an optional feature for times when we use a git dependency on
# wgpu (which means the dependency used in wgpu-profiler would be incompatible)
wgpu-profiler = ["dep:wgpu-profiler", "vello/wgpu-profiler"]
# Test for dependencies which implement std traits in ways that cause type inference issues.
_ci_dep_features_to_test = ["dep:kurbo", "kurbo/schemars"]

[lints]
workspace = true
Expand All @@ -41,6 +43,8 @@ log = { workspace = true }
# We're still using env-logger, but we want to use tracing spans to allow using
# tracing_android_trace
tracing = { version = "0.1.40", features = ["log-always"] }
# For _ci_dep_features_to_test feature tests.
kurbo = { workspace = true, optional = true }

[target.'cfg(not(target_os = "android"))'.dependencies]
# We use android_logger on Android
Expand Down
9 changes: 9 additions & 0 deletions examples/with_winit/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,3 +945,12 @@ fn android_main(app: AndroidApp) {

run(event_loop, args, scenes, render_cx);
}

#[cfg(all(feature = "_ci_dep_features_to_test", test))]
#[test]
// This just tests that the "kurbo" dependency we enable schemars for
// aligns to the same version that vello's peniko dependency resolves to.
fn test_kurbo_schemars_with_peniko() {
use std::marker::PhantomData;
let _: PhantomData<kurbo::Rect> = PhantomData::<vello::peniko::kurbo::Rect>;
}
4 changes: 3 additions & 1 deletion vello/src/scene.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,9 @@ impl<'a> DrawGlyphs<'a> {
EmojiLikeGlyph::Bitmap(bitmap) => {
let image = match bitmap.data {
bitmap::BitmapData::Bgra(data) => {
if bitmap.width * bitmap.height * 4 != data.len().try_into().unwrap() {
if bitmap.width * bitmap.height * 4
!= u32::try_from(data.len()).unwrap()
{
// TODO: Error once?
log::error!("Invalid font");
continue;
Expand Down