Open IceTDrinker opened 8 months ago
Could be easier and may "just" need to check the manifest_path
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?
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
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
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
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.
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
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.
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
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):
The plan for the semver trick is:
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
https://github.com/mozilla/cbindgen/blob/6bfc2176187a6fc6fba6315323b0296112330294/src/bindgen/cargo/cargo.rs#L185
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)