build-sys: Drop cxxbridge-cmd out of build-dependencies

[cgwalters]

xref coreos/cargo-vendor-filterer#111

This was part of an attempt to avoid lockfile skew (which has bit me) but right now cargo says this is actually an invalid dependency:

$ cargo metadata --format-version=1
warning: cxx v1.0.136 (/var/home/walters/src/github/dtolnay/cxx) ignoring invalid dependency `cxxbridge-cmd` which is missing a lib target

Yet the problem appears to be that other times cargo doesn't ignore it.

[cgwalters]

Although maybe we could add a dummy/stub lib target to that?

[cgwalters]

Or maybe reverting 58cd415 is the right thing?

[dtolnay]

That warning by itself is not indicative of a problem to me. I would like to better understand how this ends up leading to the failure described in your link. Are you able to share any insight about that? What part of what cargo metadata is emitting to you is causing anything to break?

[cgwalters]

What part of what cargo metadata is emitting to you is causing anything to break?

The goal of cargo-vendor-filterer is to allow people who e.g. only want to build software for Linux to omit vendoring e.g. Windows dependencies. It uses cargo metadata --filter-platform=x86_64-unknown-linux-gnu (etc) to do that.

It seems that cargo metadata omits this from the list as invalid, but cargo build still tries to load it - and will fail, because the way cargo vendor-filterer works is to replace each omitted package with an empty stub.

[dtolnay]

I guess I am confused what the difference is between this "platform-specific" dependency and any other platform-specific dependency. Like if we replaced the 'cfg(any())' on this line with 'cfg(windows)' then cargo vendor-filterer would handle it correctly? Surely not.

[dtolnay]

I reproduced coreos/cargo-vendor-filterer#111 by running cargo vendor-filterer on the following crate:

[dependencies]
cxx = "1"

[package.metadata.vendor-filter]
platforms = ["x86_64-unknown-linux-gnu"]

and I observe that it's generating these two stub crates (among others):

# vendor/windows-sys/Cargo.toml

[lib]
doctest = false
name = "windows_sys"
path = "src/lib.rs"
test = false

[package]
autobenches = false
autobins = false
autoexamples = false
autotests = false

# vendor/cxxbridge-cmd/Cargo.toml

[package]
autobenches = false
autobins = false
autoexamples = false
autolib = false
autotests = false

That seems like something to fix in cargo-vendor-filterer. As part of generating the empty stub src/lib.rs, it could make sure to put a [lib] entry in the vendored manifest if there isn't one.

The vendor/cxxbridge-cmd/Cargo.toml vendored instead by cargo vendor does correctly contain a [[bin]]. But cargo-vendor-filterer delegates to cargo vendor right? (Based on what I see in the command's output.)

Can you please look into why the [[bin]] written by cargo vendor is disappearing? Is cargo-vendor-filterer reading and rewriting these manifests incorrectly?

[cgwalters]

That seems like something to fix in cargo-vendor-filterer. As part of generating the empty stub src/lib.rs, it could make sure to put a [lib] entry in the vendored manifest if there isn't one.

We could, but that doesn't feel to me like a clean fix. Couldn't cxxbridge-cmd equally well have a [lib] in its original manifest, even if it's a dummy stub? That'd I think also fix this.

Can you please look into why the [[bin]] written by cargo vendor is disappearing? Is cargo-vendor-filterer reading and rewriting these manifests incorrectly?

That's part of the explicit list https://github.com/coreos/cargo-vendor-filterer/blob/c6bff717af03fac35c4b82fc9fc54dac3b6bb0f0/src/lib.rs#L29

It's important to go back to the start and understand how vendor-filterer works: It was spawned by this comment from a cargo maintainer: rust-lang/cargo#7058 (comment)

Now, for sure I would say that the generated manifest here is broken, because we removed the bin, and even though we wrote an empty src/lib.rs, there isn't a [lib] section (and we turned off autodiscovery).

I for sure could easily generate a [lib] section and perhaps should do so just to be compatible with older versions of cxx. But before I do that...well, I just updated this PR to add one to cxxbridge-cmd. WDYT?

[cgwalters]

Can you please look into why the [[bin]] written by cargo vendor is disappearing? Is cargo-vendor-filterer reading and rewriting these manifests incorrectly?

This said, I guess we could change the logic for generating the stub crate to be: - If the project has a [lib] section, then make that a stub. If it has a [bin], then make that the stub. That would not be hard, and would be a targeted fix for this.

Still though, the more I think about this the more I feel like this is at heart a cargo bug.
Either what cxx is doing right now is valid (and hence cargo-metadata should stop warning, and it should include the package) or cargo build should error out too.

[cgwalters]

Ahh there's another wrinkle in this, which is cxxbridge-cmd sets autolib = false, which makes sense, but almost every other crate doing that that I think is either explicitly setting [lib], or is filtered out (and not depended on in any way).

Yes, cargo-vendor-filterer should clearly be setting [lib] in this case I'd say.

[cgwalters]

Yes, cargo-vendor-filterer should clearly be setting [lib] in this case I'd say.

➡️ coreos/cargo-vendor-filterer#114

Which does fix this, so there's less urgency to merge a fix on this side. That said, I do still think it probably makes sense to a fix either here or in cargo (possibly both) because we still have the core issue that cargo metadata and cargo build disagree.

[dtolnay]

I prefer for the issue to be fixed in cargo-vendor-filterer.

You'll need that for artifact dependencies anyway. https://doc.rust-lang.org/1.84.0/cargo/reference/unstable.html#artifact-dependencies

[cgwalters]

OK. But do you have an opinion on whether there's a cargo bug here?

A different angle to look at this from is: what does it mean to have a build-dependency on a bin crate? It doesn't look like cargo actually compiles it.

Oh...now I see, you used this [target.'cfg(any())'.build-dependencies] trick to make cargo build in cxx ignore it - if I move it directly under [build-dependencies] I get the same warning from cargo build.

But surely this is effectively working around a cargo bug?

You'll need that for artifact dependencies anyway. https://doc.rust-lang.org/1.84.0/cargo/reference/unstable.html#artifact-dependencies

Hum, this one is interesting. So in theory maybe what we want here is some sort of "synthetic only" dependency to basically express "if this crate is in the build graph, it must match this version constraint" (but if not, ignore it) right?

[dependencies]
cxxbridge-cmd = { version = "=1.0", artifact = "version-match-only" }

?

[dtolnay]

Cargo's behavior is fine in my opinion. I don't think adding a new kind of dependency, distinct from an unconditionally-disabled platform-specific dependency, would be justified. And once the artifact dependencies functionality is stable we'll switch to that.