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

More flexible Requires and Requires.private support #151

Open
gdesmott opened this issue Dec 8, 2020 · 8 comments
Open

More flexible Requires and Requires.private support #151

gdesmott opened this issue Dec 8, 2020 · 8 comments

Comments

@gdesmott
Copy link
Contributor

gdesmott commented Dec 8, 2020

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? :)

@sdroege
Copy link
Contributor

sdroege commented Dec 8, 2020

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.

@gdesmott
Copy link
Contributor Author

gdesmott commented Dec 8, 2020

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?

@lu-zero
Copy link
Owner

lu-zero commented Dec 8, 2020

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

lu-zero commented Dec 21, 2020

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).

@gdesmott
Copy link
Contributor Author

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.

@lu-zero
Copy link
Owner

lu-zero commented Sep 10, 2021

Is this still a problem?

@gdesmott
Copy link
Contributor Author

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

@lu-zero
Copy link
Owner

lu-zero commented Sep 20, 2021

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

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

No branches or pull requests

3 participants