Make framebuffer an optional feature

[lluchs]

Some users of libremarkable might want to use it only for its input
handling, but not for its framebuffer functionality. By disabling the
framebuffer feature, users can also avoid lots of dependencies (73
instead of 151 dependencies to install), some of which (e.g., zstd) even
need a C compiler.

The default features will build everything as before, so this will not
cause any compatibility issues.

See rien/reStream#46 for an example of a libremarkable use that does not need any framebuffer functionality.

[fenollp]

@lluchs The addition of the dimensions module and the splitting into various features makes sense to me, plus I'd like to see usage of libremarkable in reStream (and others) and would love to help get your PR over there closer to being merged. Could you rebase your PR?

@bkirwi @LinusCDE What do you think of these changes?

[LinusCDE]

The changes seem alright to me. 👍

[bkirwi]

Quick thoughts:

• I have a slight bias against using features in general; they make the project somewhat harder to reason about for maintainers and users.
• On the other hand, the reduction in dependencies here is very significant, and probably worth it.
• If it were up to me, I'd make the cut slightly differently: keep the framebuffer and its basic features, but exclude the app stuff and the more complex parts of FramebufferDraw. This would have a similar reduction in the number of dependencies -- I think memmap2 is the only additional dep over what's required in this PR -- but still be useful for those who are working with the framebuffer directly and not using the app framework. (Disclaimer: this describes me, so I'm somewhat biased!)

If folks are open to the latter bullet, I'd be happy to make an attempt at another branch. If that doesn't make sense to folks, agree this PR is still better than status quo!

[bkirwi]

I guess concretely, looking at the specific dep list:

framebuffer = ["rusttype", "mmap", "image", "ioctl-gen", "line_drawing", "zstd"]
appctx = ["aabb-quadtree", "hlua"]

1. zstd supports CompressedCanvasState, which is only used in the demo app, and could be moved to appctx.
2. rusttype, image, line_drawing are all in support of FramebufferDraw, which is mostly used by the app stuff.
3. mmap and ioctl-gen are, I think, essential for the framebuffer stuff to work at all.

My impression is that 3. is necessary for most (but not all!) apps, 2. is sometimes used but not as essential, and 1. is not used much. (But I haven't searched exhaustively of course.) So I guess the specific proposal would be to move 1. and 2. behind the appctx flag, and not have 3. behind a flag at all.

(Also happy to just merge some version of this and have that discussion as followup.)

[LinusCDE]

• I have a slight bias against using features in general; they make the project somewhat harder to reason about for maintainers and users.

I personally have not much of an opinion on this, yet. Surely this adds a bit of complexity. If the default flags are basically all, applications developers wouldn't notice it on their part while allowing "power developers" do disable stuff they dislike.

My gripe was that e.g. libremarkable used a specific zstd version and I later found out for a feature I'm not sure anyone has ever used. (But I kinda rebuilt it in doomarkable by accident). Such a feature seemed sensible to remove though since I have never heard of an application using this component.

1. zstd supports CompressedCanvasState, which is only used in the demo app, and could be moved to appctx.
2. rusttype, image, line_drawing are all in support of FramebufferDraw, which is mostly used by the app stuff.
3. mmap and ioctl-gen are, I think, essential for the framebuffer stuff to work at all.

My impression is that 3. is necessary for most (but not all!) apps, 2. is sometimes used but not as essential, and 1. is not used much. (But I haven't searched exhaustively of course.) So I guess the specific proposal would be to move 1. and 2. behind the appctx flag, and not have 3. behind a flag at all.

I have used the FramebufferDraw feature in almost every project of mine and personally see it as essential. I wouldn't put it together with appctx since that would lump zstd into my dependencies.

I'm not sure on the best way either, but I find the original pr to be a sensible first step. But I also agree that we could move zstd and CompressedCanvasState into appct. But maybe I'm biased here since my projects of mine use some kind of own light "UI" (Usually with a canvas.rs as a light wrapper over the framebuffer instance) instead of using appctx.

[bkirwi]

We should add "framebuffer" to the list here... enabling appctx without framebuffer would break the build.

[bkirwi]

If the default flags are basically all, applications developers wouldn't notice it on their part while allowing "power developers" do disable stuff they dislike.

Yeah. My main worry is that it introduces configurations that are difficult to test.

(For example, even the two features added here have a problem interaction.)

I have used the FramebufferDraw feature in almost every project of mine and personally see it as essential.

Yeah, fair enough. I also use FramebufferDraw, but have ended up reimplementing the high level stuff like text rendering over time to get more control. (Still using the lower-level stuff like line drawing, but that doesn't need dependencies.) But it is awfully convenient to have the basics built in for sure...

Anyways, maybe we should put the zstd stuff behind the appctx feature, and we can come back to the rest of it later if we want?

[LinusCDE]

(For example, even the two features added here have a problem interaction.)

Good catch!

Anyways, maybe we should put the zstd stuff behind the appctx feature, and we can come back to the rest of it later if we want?

Agreed. While optimizing dependencies a lot would be cool, having features at all is already a lot better than before (where you needed all dependencies even if you e.g. not used the framebuffer at all). We can always iterate on this later.

[bkirwi]

It sounds like we're roughly on the same page, but this branch has bitrotted a bit... unless anyone objects or wants to grab it first, I can merge / make those changes directly on this branch?

[bkirwi]

Okay, merged and applied the minor changes we discussed. I've also added checks for the two new build features into the Check build stage, so we should be able to spot any regressions...

[bkirwi]

(I've also tested with one of my apps that does not use appctx -- I can compile with only the framebuffer feature enabled. But if anyone else wants to kick the tyres that's even better!)

[bkirwi]

@LinusCDE / @fenollp - i think this is ready to merge; approved?

[fenollp]

Two features are introduced but only one is checked?

[bkirwi]

Basically: after this PR, there are three interesting cases to check: [], [framebuffer], and [framebuffer, appctx]. default = [framebuffer, appctx], so that combination was already being tested by the existing check... this commit adds checks for the other two options.

I agree it's nonobvious, though. Maybe I should add a comment?

[fenollp]

I'm probably missing something otherwise please add another check in CI :)

[LinusCDE]

I'll let you both be the judge when this is ready. Seems pretty much finished to me if nothing else comes up.