cargo-c icon indicating copy to clipboard operation
cargo-c copied to clipboard

More flexible Requires and Requires.private support

Open gdesmott opened this issue 3 years ago • 8 comments

Since #143 users can now manually pass a set of Requires and Requires.private fields for the generated pkg-config file.

It's been suggested to automatically populate those from system-deps's metadata.

We also have the problem of dependencies which may be either external or built-in. As a result an external dependency should or should not be listed as a requirement in the generated pkg-config file.

So we have various problems to solve here:

Which pkg-config modules are required to build the crate?

As suggested on #139 this information can be extracted from system-deps metadata. cargo metadata already does all the Cargo.toml aggregation for us, so all we have to do is "just" to recursively parse each dep of our crate and collect all the packages from there.

Unfortunately it's a bit trickier as the exact deps used may depend on build time features or env variable (such as SYSTEM_DEPS_$NAME_BUILD_INTERNAL to statically link to a dep). system-deps could generate something like target/debug/system-deps.json with the exact settings (pkg-config name, flags, etc) which have been configured for each -sys crate.

cargo-c would then aggregate both to retrieve the exact list of system packages which have been used during the build.

Requires vs Requires.private

Once we have all the packages cargo-c has no way to know if those should be listed as Requires or Requires.private. So we'd still need some manual configuration from users.

sys crates which are not using system-deps

The solution proposed above rely on system-deps so won't work with crates which are not using it. I'm afraid there is not much we can do for those.

@sdroege : any smart idea here? :)

gdesmott avatar Dec 08 '20 12:12 gdesmott

Once we have all the packages cargo-c has no way to know if those should be listed as Requires or Requires.private. So we'd still need some manual configuration from users.

The distinction here would be based on whether the dependency is required by the header files, right? So if the header file is generated by cbindgen, we should somehow know or not? If the header file is generated otherwise then the user will have to provide this information.

sdroege avatar Dec 08 '20 12:12 sdroege

The distinction here would be based on whether the dependency is required by the header files, right?

Yes, any type exposed in the public API, so in the header, should have its lib be in Requires, all the other deps should be Requires.private.

So if the header file is generated by cbindgen, we should somehow know or not?

Yes, cbindgen is generating the header. I never used it (no header for the gst plugins) but according to the doc it will look for types in the dependencies as well. I don't know if it supports re-using types from an external C library or not. If it doesn't then that's easy, we just always use Requires.private.

@lu-zero : do you know how this works exactly in cbindgen?

gdesmott avatar Dec 08 '20 13:12 gdesmott

cbindgen let you blacklist types/functions/constants, so you can hide them from your .h and just add verbatim #include lines.

I would spend energy in converting packages to use system-deps instead of figuring out how to guess something from a situation that would remain completely brittle, no matter what we do.

lu-zero avatar Dec 08 '20 16:12 lu-zero

I'm thinking of making a new release of cargo-c this week, after we could see if we can make a proof-of-concept. I'll put down some notes on what we could do manually and then how to automate it (we might need to modify cargo a lot).

lu-zero avatar Dec 21 '20 07:12 lu-zero

Works for me. I don't have much time to work on this atm so having a release with the existing features would be good. The existing Requires API still has some use.

gdesmott avatar Dec 21 '20 08:12 gdesmott

Is this still a problem?

lu-zero avatar Sep 10 '21 13:09 lu-zero

I think so, the general issue described in the top comment still applies.

gdesmott avatar Sep 20 '21 15:09 gdesmott

I guess now the time would be fine to poke the concept. I'd start restricting it to system-deps users only.

lu-zero avatar Sep 20 '21 16:09 lu-zero