Add Cargo features for more dependencies

[LoganDark]

This is what #55 should have been! A cargo features revolution!

This now allows turning off the input, battery, storage, framebuffer drawing, framebuffer text drawing, and lua features separately, basically a lot of stuff that can be separated out to make this library a lot more modular and configurable.

[LoganDark]

Ideally default-features should just be ['framebuffer-storage', 'framebuffer-text-drawing', 'battery', 'appctx-lua'] but I'm not sure (even if features are additive) if that would be equivalent to what is there now

[bkirwi]

Looks like this doesn't compile under at least some possible feature combinations; for example cargo build --no-default-features --features appctx.

---

Like I mentioned in #55, I'm pretty down on having large numbers of interlocking features like this. It makes it really hard to check properties (even basic stuff like whether the code compiles) under all configurations you ostensibly support.

I'm a bit more open to excluding some of the heavier deps, like hlua and image, that often go unused. We don't necessarily need to add any new features for that... could use the feature that comes for free with the optional dep, a la #[cfg(feature = "hlua")]. (Hopefully that avoids most of the extra compilation / hassle that's been bothering you?) On the other side, having features for input and battery seems like a maintenance hassle for little benefit.

We should also make sure to add checks to the github action for any new combination of features we intend to support.

Anyways, will see what the other maintainers think before asking for any changes!

[LoganDark]

Looks like this doesn't compile under at least some possible feature combinations; for example cargo build --no-default-features --features appctx.

appctx should enable all the features it needs - I'll take a look.

Like I mentioned in #55, I'm pretty down on having large numbers of interlocking features like this. It makes it really hard to check properties (even basic stuff like whether the code compiles) under all configurations you ostensibly support.

They're not all interlocking - although I get your point.

[bkirwi]

They're not all interlocking - although I get your point.

Yeah, that's fair, and I don't want to overstate it! I've just been burned enough times by conditional compilation to have the flinch reflex...

[LoganDark]

They're not all interlocking - although I get your point.

Yeah, that's fair, and I don't want to overstate it! I've just been burned enough times by conditional compilation to have the flinch reflex...

Yes, because Rust conditional compilation evokes duck-typing vibes and I don't like that either - feels a bit like Python. But, as long as all the permutations compile, you're good to go from there. I just need to test them all before committing :)

Fix coming soon!

[LoganDark]

Sorry for the long wait. I found out it was a lifetime error - rather than remove the lifetime from ApplicationContext only when appctx-lua is disabled, which would make it impossible for libraries to work both with and without it, I just added a PhantomData when Lua isn't enabled.

I'm not sure what the purpose is of allowing lifetimes other than 'static for the Lua, but it's potentially a useful feature that someone might use one day.

[LoganDark]

As a side note, it's really scary that the tests passed when the library wouldn't even compile with some features disabled. Is there no better way to test it than to try each permutation manually?

[LoganDark]

On the other side, having features for input and battery seems like a maintenance hassle for little benefit.

input was a little involved, but only a little. They aren't quite interlinked as deeply as you imply. For example battery is just excluding compilation of a module since it's completely independent.

I've now tested compilation with pretty much every combination of features. Enabling hlua on its own doesn't enable appctx but that's okay and causes no compilation errors.

[LoganDark]

image is extremely expensive to build on my machine, taking up to 27.18 seconds. Putting it behind a feature and disabling the feature reduces the number of crates that need to be built by around 31, and it makes up over half the crates required for a compilation with only the features framebuffer-drawing,image (21 crates without image, 52 with).

[LoganDark]

The things behind features, like draw_text, should probably be put into their own trait. This is already tons of breaking changes anyway. So I'll probably do that soon

[LoganDark]

libremarkable can now be made to build on desktop platforms with some subset of features (framebuffer-types, input-types)

[LinusCDE]

Didn't notice that this wasn't merged for 0.6.0. I'm very sorry for having missed this!

Also just noticed a situation, where I would have liked to have storage.rs but ended up reimplmenting it as I wanted to keep appctx out. Excactly what this PR would have solved.

@bkirwi Anything against merging this, so it can be included in a future (hopefully just) 0.6.1 ?

[LoganDark]

Didn't notice that this wasn't merged for 0.6.0. I'm very sorry for having missed this!

Even with me pinging and bothering you constantly for days? :(

I'll make sure to be even more annoying in the future!

[LoganDark]

By the way this is a breaking change due to the input types moving, so you'll have to do 0.7 for it

[LinusCDE]

Was kinda busy the last few days and had my github notifications unread. I usually at least look at all the mentions (automatically marked important/red for me thanks to my email filter) but didn't even bother looking through those the last few days. Should really at least do those. Also kinda assumed the release would have included this change.

Just got to do a bit foss today. Wanted to finally get the next release of plato out. Resulted in 2 new PRs and me now going through all the notifications. 5 hours in and still gotta do some package testing. Long night I guess but also a really productive day.

[LinusCDE]

We could also just do a breaking 0.6.1 for this in a few weeks. Technically this is not wrong.

https://doc.rust-lang.org/cargo/reference/semver.html#change-categories

This guide uses the terms "major" and "minor" assuming this relates to a "1.0.0" release or later. Initial development releases starting with "0.y.z" can treat changes in "y" as a major release, and "z" as a minor release. "0.0.z" releases are always major changes. This is because Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

[LoganDark]

We could also just do a breaking 0.6.1 for this in a few weeks. Technically this is not wrong.

https://doc.rust-lang.org/cargo/reference/semver.html#change-categories

This guide uses the terms "major" and "minor" assuming this relates to a "1.0.0" release or later. Initial development releases starting with "0.y.z" can treat changes in "y" as a major release, and "z" as a minor release. "0.0.z" releases are always major changes. This is because Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

It is because 0.6.0 to 0.6.1 is not breaking. 0.0.1 to 0.0.2 would be breaking.

[LinusCDE]

It is because 0.6.0 to 0.6.1 is not breaking. 0.0.1 to 0.0.2 would be breaking.

Thanks for the correction. Somehow I didn't notice that. Have been on gh too long for today it seems :laughing: .

[LinusCDE]

Using --all-features might be a good catch all.

[bkirwi]

I think default includes all features right now, but probably wise to be explicit! Other interesting points to test would be the one that previously had a bug here and the one that @LoganDark mentioned using for framebuffer-only unit tests.

[LoganDark]

It sounds like the other maintainers are happy to manage the extra features, so I'll leave any objections I have there.

The only pending change I know of is adding the new features to the github action so they're checked in CI. You can see the existing examples here. I don't think it makes sense anymore to test all possible combinations, since there's a lot now, but we should at least add the ones we'd expect to be common.

Would you be willing to do that for me? I'd really like to get this merged before 2023 but I'm currently caught up in a lot of stuff and have no idea how GitHub Actions / CI works.

[bkirwi]

Sure - should just be a small change to the file I linked; ought to be able to get to it within the next couple days.

[LoganDark]

Any progress?

[bkirwi]

There sure is now!

@LinusCDE / @fenollp - feel free to merge if the update looks good!

[LoganDark]

I think a future extension to this would be completely deleting all the Lua stuff from this library - as it really doesn't seem to have any place here, IMHO. It can be extracted to a separate crate, libremarkable-lua for example.

[LinusCDE]

Great idea. Not sure how closely appctx is tied to be used with lua.

Moving maybe both out (or just making appctx about non-lua while leaving the possibility to re-add it in another crate) could make an interesting new project. Having a lua runtime and adding support to tie lua into libremarkable would make a really powerful opportunity to allow users to create UIs and control all the stuff libremarkable can, from lua. With proper documentation and examples this would be a powerful way to enable users to create any app with a simple lua script!

Edit: We'd need a proper and recognizable name for that crate though. Maybe something like remarkable-lua (or better something way more creative that still gets the point across).

[LoganDark]

Great idea. Not sure how closely appctx is tied to be used with lua.

Ideally whatever features may be Lua-specific should be generalized and made available for use by all downstream libraries.