EBNull
commented
5 months ago Thanks for considering the proposal and working on a potential solution that meets the values of the project.
I understand your reasoning re dependencies and the like; you're right that this addition might not justify the addition of ~180kB.
I implemented this initially for myself and figured I could share what I had done.
Ultimately, the addition of this is of course your call - feel free to close this unimplemented. Thanks for your work on evsieve! I appreciate the thought that has gone into the program and the stream-based paradigm it operates under.
I'm really on the fence about this one.
On one hand, providing information about MSC events is probably not going to hurt the user, and may help them understand why those events exist.
This feature feels like it belongs more to another program like evtest, but then again, I have been using evsieve as an evtest alternative myself. And just because evtest should have this feature doesn't mean that evsieve shouldn't.
However, evsieve calls itself a "reasonably lightweight" utility. "Reasonably lightweight" does not mean "minimalist". Features that few people use are acceptable as long as they have negligible performance impact on those who do not use the feature.
I imagine very few people map EV_MSC events with evsieve, and this pull request makes evsieve 240kb larger in release+strip mode, which is an increase of 24%. That impacts the load time of the program and how much memory it uses. I don't think many users would actually care about the scancodes, so it does feel somewhat bloaty.
Also, you may have noticed that Cargo.toml contains only two dependencies, both of which are associated with the rustlang team. I've been aggressively minimizing the dependencies of this program for reasons that I don't even know myself. This pull request tries to add a new obscure dependency, which means that I have to vet the dependency myself for security purposes. As well as all future versions of it. That sounds like work. It also pulls in thiserror as recursive dependency, which pulls in a bunch more of recursive dependencies. I know that thiserror is a bog standard library in the Rust ecosystem, but still.
I tried to implement the feature myself without the hut dependency (see commit 68b0fb0) in an attempt to reduce the bloat, but it still ended up increasing the size of the binary by 180kB, which is more than I expected. I think that there is a lot of overhead because each string pointer occupies an additional 16 bytes in addition to the actual data in the string.
I think that there are two ways to get this done with less bloat:
The first approach has the issue that it complicates the compilation and deployment; currently evsieve is a single binary and most users are still supposed to compile it themselves. The second approach has the issue that I'm going to need to add some decompression library to my dependencies.
Regardless of the approach, there is also the issue of how to display the usage tables in the print messages. It is said that 80 characters is the standard terminal width, and adding the usage information causes the prints to exceed that width. This causes line wrapping on small terminals, such as a terminals which occuppy half of my 16:9 screen. That causes line wrapping, and line wrapping is not pleasant to look at. I think that the additional inconvenience to thin terminals may not even be worth the extra information for most users.
I suppose I could print the information if a flag
hidinfois provided to the--printargument, so it is only displayed to those who really want to know. But then this feature loses the advantage of explaining to unknowning users why those MSC_SCAN events exist and what purpose they serve. Which makes the bloat even harder to justify.My current thoughts are: