rust-lang / libz-sys

Rust crate package to link to a system libz (zlib)
Apache License 2.0
110 stars 73 forks source link

Add ZLIB_DIR config option, and don't vendor zlib or zlib-ng automatically #147

Open micolous opened 11 months ago

micolous commented 11 months ago

libz-sys should have a configuration option like ZLIB(_LIB|_INCLUDE)_DIR which allows you to explicitly point at which zlib to use on all platforms, similar to what openssl-sys does with OPENSSL(_LIB|_INCLUDE)_DIR (documentation).

If those environment variables are unset, then libz-sys should continue to use vcpkg, pkg-config or system paths to find zlib.

Additionally, vendoring should be behind a feature flag, disabled by default, so that libz-sys failng to find zlib is a hard error by default:

These changes would require a major version bump, as they're SemVer incompatible changes, and will likely break some environments until they can be fixed up.

Some related issues include:

83 has a similar request to disable auto-vendoring for zlib-ng.

Byron commented 11 months ago

Thanks compiling this request, it makes sense to me, and especially the example of openssl-sys seems to hammer home why the current behaviour should be adjusted.

A major version bump seems like the right path to release such a change, and I think that flate2 should be the crate to adopt the next major release of libz-sys first and possibly also signal a breaking change to avoid strange effects in some dependency trees which relied on implicit vendoring.

In any case, it's quite a journey, but one that seems to make sense to take.

joshtriplett commented 10 months ago

One issue with a major version bump is that the ecosystem would all have to migrate over, and no crate could have two versions in its dependency tree at the same time, because you can't have two versions of zlib linked into your program at the same time.

Byron commented 10 months ago

I wonder if there is a way to do this without a major version bump, as that seems like something to generally be avoided. Maybe it's also possible to 'protect' the current release before creating a new major version to support multiple major versions in the same tree. Could symbol renaming be used for this?

micolous commented 10 months ago

I proposed this was SemVer breaking because the way to choose which zlib is built is incompatible. That point may be debatable in light of other potential issues from a major version bump.

Removing vendoring is more likely to be an end-user-impacting change, rather than one seen by dependants' developers. The symptom will be along the lines of "package X (which depends on zlib) doesn't build anymore", because the end-user's build environment.

This is most likely to impact building on Windows (where you need to bring your own zlib) and to a lesser extent Linux (where someone didn't install the zlib-dev/zlib-devel package).

For end-users, it's all fixable pretty easily – this needs some communication with downstream dependants. However, they have limited actions they could take:

Unfortunately, I don't believe that the Rust ecosystem has any way to manage this sort of detection or communication at scale.

In any case, the problem is generally with individuals' build environments rather than dependants "holding it wrong", and the current state of libz-sys masks this sort of problem.

The risk with any version bump and symbol renaming is shaking out all the dependants which are holding their version of libz-sys back. Which makes me lean towards avoiding a major version bump if possible.

I think there's going to be some short term pain no matter what, but this is still a defect which needs fixing.

micolous commented 10 months ago

Actually, there may be one channel to communicate via: rustsec.

While it's not a security bug in the traditional sense, I'd argue that "silent auto-vendoring" is a soundness issue in the sense it causes unexpected behaviour, that may warrant an "informational" advisory.

Byron commented 10 months ago

After having re-read the thread, here is the major points as I understand them:

First of all, if it's a bug it's something that could be fixed without a major version change. This would cause downstream churn for everyone who used auto-vendoring by default who will now have to chose vendoring at the very least. If I see this correctly, this would be an issue for those who build binaries, but also for those who want to run tests of their libraries.

If I remember correctly, compiling anything with OpenSSL fails by default if the developer sources aren't installed on the system (gitoxide suffers from this), and It's always frustrating as the issue occours once, and then never again as the respective files persist on the system so one forgets about it. Having the same behaviour for flate2 would probably have a large impact to how 'easy' the cargo toolchain is perceived.

This is why I want to consider the current convenience as valuable and thus as something worth protecting.

What if the auto-vendoring could be turned off?

Doing so should be another solution to the problem, as it would allow those who have trouble due to vendoring could turn it off. When off, it could act similarly to openssl-sys, and could also be made to properly interact with static (along with fixing the mentioned issue).

To me this seems feasible, but please help point out the propositions flaws.

BlackDex commented 4 months ago

Any progress or ideas on this?

If you build on a system which has a static zlib/libz already compiled, thus a libz.a already exists, it's useless (and maybe even harmful) to build it via vendor. The versions could mismatch with other libraries build using the systems static libz.a

While it probably doesn't hit any errors right now, it could of course.

Byron commented 4 months ago

There were no additional comments related to the question "What if the auto-vendoring could be turned off?", which might make it the way to go.

Such a change would have to be contributed though.

chenrui333 commented 1 week ago

Any updates on this thread?