linebender / parley

Rich text layout library
Apache License 2.0
158 stars 18 forks source link

Add the standard Linebender CI script and satisfy Clippy. #33

Closed xStrom closed 2 months ago

xStrom commented 2 months ago

This PR ports over the standard Linebender CI from Vello.

The effective MSRV is 1.70 because of peniko.

The standard .rustfmt.toml and .clippy.toml files are added, see Kurbo's repo for reference.

The CI runs three different Android targets armv7-linux-androideabi, aarch64-linux-android, x86_64-linux-android based on android_trace. Should be fine given the small nature of this repo.

Clippy fixes are mostly trivial. I added allow attributes for the few that would introduce too much noise into the code. Two lints are worth pointing out in more detail.

  1. The removal of std::mem::drop(fs) in src/shape.rs. Clippy complained that FontSelector doesn't implement the Drop trait. However even if it did, to me it looks like the scope of the fs variable ends right there anyway and so it would be auto-dropped. Hence the safe removal. Did I miss something?
  2. Compiling without the std feature was completely broken. For now I added a bunch of feature gates to get it compiling, but I didn't pay any attention to whether the crate still provides any value without std enabled. This also introduced a bunch of dead code, for which I currently just added a warning suppression. If we continue to have a std feature then that dead code should get feature guards too. Some of it isn't trivial, because e.g. a generic enum loses its genericness but all the callers still expect it to be generic.
xStrom commented 2 months ago

Looking more into the std feature, even though parley now compiles without it set, it still actually depends on the Rust std library. The following command, which ensures the Rust std library isn't available, will fail:

cargo check --no-default-features --target x86_64-unknown-none

So if we want to have true no_std support there is still quite some work to be done in addition to this PR.

@dfrg can you give some insight into your goals and expectations around the std feature? Would it be feasible for fontique and/or parley? Should we continue with trying to get it to work?

When I brought this issue up at office hours, Raph contemplated whether we even need the std feature and perhaps we should just always requrie std to be present. That would certainly be the easy way out. However we do successfully support a no_std story in both peniko and kurbo, so there is precedent in getting it done.

dfrg commented 2 months ago

Oops, yes, this is a mess. The back story is that fontique had a working no_std build prior to adding the query functionality. When I vendored the code into parley, I never updated it so there were a lot of dangling cfg feature attributes.

The current state is that parley depends on the query code which has a hard dependency on the source cache and thus std. This can and should be fixed but requires some minor architectural changes.

The major open question here is whether fontique should be a separate crate. @nicoburns and I have both made cases for this and I think we should do it.

In the meantime, the changes here put us into a weird state (disabling std will compile but makes parley effectively non-functional) but also get CI working and are already done so my vote is to land this now and address the structural issues in a followup.

nicoburns commented 2 months ago

Looks like Fontique is primarily using std for filesystem access (and env variables in a couple of places). I suspect the way to address that medium-to-long-term is to separate out:

  1. The code which enumerates and loads fonts from the system
  2. The queryable "font database"

1 will always require std, but could potentially be elided or replaced by other code in no_std contexts. 2 likely only requires alloc and can be changed to unconditionally use alloc.

xStrom commented 2 months ago

Alright, sounds like we want to get std working later, I'm fine with that. Yeah this PR doesn't really get us all the way there, but it at least makes it compile so it's a step in the right direction. We can work on the changes later and it doesn't have to be a blocker for the first release either.

The major open question here is whether fontique should be a separate crate. @nicoburns and I have both made cases for this and I think we should do it.

I think that question has a pretty definitive answer now, which is that yes we'll make it a crate. Raph was the only one hesitating, but when talking about it at the last office hours, he said that you and Nico convinced him it's worth it. So now there is nobody left who isn't on board with the idea. 👍

The split itself should be pretty straightforward and I was planning on giving it a go tomorrow. Wanted to get a working CI first though, so that it would be more clear if the split PR breaks anything.

I'll leave this PR open for now (as it is bed time for me), and maybe @DJMcNab wants to give this a quick peek tomorrow. Then in the afternoon I'll merge it and also submit the split PR.

xStrom commented 2 months ago

Alright I'll go ahead and merge this. Any additional tweaks can be follow-ups with a working CI to test them.