use zlibVersion() instead of a const for the version#491
use zlibVersion() instead of a const for the version#491Byron merged 1 commit intorust-lang:mainfrom
zlibVersion() instead of a const for the version#491Conversation
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot, it's great to see that this could work!
I am a bit afraid that this could be a breaking change, as I am pretty sure the the value returned by zlibVersion() won't be exactly the same in all cases.
It's certainly OK to mark this as breaking change and increment the minor version, after all it's probably not many people relying on it.
If we go in that direction, it would probably be helpful to document the new version string format so it can go into the changelog.
Also, I think it's worth it to have another opinion on this.
This mechanism is for the library to ensure that 1) everything was linked correctly and 2) the required features in zlib are supported. Generally that means that certain symbols are available that weren't in earlier versions. So the one failure mode I can see is if I believe though that the symbols/functions that |
|
As far as I can see it's not something that's ever accessible externally, the version string is just accessed and passed into the function where in case of the original zlib, there is a check whether the first byte matches with the internal version number and that the string pointer is not The underlying zlib version isn't something that can be relied on in any case unless you control the full distrubution since e.g some linux distros like fedora swap out zlib for zlib-ng and debian (and by extension ubuntu) patches flate2 to remove all zlib variations and zlib-rs to only use system stock zlib. |
Byron
left a comment
There was a problem hiding this comment.
Thanks so much for elaborating, @folkertdev and @oyvindln!
Then it should be good to go and in future, zlib-rs and flate2 don't have to be published in lock-step anymore.
However, I will give it till Friday for more comments, and then merge it should there be nothing else coming up.
|
Sounds good! I think that, because zlib-rs is pre 1.0, we'll need to change the version bound to (to |
I don't think there is any downside to just using these functions really. Technically it's an extra function call (unless LTO is able to remove it, I guess). For zlib-rs, the function can just be inlined (it's a
const fneven).