Conversation
On my i5-8350U, this lowers the build time in release mode from 1.55s to 1.16s, just for this crate. On my iBook G4 the improvement is much more dramatic, going from 57.06s to 43.25s. And since the flate2 dependency isn’t built any longer, a full build goes from 2:13 and 11 units down to 1:16 and 8 units.
It is still a direct dependency of flate2.
On my i5-8350U, this lowers the build time in release mode from 1.55s to 0.68s, just for this crate, with only the "encoder" feature enabled. On my iBook G4 the improvement to this crate is much more dramatic, going from 57.06s to 24.01s.
Since edition 2021 (bumped in af38962), std::convert::TryInto, std::convert::TryFrom and std::iter::FromIterator are already imported in the prelude, so we don’t have to import them manually.
b2a1c54 to
f7880c3
Compare
This might work: And yes, merging this without covering all potential feature combinations would not work out well. You've already seen from the PR itself that it is quite brittle to miss the annotation on one or another item. |
c6dfa76 to
feb3e77
Compare
|
Thanks! I now check I consider this PR ready for review now. :) |
.github/workflows/rust.yml
Outdated
| strategy: | ||
| matrix: | ||
| features: ["", "unstable", "benchmarks"] | ||
| features: ["decoder", "encoder", "decoder,encoder", "decoder,encoder,unstable", "decoder,encoder,benchmarks"] |
There was a problem hiding this comment.
We should probably also keep a check somewhere that empty features works. (I don't expect it to do anything useful, but we wouldn't want there to be compile errors either)
There was a problem hiding this comment.
Default features do work, but CI always passes --no-default-features. Or do you mean building a crate which does absolutely nothing useful (that is, with no features) should be supported?
There was a problem hiding this comment.
Passing no features at all should be supported. Or at least it shouldn't produce a compile error. (I have no reason to expect that it wouldn't work now, but good to keep a CI job to confirm)
There was a problem hiding this comment.
What do you expect the crate to do with no decoder and no encoder enabled? Export no symbol whatsoever?
There was a problem hiding this comment.
I’ve changed the commit adding compile_error!() into one which hides everything and disables all dependencies, making the crate a noop when no features are enabled.
I still think that can’t be what users want, and at least a compile-time warning would be better (but there is no compile_warning!() in std…). If you depend on a crate but disable all of its features, why do you depend on it in the first place? An error would certainly let the user think about what they are doing.
|
One other question. Does this qualify as an API breaking change? I'm not sure if anyone is currently running without default features, but if so their code would probably stop working after this PR. |
|
It does indeed break their code, we might want to bump to 0.19.0 once we merge that one. Another option (but I dislike it much more) is to expose the same API but without any implementation, which would then error out at runtime instead of at compile-time. But then it doesn’t let users of this crate aware that they have to enable the features they want. Although the default features currently don’t enable anything, so I hope not many people disable them. |
This makes the crate completely empty, not exporting anything and not depending on anything.
Thanks @197g for the fix!
This makes sure we won’t regress on this separation.
feb3e77 to
e9a1c0b
Compare
| decoder = ["bitflags", "crc32fast", "fdeflate"] | ||
| encoder = ["bitflags", "crc32fast", "fdeflate", "flate2"] |
There was a problem hiding this comment.
Since bitflags, crc32fast, and fdeflate are needed regardless of whether encoding or decoding is used, I don't think they should be optional dependencies
There was a problem hiding this comment.
Since both decoder and encoder are optional now, we can build with no dependency at all. (I still don’t think this configuration should be possible though.)
|
I think that's a good direction. It is a breaking change at least theoretically. It could be made backwards compatible by adding a third feature that enables support for the other two (like |
|
I believe this will be solved soon by migrating to |
This allows applications which only want to decode PNGs, or to encode PNGs, to not embed code for the direction they don’t need, with a compile-time error if neither are enabled.
On my i5-8350U, in release mode, current master builds in 1.55s, with only the
decoderfeature it builds in 1.13s, and with only theencoderfeature in 0.68s.On my iBook G4, still in release mode, the improvements are much more dramatic, a full build of
pngthat would previously take 2:13 and 11 units, goes down to 1:16 and 8 units with only thedecoderfeature enabled, since it doesn’t build theflate2crate and its dependencies. With just theencoderfeature it takes 1:40, which is also much better on this slow CPU. :)I’ve also cleaned up extra imports already present in the prelude since edition 2021, and removed the now-indirect
miniz_oxidedependency.This PR is best reviewed commit-per-commit.
Edit: I still need help to get the docstring tests to only run if the relevant feature is enabled, I couldn’t figure out how to do that so far.
Edit 2: Do we want to also test for only-encoder and only-decoder configurations now, and disable no-default-features checks as that means nothing?