Skip to content

Only fetch dependencies for current platform by default#676

Merged
emilio merged 8 commits into
mozilla:masterfrom
jonhoo:metadata-for-target
Apr 12, 2021
Merged

Only fetch dependencies for current platform by default#676
emilio merged 8 commits into
mozilla:masterfrom
jonhoo:metadata-for-target

Conversation

@jonhoo

@jonhoo jonhoo commented Apr 6, 2021

Copy link
Copy Markdown

cbindgen invokes cargo metadata to find the sources of dependencies so
that it can generate bindings for types that originate in those
dependencies. However, cargo metadata defaults to fetching
dependencies for all platforms unless it is specifically invoked with
the --filter-platforms argument.

This PR makes cbindgen pass that argument to cargo metadata using the
host platform, which will significantly reduce the time of the first
call in cases where the dependency tree includes many target-specific
dependencies.

Fixes #675.

Jon Gjengset added 2 commits April 6, 2021 11:35
cbindgen invokes `cargo metadata` to find the sources of dependencies so
that it can generate bindings for types that originate in those
dependencies. However, `cargo metadata` defaults to fetching
dependencies for _all_ platforms unless it is specifically invoked with
the `--filter-platforms` argument.

This PR makes cbindgen pass that argument to `cargo metadata` using the
host platform, which will significantly reduce the time of the first
call in cases where the dependency tree includes many target-specific
dependencies.

Fixes mozilla#675.
@jonhoo

jonhoo commented Apr 6, 2021

Copy link
Copy Markdown
Author

The CI failure is due to a new clippy lint (clippy::wrong-self-convention) — PR incoming for that.

@jonhoo

jonhoo commented Apr 6, 2021

Copy link
Copy Markdown
Author

Fix in #677.

@emilio emilio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm unsure about which one should be the default tbh. It seems reasonable to add cfg conversions and them just work... Plus I'm not in love that this breaks for pretty much all cross-compilation use cases by default...k

That plus the fact that not fetching all dependencies is technically a breaking change makes me err more on the side of inverting the flag.

Actually, does cargo read CARGOFLAGS internally? If so, this is the same as `CARGOFLAGS="--filter-platform " cbindgen, isn't it? If it isn't, I think a more generic solution is just to allow passing flags to cargo, one way or another... wdyt?

Comment thread src/bindgen/cargo/cargo_metadata.rs Outdated
log::debug!("Discovering host platform by {:?}", rustc);
if let Ok(out) = rustc {
if let Ok(stdout) = std::str::from_utf8(&out.stdout) {
let field = "host: ";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this also seems pretty odd of a default... Thinking a bit more about this, shouldn't this default to fetch the target dependencies?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow? From what I can tell, cbindgen is never passed a target platform, so the only reasonable thing to use here is the host platform (which is what this code does). Or rather, it uses the host field of rustc -vV for whatever rustc cbindgen is told to use.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, my point is that cbindgen doesn't have target information, but using the host also isn't the right thing if you run cbindgen as part of the build.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're imagining that someone runs, say, cargo build --target xyz, and then build.rs invokes cbindgen, and it ends up using the host target even though xyz was explicitly passed in?

It looks like we can use some of the environment variables cargo sets for build scripts here. Specifically, the CARGO_CFG_ variables. I'll give that a stab!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, or better yet, it also just sets TARGET.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at b5abdf6 — I think that does what you want!

@jonhoo

jonhoo commented Apr 6, 2021

Copy link
Copy Markdown
Author

Yeah, neither option is great here. If the default is to not fetch all dependencies, then cross-compilation breaks (if types from target-specific dependencies are used). If the default is to fetch all dependencies, then builds that do not use target-specific dependencies are slowed down due to the additional downloads and scanning. I think I agree with you that it's better to not break by default than to be slow by default though, so swapped the default in 02d8bfd.

No, cargo doesn't read CARGOFLAGS. I don't think that's what we'd want anyway, since this is specifically just a flag to cargo metadata — all other cargo commands default to only operating on the current platform. So, for example, if you call cargo rustc, it won't fetch dependencies for other targets. It's really just cargo metadata that has an inverted default compared to the other tools. So for example when we call cargo rustc here, that is only building for the current target platform:
https://github.com/eqrion/cbindgen/blob/54dfcf89e419afbf60f123088462ef91edd85916/src/bindgen/cargo/cargo_expand.rs#L93

@jonhoo

jonhoo commented Apr 9, 2021

Copy link
Copy Markdown
Author

Gentle nudge on this @emilio — I think it now works the way you wanted it to?

@emilio emilio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks sensible, thanks!

@emilio emilio merged commit 93c06c5 into mozilla:master Apr 12, 2021
emilio added a commit that referenced this pull request Apr 12, 2021
@jonhoo

jonhoo commented Apr 12, 2021

Copy link
Copy Markdown
Author

Happy to help! For what it's worth, I do think flipping the default on the next breaking change might be worthwhile — it's probably better to not automatically fetch more dependencies than usual by default, and then have crates that need cross-compilation opt into it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Only use metadata for current target

2 participants