Remove smdhtool dependency by using cytryna library#50
Remove smdhtool dependency by using cytryna library#50Maccraft123 wants to merge 2 commits intorust3ds:masterfrom
Conversation
ian-h-chamberlain
left a comment
ian-h-chamberlain
left a comment
There was a problem hiding this comment.
Nice and simple API and really quite a small amount of code changes! I think this looks pretty good overall, but would be good to get some other @rust3ds/active opinions as well.
There was a problem hiding this comment.
Whew, this adds quite a lot of dependencies to Cargo.lock huh?
I tried building with cargo install -Zunstable-options --timings just to see long it takes, and I noticed something interesting — with this change, we now have syn at both v1.0 and v2.0 which are one of the longest parts of the build. I wonder if there is some way we could massage the dependencies (either here or in cytryna) to maybe reduce the dependency tree a little?
On the whole though, I think the spirit of #2 probably means it's worth the extra build time. It might also be an incentive to prebuild binaries with something like https://github.com/axodotdev/cargo-dist or at least make sure we're compatible with https://github.com/cargo-bins/cargo-binstall, but those can be done separately at some point.
There was a problem hiding this comment.
Should be a bit better now... aside the "now we have 2 versions of syn" stuff
| toml = "0.5.6" | ||
| clap = { version = "4.0.15", features = ["derive", "wrap_help"] } | ||
| shlex = "1.1.0" | ||
| image = { version = "0.24.7", default-features = false, features = ["png"] } |
There was a problem hiding this comment.
relating to my other comment, we could also consider lodepng (smdhtool uses the C++ version of this I think) or png as potentially lighter-weight alternatives, although image is nice in that we could potentially support other image formats. I'm not sure that we really need to support anything other than PNG though honestly, it's a pretty universal format that most users should be able to produce.
@Meziu any preference here?
There was a problem hiding this comment.
image without all of its default features doesn't have many dependencies, and with png feature it uses png crate under the hood
There was a problem hiding this comment.
Ah, I didn't realize that cytryna itself had a direct dependency on image. It should be fine though, as you said its dependency tree is not too large
| /// Builds the smdh using `cytryna` library. | ||
| pub fn build_smdh(config: &CTRConfig) { | ||
| let smdh = Smdh::builder() | ||
| .with_short_desc(&config.name).unwrap() |
There was a problem hiding this comment.
Is it possible the user could input something that would cause these unwraps to fail? Would the error be understandable enough to easily fix if so?
If not we could augment with an expect or map_err just to make it more clear, I guess.
There was a problem hiding this comment.
User could input a very long string for any of those which would error out and yeah expect for those is a good idea
2 participants
This is related to #2 and in future similar PRs will close that issue