cbindgen ignores version/don't parse depedency flag when selecting crates to expand which can make it unusable in certain situations

[IceTDrinker]

Hello,

We are currently exploring using the https://github.com/dtolnay/semver-trick to be able to give old crates the ability to convert some serialized data to more modern formats.

The basics are as follow (taking our https://github.com/zama-ai/tfhe-rs project as example):

• we have released 0.4.0 with a certain serialization format
• we plan on releasing 0.5.0 with breaking changes to the serialization
• we want to provide a way for users to update their serialization format for old data

The plan for the semver trick is:

• release 0.5.0
• update the 0.4 code base to be able to convert the old format to the new format and release 0.4.1

We have C APIs we need to be able to expose so we want to run cbindgen.

Currently cbindgen parses the cargo metadata ignoring the version of the package for which the binding is being generated

cbindgen/src/bindgen/cargo/cargo.rs

Line 185 in 6bfc217

fn find_pkg_ref(&self, package_name: &str) -> Option<PackageRef> {

In the current prototype we randomly expand the 0.4.1 crate (which we want, it is the current workspace crate being worked, it's the old one that will expose the forward compatible conversion functions for serialization) but we also sometimes expand the 0.5.0 (as it appears as a dependency and cbindgen does not discriminate against it) which causes issues with a CargoExpand error indicating we can't specify features for dependencies. We have disabled parsing for dependencies but the flag is not considered either (it would be fine for us if it took it into account)

I believe it's possible to support the semver-trick edge case (it is very much an edge case I get that perfectly), I'm willing to contribute the patch if necessary or collaborate/answer questions on that particular thing.

I'm sure we can hack up something on our end but I think it would be better if it's a thing cbindgen supports, I don't know if cargo metadata track dependencies, but if it does I would say that honoring the "do not expand dependencies" during the metadata parsing would be an acceptable solution and maybe would avoid issues in situations where two crates may accidentaly share a name in local dev environments (though in that case the easy fix is not naming different stuff with the same name)

[IceTDrinker]

Could be easier and may "just" need to check the manifest_path

[mversic]

We have disabled parsing for dependencies but the flag is not considered either (it would be fine for us if it took it into account)

what happens in this case? what flag isn't considered?

[IceTDrinker]

We set parse_deps to false, but as the bug is happening in the way candidate crates are derived from the cargo metadata I believe it has no discernable effect

[mversic]

that sounds like a bug. Couldn't we fix this behavior? The dependency should get excluded if parse_deps: false or if it's listed in excluded

[IceTDrinker]

Well the actual bug is parsing metadata without checking the version IMO, but as indicated on the PR, I don’t know what the course of action should be on cbindgen side to determine the version of the crate to properly select it in the cargo metadata.

I was thinking about canonicalizing paths but it introduces potential panics and would likely break with complex and networked build systems

[IceTDrinker]

To be more precise, if a project has a dependency with the same name, cbindgen gets confused with the path to use, cargo metadata has two entries with the same name but normally different path and versions, as it’s a hashmap we randomly pick the right crate.

One possibility is the manial version filter I proposed (as automating it with cbindgen looks non trivial), the other is based on path but introduces potential issues.

[IceTDrinker]

Basically cbindgen does not even realize it's parsing a dependency as it's relying on the name to know if it's the main project

[alilleybrinker]

The cbindgen expand config could be modified to optionally accept a version number to match in addition to the package name. I’ve encountered this same issue on one of my own projects as well.

[IceTDrinker]

Yep tried that as first version the patch and I think that’s what we ended up shipping (or some path based thing to validate the manifest) in a fork for our use case

[twitu]

I'm facing a similar situation. The below snippet declares MAX twice which fails to compile.

#[cfg(feature = "a")]
pub const MAX: u64 = u64::MAX();

#[cfg(not(feature = "a"))]
pub const MAX: u128 = u128::MAX();

In this case even cbindgen:ignore doesn't help because cbindgen exports the variable as is without inlining its value.

/// cbingen:ignore
#[cfg(feature = "a")]
pub const MAX: u64 = u64::MAX();

/// cbingen:ignore
#[cfg(not(feature = "a"))]
pub const MAX: u128 = u128::MAX();

pub const C_EXPORT_MAX = MAX;

Better support for feature flags will certainly help here. On the other hand, better integration of cbindgen with cfg attributes is another way to make it play well with feature flags.

#[cfg_attr(not(feature = "a"), cbindgen:ignore)]
pub const MAX: u64 = u64::MAX();

#[cfg_attr(feature = "a", cbindgen:ignore)]
pub const MAX: u128 = u128::MAX();

[emilio]

@twitu that's a totally different issue. But it seems what you want there is to teach cbindgen about the feature, which is supported. Then cbindgen should generate appropriate #ifdefs.

[twitu]

Indeed it's a different and supported feature thanks for pointing it out. Although, I noticed that the cython code it generates for ifdef will soon be deprecated by cython. I'll open a new issue for this. #1044