Also emit DEP_Z_INCLUDE when using pkg-config

[sfackler]

Downstream build scripts use DEP_Z_INCLUDE to configure the builds of other native libraries (for example in libgit2-sys). Previously, if libz was found via pkg-config and didn't have a system-default include directory those builds would fail.

[Byron]

Thanks a lot for the fix!

It looks good to me, but I wouldn't know how to test it, nor do I think it would be easy to write a test for this.

Can I assume that you validated this works as expected so I can merge?

Also, if you'd like to see a new release with this fix so it can trickle downstream, you could open another PR with the manifest version update, as I think this repository doesn't allow self-approvals.

Thanks again.

[sfackler]

Yep, I validated it with [patch.crates-io] on an internal thing that was previously failing to build libgit2.

Will follow up with a version bump PR.

[weihanglo]

Would it be better using std::env::join_paths instead of commas to maximize the compatibility?

[Byron]

This could be a great catch!

cargo:include is seemingly unlisted in the docs, maybe I am not looking in the right place though.
With the docs it should be clear what separators can be used - it's surprising that , is support if : or ; would be supported as well. But then again, everything is possible if it's documented somewhere.

[sfackler]

It is the old syntax for

cargo::metadata=KEY=VALUE — Metadata, used by links scripts.

[weihanglo]

cargo:include is arbitrary user-defined cargo:KEY=VALUE, which is superseded by cargo::metadata=KEY=VALUE to have a better extensibility. I am looking at other -sys crate and some of them have the same (not ideal?) pattern.

[Fabian-Gruenbichler]

this change broke cross compiling for Debian builds of rustc. Debian builds never use bundled system libraries (they are stripped from upstream sources). before this commit, pkgconf found the system libz and the build script exited. after this commit, even if pkgconf finds the system libz, the build script continues and the fallback to build the bundled version when cross compiling obviously can't work for us if the bundled sources are missing.

it's easy enough to patch around this on our end, I just wanted to make you aware that this not only changed the build script to emit an additional line, but changed its behaviour in a pretty fundamental way that might not have been intended.

[Byron]

Thanks for letting us know!

It's unsurprising that any change by now has great potential to break downstream. The reason for why the build script is what it is is hard to deduce, and even if it was perfectly documented it would probably be all too easy to make unintended changes.

After all, for the most part, the build script is untested as well, and testing this isn't trivial at all. In theory, each downstream user would have to contribute a CI setup that allows testing their particular requirements.

In lieu of that, maybe it would be possible for those who wish to prevent future breakage to get time to sign-off on planned changes. In theory, subscribing to all PRs should be enough to signal interest and make sure nothing breaks.

But that's all I got, and hope we can find something actionable here as otherwise I think it will just keep breaking.

CC @jongiddy

[Fabian-Gruenbichler]

on the Debian side we are used to patching build scripts (since we strip all bundled library copies and force the builds to use the system copy via pkgconf), and as far as that is concerned, libz-sys is actually quite good ;)