feat: allow users to specify a crate version for bindings generation #901
Conversation
IceTDrinker
force-pushed
the
am/feat/specify-crate-version
branch
from
9ee152c
to
4877deb
Compare
|
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 :| |
|
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) |
|
@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
force-pushed
the
am/feat/specify-crate-version
branch
2 times, most recently
from
3cb23c9
to
3092504
Compare
- the old code was selecting the first package that was found which meant that dependencies with the same name could be used for bindgen instead of the intended package - a fallback to the old behavior is kept for now in case manifest paths are not matching because of relative paths for example - this should be backwards compatible with all cbindgen usage, the selection bug may still trigger in case the manifest path does not match
IceTDrinker
force-pushed
the
am/feat/specify-crate-version
branch
from
3092504
to
17a4414
Compare
|
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 |
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 |
this is a PR that attemps to solve #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.0to 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 enoughTo 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