rust-lang / pkg-config-rs

Build library for invoking pkg-config for Rust
https://docs.rs/pkg-config
Apache License 2.0
173 stars 79 forks source link

Allow disabling pkg-config altogether #141

Open ctron opened 1 year ago

ctron commented 1 year ago

As I understood it is possible to disable pkg-config for a single dependency using FOO_NO_PKG_CONFIG.

However, I didn't see any way to disable this for all crates. Similar to the PKG_CONFIG_ALL_STATIC variable.

I think support for this would make sense to fall back to an "all vendored" build. Failing the build if that isn't possible or misconfigured.

sdroege commented 1 year ago

That seems out of scope for pkg-config, but the system-deps crate is doing such things on top of pkg-config and also provides a generic way of hooking into functions for building the dependencies internally.

cargo really needs a better mechanism for external dependency discovery and building.

ctron commented 1 year ago

Hm, but it looks like people use pkg-config directly: https://github.com/sanpii/libpq.rs/blob/main/libpq-sys/Cargo.toml

So having the feature to disable this for one dependency, but not for all, doesn't seem reasonable to me.

sdroege commented 1 year ago

The main problem here is that this is not really composable. You would set this environment variable and it would affect literally every crate in your dependency tree that uses pkg-config. libpq-sys might have a fallback to build its dependencies internally, but something else in your dependency tree might not and it would simply fail now.

Hence my comment that this would ideally be solved at a higher level, ideally in cargo itself by providing mechanisms for external dependency discovery / building. The way how it is right now, everybody implements their own artisanal build.rs that behaves different from anybody else's and by that makes it a mess to build anything with external dependencies that regularly requires source changes for building in a specific environment.

ctron commented 1 year ago

libpq-sys might have a fallback to build its dependencies internally, but something else in your dependency tree might not and it would simply fail now.

Which is exactly what I would like to achieve. As right now, adding a new dependency to the tree (especially unintentionally) would simple enable using pkg-config. However, if I want to opt-out of using pkg-config, and use an "all vendored" approach, I wouldn't be aware of this.

Having a flag to completely opt-out of pkg-config would ensure that a failing build make me aware of that fact, allowing me to choose a way to deal with this.

sdroege commented 1 year ago

You could also set PKG_CONFIG=/usr/bin/false for that or just not have it available in the environment at all :) I'm not 100% opposed to adding an environment to pkg-config for this, it just doesn't seem to actually solve any problem.

But even then this doesn't prevent some build.rs to e.g. call xml2-config to find libxml2 instead of going via pkg-config, or to use the cmake dependency mechanism. I don't see how it would really solve your use-case of having everything vendored, which (again) seems like something to be solved at the cargo level instead of relying on different build.rs to do anything consistent.

ctron commented 1 year ago

I any case, I agree that there should be a more generic way to deal with this in cargo!

And using PKG_CONFIG=/usr/bin/false sounds like a neat way to solve my issue :)

It is true that it wouldn't prevent any library to do something in addition. But I saw a bunch of -sys creates which check for pkg-config first, and if that is available ignore all their other options that they document. Which leads to a case that you are never really aware of which flags are now actually in place.

I also understand that this isn't an issue created by pkg-config, but the way it is used in various build.rs scripts.

Then again, the changed sounds simple to me, and follows an existing pattern in pkg-config, why I assumed it would be a reasonable extension.

If you disagree, then I guess PKG_CONFIG=/usr/bin/false is good enough.

sdroege commented 1 year ago

I'm not sure, ideally I'd like to not add new such API unless absolutely needed.

Maybe @thomcc has a second opinion here?