Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
micolous opened this issue Jul 28, 2023 · 9 comments
Open

Comments

@micolous
Copy link
Contributor

micolous commented Jul 28, 2023

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:

  • While auto-vendoring makes libz-sys build in more scenarios with zero configuration, it leads to surprising behaviour, and makes it more difficult to track down your dependencies. Again, this is something that openssl-sys does well.

  • There's also no way to reliably force the library to be vendored, except by setting the static option - which is itself implemented incorrectly, as it never checks for a system-installed static zlib except with vcpkg.

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
Copy link
Member

Byron commented Jul 28, 2023

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
Copy link
Member

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
Copy link
Member

Byron commented Aug 11, 2023

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
Copy link
Contributor Author

micolous commented Aug 29, 2023

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:

  • If they have C code which links with zlib (as in the case with curl-rust), they'd need to fix it.
  • If they provided build instructions from setting up an environment from the rustup stage, that'd need to be updated.
  • If they build/run their code in CI, that would need checking after the change.
  • ...otherwise there's not much they can do.

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
Copy link
Contributor Author

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
Copy link
Member

Byron commented Sep 2, 2023

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

  • the current auto-vendoring behaviour is a problem as it is implicit and doesn't interact well with dependency trees that have ZLIB configured in any other way. In these cases, it can't be deactivated.
  • Vendoring cannot be activated by feature toggle either, except one accepts a static build. The latter has its own issue by ignoring system-libraries on non-windows platforms.
  • The current behaviour is convenient for many, but may prevent usage or make it more difficult for a few.

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
Copy link
Contributor

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
Copy link
Member

Byron commented Feb 21, 2024

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
Copy link

Any updates on this thread?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants