mozilla / cbindgen

A project for generating C bindings from Rust code
Mozilla Public License 2.0
2.29k stars 294 forks source link

feat: allow users to specify a crate version for bindings generation #901

Closed IceTDrinker closed 4 months ago

IceTDrinker commented 8 months ago

this is a PR that attemps to solve https://github.com/mozilla/cbindgen/issues/900 in a way that does not break anything that already works with cbindgen as this is a pure addition to the code base leaving all existing behaviors untouched (other solutions may involve canonicalizing paths which may panic in places where there were no panics and may cause issues for any build systems with symlinks and network disks).

This is akin to cargo's package spec e.g. cargo build -p my_crate@0.1.0 to allow disambiguating which crate the command is referring to in cases where several crates either share the same name or several version of the same crates happen to be in the dependency/cargo metadata for a given project. The good news is that the package version is populated by Cargo in the environment when in a build system so it is easy to use for build script users. For binary users a quick parsing of their Cargo.toml with theri favorite Unix utils should be enough

To avoid changing flags a separate flag is added to the cbindgen binary and a with_crate_version is added to the cbindgen Builder to be able to use that in a build script.

@emilio as indicated in the contributing.md I am requesting a code review for this PR, I'm not quite sure how to write a test case for this right now, if you have some pointers? Maybe a separate crate_version file akin to what the profile test file might be doing ?

Cheers

IceTDrinker commented 6 months ago

hello any news on that @emilio I guess (?) or @mversic sorry if I'm not pinging the right persons, but if this PR does not get a chance to progress we likely will have to publish a custom version of cbindgen and I'd prefer avoiding that

I'm afraid the deadline is too close anyways for us to hold out longer on this :|

mversic commented 6 months ago

but I also think this could be resolved without introducing another flag

IceTDrinker commented 6 months ago

but I also think this could be resolved without introducing another flag

Definitely agree, the thing is I have no idea how cbindgen might be used in the wild, not being a maintainer myself I chose a path of least resistance and safety with respect to the current behavior, adding a new flag and keeping the old default behavior was a way to guarantee things would work as before for people not aware/needing the flag.

Let me know what should be done here (not sure there is a « simple » way to derive a crate’s version apart from using the cargo env var about the version but that would make cbindgen depend on the cargo environment being there and set-up which I believe is not the case today)

IceTDrinker commented 6 months ago

@mversic I think I have something which looks to be working with manifest paths, it probably can break things with people using relative paths so I added a fallback that has the old behavior of returning the first package that has been found, otherwise it means cbindgen needs to canonicalize some paths and this can result in panics that could never occur before

IceTDrinker commented 6 months ago

Gentle ping @emilio it was suggested on Zulip I pinged you here on this PR for issue #900.

Recap:

cbindgen parses the cargo metadata based on name only, which fails randomly to select the proper crate for cbindgen if a dependency has the same name as the main crate (as the cargo metadata is in a hash table).

I had a first implementation that was a pure addition, old code can still be found here: https://github.com/IceTDrinker/cbindgen/tree/saved/am/feat/specify-crate-version-v0

The first approach to a fix linked above is 100% backwards compatible by adding a new setting for the bindgen.

The current PR has a different approach validating the Cargo manifest path, issue here is I'm not sure it manages all edge cases with e.g. unresolved paths like relative paths. Canonicalizing paths could introduce panics that did not exist before, so I went for a middle ground which tries to validate the manifest path, and if it does not succeed falls back to the old behavior.

Would be happy to help or chat on the channel that is more convenient for you.

I'm unfortunately a bit time pressed and if the patch does not land in a release before January 19th we'll publish a crate which is cbindgen + that patch under a name that is distinct enough that it won't interfere with cbindgen on crates.io and will allow us to release

Cheers

IceTDrinker commented 4 months ago

Would be good to get a test for this...

Fully agree, if you have pointers on how to write them I'll take it, I was not quite sure how the test system worked