linebender / parley

Rich text layout library
Apache License 2.0
228 stars 28 forks source link

Update to current `swash` 0.1.19 #124

Closed waywardmonkeys closed 1 month ago

waywardmonkeys commented 1 month ago

This lets everything use the same version of the Fontations dependencies.

waywardmonkeys commented 1 month ago

@xStrom Do you have any idea why this fails? It is kind of strange (to me at least).

waywardmonkeys commented 1 month ago

This does bring us back to where @nicoburns feels that the no_std support is fairly broken (and at this point after re-digging into it earlier today, I partially agree).

nicoburns commented 1 month ago

@waywardmonkeys I can't reproduce the clippy failure locally, but it does seem to be correct that that import isn't required: code compiles just fine with only libm feature (no std feature) without it.

waywardmonkeys commented 1 month ago

You have to do this to reproduce it:

❯ cargo clippy -p parley --no-default-features --features libm
warning: unused import: `core_maths`
 --> fontique/src/attributes.rs:7:5
  |
7 | use core_maths::*;
  |     ^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `fontique` (lib) generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
waywardmonkeys commented 1 month ago

And there are ways to build so that you get the failure. The code that breaks are the calls to f32::fract() which are provided in the no_std case by core_maths::CoreFloat.

waywardmonkeys commented 1 month ago

No warning:

      Running
      `.../.rustup/toolchains/stable-aarch64-apple-darwin/bin/clippy-driver
      .../.rustup/toolchains/stable-aarch64-apple-darwin/bin/rustc
      --crate-name fontique
      --edition=2021
      fontique/src/lib.rs
      --error-format=json
      --json=diagnostic-rendered-ansi,artifacts,future-incompat
      --diagnostic-width=447
      --crate-type lib
      --emit=dep-info,metadata
      -C embed-bitcode=no
      -C debuginfo=2
      -C split-debuginfo=unpacked
      --warn=unused_qualifications
      '--warn=clippy::trivially_copy_pass_by_ref'
      '--warn=clippy::semicolon_if_nothing_returned'
      '--warn=clippy::doc_markdown'
      --cfg 'feature="libm"'
      --cfg 'feature="std"'
      --cfg 'feature="system"'
      --check-cfg 'cfg(docsrs)'
      --check-cfg 'cfg(feature, values("default", "icu_properties", "libm", "std", "system", "unicode_script"))'
      -C metadata=41a8710cc85c3996
      -C extra-filename=-41a8710cc85c3996
      --out-dir .../parley/target/debug/deps
      -C incremental=.../parley/target/debug/incremental
      -L dependency=.../parley/target/debug/deps
      --extern core_foundation=.../parley/target/debug/deps/libcore_foundation-0b75b6efed6c29bd.rmeta
      --extern core_text=.../parley/target/debug/deps/libcore_text-611ff251778b78b4.rmeta
      --extern core_maths=.../parley/target/debug/deps/libcore_maths-eba51a8405b050e8.rmeta
      --extern hashbrown=.../parley/target/debug/deps/libhashbrown-348c6abd65fec943.rmeta
      --extern icu_locid=.../parley/target/debug/deps/libicu_locid-3906c7c48555d1b8.rmeta
      --extern memmap2=.../parley/target/debug/deps/libmemmap2-ed24483a7659eaef.rmeta
      --extern objc2=.../parley/target/debug/deps/libobjc2-ea31d3118c8350ed.rmeta
      --extern objc2_foundation=.../parley/target/debug/deps/libobjc2_foundation-4ffd1c07b6d62f91.rmeta
      --extern peniko=.../parley/target/debug/deps/libpeniko-7edd5f4d286a9563.rmeta
      --extern skrifa=.../parley/target/debug/deps/libskrifa-249a6d4923070689.rmeta
      --extern smallvec=.../parley/target/debug/deps/libsmallvec-2d73d348b7e5db9b.rmeta`

Warning:

      Running
      `.../.rustup/toolchains/stable-aarch64-apple-darwin/bin/clippy-driver
      .../.rustup/toolchains/stable-aarch64-apple-darwin/bin/rustc
      --crate-name fontique
      --edition=2021
      fontique/src/lib.rs
      --error-format=json
      --json=diagnostic-rendered-ansi,artifacts,future-incompat
      --diagnostic-width=447
      --crate-type lib
      --emit=dep-info,metadata
      -C embed-bitcode=no
      -C debuginfo=2
      -C split-debuginfo=unpacked
      --warn=unused_qualifications
      '--warn=clippy::trivially_copy_pass_by_ref'
      '--warn=clippy::semicolon_if_nothing_returned'
      '--warn=clippy::doc_markdown'
      --cfg 'feature="libm"'
      --check-cfg 'cfg(docsrs)'
      --check-cfg 'cfg(feature, values("default", "icu_properties", "libm", "std", "system", "unicode_script"))'
      -C metadata=8f9b0093d2ab25c1
      -C extra-filename=-8f9b0093d2ab25c1
      --out-dir .../parley/target/debug/deps
      -C incremental=.../parley/target/debug/incremental
      -L dependency=.../parley/target/debug/deps
      --extern core_foundation=.../parley/target/debug/deps/libcore_foundation-0b75b6efed6c29bd.rmeta
      --extern core_text=.../parley/target/debug/deps/libcore_text-59fa1e071e5581f5.rmeta
      --extern core_maths=.../parley/target/debug/deps/libcore_maths-eba51a8405b050e8.rmeta
      --extern hashbrown=.../parley/target/debug/deps/libhashbrown-75fc60be1a34ac66.rmeta
      --extern icu_locid=.../parley/target/debug/deps/libicu_locid-7efd4f24404316f9.rmeta
      --extern objc2=.../parley/target/debug/deps/libobjc2-ea31d3118c8350ed.rmeta
      --extern objc2_foundation=.../parley/target/debug/deps/libobjc2_foundation-e3b2af3e8e0e0d47.rmeta
      --extern peniko=.../parley/target/debug/deps/libpeniko-6253f9103bb8f8cc.rmeta
      --extern skrifa=.../parley/target/debug/deps/libskrifa-f09209c52f086386.rmeta
      --extern smallvec=.../parley/target/debug/deps/libsmallvec-2d73d348b7e5db9b.rmeta`

So the difference is that in the first case (the old / current code), it was getting both libm and std, while in the code from this PR, it just gets libm.

waywardmonkeys commented 1 month ago

My inclination is to consider removing the no_std CI for now until things can be made to work better.

The build isn't actually usable anyway right now on a no_std target (like x86_64-unknown-none) (doesn't even come close to compiling).

But it would be nice to hear from more people.

nicoburns commented 1 month ago

I don't think I quite understand why this is happening. The error is in fontique but it only happens when you compile parley as the top-level crate. Is it because a parley dependency is pulling in the std crate but not enabling the std feature of fontique?

My inclination would be to just stick an #[allow(unused)] on that import for the time being.

It's a bit loose, but if we wanted to be more precise then we should just use libm functions directly rather than core_math with a wildcard import (I personally really dislike anything with a wildcard import, as it makes it really hard to see which code actually requires the import).

waywardmonkeys commented 1 month ago

About to eat dinner, but did a quick change and push to see if that works to suppress it.

waywardmonkeys commented 1 month ago

It's a bit loose, but if we wanted to be more precise then we should just use libm functions directly rather than core_math with a wildcard import (I personally really dislike anything with a wildcard import, as it makes it really hard to see which code actually requires the import).

This can be better by importing just the trait CoreFloat, but what you asked for isn't how it would work (or works in many other no_std crates).

I can explain to you what's going on here with that another time.

nicoburns commented 1 month ago

This can be better by importing just the trait CoreFloat, but what you asked for isn't how it would work (or works in many other no_std crates).

That would be an improvement. But I still don't love conditional imports all over the place.

In Taffy we have a sys module that does the majority of the conditional importing and means that most modules can just unconditionally import the types that they need from that module.

waywardmonkeys commented 1 month ago

I will merge this after I eat and do a quick review of the repo to see if we are ready for the release.