-
Notifications
You must be signed in to change notification settings - Fork 375
Plan For Refactor: Ignore Extern Crates Completely #331
Description
In Rust 2018 you're idiomatically supposed to avoid using extern crate as much as possible (not always possible). This means cbindgen cannot rely on extern crate being present.
At the same time, even if extern crate is present, that name might be a rename that occured in the Cargo.toml, and so that name still won't show up in the Cargo.lock file (it only contains true package names, and not renames).
# true name is wr_malloc_size_of, but extern crates will show malloc_size_of
malloc_size_of = { version = "0.0.1", package = "wr_malloc_size_of" }Both of these issues point to it being desirable for us to ignore extern crates and to just dig through the Cargo.lock file, which should have everything we need to know about the crate graph already laid out for us.
A quick peek at the source indicates one potentially serious barrier to adopting this strategy universally, though: cfg collection. Currently, if an extern crate or mod is adorned with #[cfg(target_os = "linux")], we collect up this fact so that every item we emit into the .h file is contained within an appropriate #if.
In Rust 2018, these cfgs would be found in the Cargo.toml (and indeed many 2015 projects already do this in addition to the cfgs on extern crates to avoid downloading dead code):
[target.'cfg(target_os = "macos")'.dependencies]
core-foundation = "0.6"
core-graphics = "0.17.1"Sadly, we only ever parse the root Cargo.toml, and don't look at Cargo.toml's of children. (or rather: we call cargo metadata and parse that, which is just a nicer version of the Cargo.toml).
So a plan of action to do this well™️ would be to change to:
- Ignore extern crates completely
- Grab the dependency graph directly from the Cargo.lock, and use that to spider deps (bonus, avoids issues with renames or
-vs_). - Augment the dependency graph with the
metadata.packages.dependencies.targetfield of every crate in the dependency graph (optional?)
Problems:
-
This will break people who are relying on platform cfgs that only exist on
externs-- this is probably an acceptable breaking change, as they should want that information to be in their Cargo.toml anyway. -
There isn't a clear thing to do when there's a diamond in the dependency graph, where each path has different cfgs. This is potentially "fine" as it should be pretty rare, and we already blow up for way more plausible failure modes like "there is a name conflict anywhere in our crate graph because we completely ignore namespacing". I'm guessing the implementation will just end up using the cfgs of the first path it visits a crate through (probably what happens today).