stop enabling dependencies in build.rs by default?

[cgwalters]

(I'm basically using issues as a communication mechanism here)

See coreos/cargo-vendor-filterer#71

TL;DR when rustix 0.37 came along with a2c6c7a this ended up enabling libc by default and that pulled in the errno dependency by default.

However, cargo metadata doesn't see this dependency, it's only enabled via build.rs.

And that ends up breaking cargo vendor-filterer.

Since in reality, libc is enabled by default, ISTM we can rework the features and build scripts here right?

[sunfishcode]

I'm confused about what's happening here. If I understand correctly, you're enabling the "use-libc" feature in your build. In rustix's Cargo.toml, it has:

use-libc = ["libc_errno", "libc"]

Enabling "use-libc" ought to pull in the "libc" dependency, without build.rs' help.

[cgwalters]

The project under discussion is https://github.com/coreos/coreos-installer/ which is not enabling use-libc - it actually doesn't even depend on rustix directly at all. (You are probably thinking of rpm-ostree which is another of our projects with does both of these)

I'm pretty sure (but not 100%) that the chain of events here is:

• a2c6c7a lands in rustix

Then tempfile updates to pull that in:

• Stebalien/tempfile@7c77c19

Then coreos-installer bumps to that version of tempfile:

• build(deps): bump tempfile from 3.4.0 to 3.5.0 coreos/coreos-installer#1156 happened

And because coreos-installer didn't (but does now) test a build from vendored sources we didn't detect that that PR broke the build.

I've reduced the failing case to

$ cat Cargo.toml
[package]
name = "rustix-vendorable"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

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

[dependencies]
errno = "0.2"
tempfile = "3.0.0"
$ cat src/main.rs 
fn main() {
    let e = errno::errno();
    dbg!(e);
$

And then:

$ rm target/vendor -rf; cargo vendor-filterer target/vendor; cargo check --config 'source.crates-io.replace-with="vv"' --config 'source.vv.directory="target/vendor"' --all-features
...
error[E0425]: cannot find function `errno` in crate `errno`
 --> src/main.rs:2:20
  |
2 |     let e = errno::errno();
  |                    ^^^^^ not found in `errno`

For more information about this error, try `rustc --explain E0425`.
error: could not compile `rustix-vendorable` due to previous error
$ 

Whereas if I revert to an earlier tempfile that uses rustix 0.36 (without the above linked "libc by default" PR):

$ cargo update -p tempfile --precise 3.4.0
    Updating crates.io index
    Updating linux-raw-sys v0.3.8 -> v0.1.4
    Updating redox_syscall v0.3.5 -> v0.2.16
    Updating rustix v0.37.19 -> v0.36.14
    Updating tempfile v3.5.0 -> v3.4.0
      Adding windows-sys v0.42.0
$

Then things build fine.

[cgwalters]

In a nutshell again, I think the problem is that rustix 0.37's default build (without any features set) has a dynamic dependency on errno because of how build.rs dynamically enables the libc feature.

And if we're going to depend on libc by default, then it seems to me we can just drop all the gyrations here in build.rs and make the dependency graph statically visible again (i.e. represent what cargo metadata sees).

[cgwalters]

Actually a required ingredient here too in this chain of events I think is

• errno introduces a new semver bump 0.3
• rustix switches over to it

Because if errno hadn't done an incompatible semver bump, then there would have only been one version in the dependency graph. (It's why errno is the problem and not libc - because there's only one version of that).

[cgwalters]

There's actually one more ingredient necessary for this failure to appear, which is that coreos-installer was using

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

Whereas some of our other projects include s390x-unknown-linux-gnu and because rustix's conditionals in Cargo.toml (i.e. the statically visible dependencies) always use libc on that platform, if the vendored set targets that, then we don't hit this issue.

It's really one of those "perfect storm" issues 😄

[sunfishcode]

In your reduced testcase, the error is:

error[E0425]: cannot find function errno in crate errno
--> src/main.rs:2:20
|
2 | let e = errno::errno();
| ^^^^^ not found in errno

This is odd, because it's not saying that the errno crate was found; it's saying that the errno function which should be defined in the errno crate is not found.

When I try that example (and fix the missing } in src/main.rs), I can reproduce the same error, but it looks like something's wrong with the vendored errno:

$ ls -l target/vendor/errno*/src/lib.rs
-rw-rw-r-- 1 sunfish sunfish    0 Jun  6 13:55 target/vendor/errno-0.2.8/src/lib.rs
-rw-rw-r-- 1 sunfish sunfish    0 Jun  6 13:55 target/vendor/errno-dragonfly/src/lib.rs
-rw-r--r-- 1 sunfish sunfish 3744 Jun  6 13:55 target/vendor/errno/src/lib.rs
$

errno-0.2.8's src/lib.rs is not empty, but the vendored file that I get here is.

The build says it's getting errno-0.2.8 here:

Vendoring errno v0.2.8 ([...]/.cargo/registry/src/index.crates.io-6f17d22bba15001f/errno-0.2.8) to target/vendor/errno-0.2.8

and the src/lib.rs in that directory is not empty. Is cargo-vendor-filterer deleting the contents of src/lib.rs?

[cgwalters]

Is cargo-vendor-filterer deleting the contents of src/lib.rs?

Yes, that's how cargo-vendor-filterer works. (And it does this because it believes the crate is unused)

See rust-lang/cargo#7058 (comment) where this approach was originally proposed a long time ago because it's possible without invasive changes to cargo.

[cgwalters]

Backing up a higher level; it's certainly within your rights to say that cargo-vendor-filterer should be invoking build.rs and in general honoring dynamic dependencies.

But two things:

• At this time, I am not aware of another crate (at least, not another important one) which is enabling dependencies dynamically in its default config
• It may be obvious, but trying to invoke build.rs would completely ruin the simplistic architecture of cargo-vendor-filterer because in the general case we'd actually need to be a whole "build system" including probably wrapping invocations of qemu etc.

[cgwalters]

I should clarify that I don't think this is any kind of urgent issue because IMO everyone using cargo-vendor-filterer and targeting Linux systems in general is almost certainly going to want to use the new tier = "2" filtering which happens to work around this today.,

But it's just a fragile fix because if rustix ever did the "no-libc" path for s390x it'd re-break.

[sunfishcode]

I'm open to making changes here, it's just that there are a lot of moving parts here, and I don't fully understand what's happening yet.

I don't think there are any dynamic dependencies here. What build.rs does is dynamically print "cargo:rustc-cfg=libc", which has the effect of adding --cfg=libc to RUSTFLAGS, however Cargo.toml doesn't use cfg(libc). Cargo.toml has its own (quite verbose) copy of the conditionals that determine whether the libc dependency should be used. So as far as I can tell, there's something else going on here.

[decathorpe]

There's actually one more ingredient necessary for this failure to appear, which is that coreos-installer was using

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

Whereas some of our other projects include s390x-unknown-linux-gnu and because rustix's conditionals in Cargo.toml (i.e. the statically visible dependencies) always use libc on that platform, if the vendored set targets that, then we don't hit this issue.

It's really one of those "perfect storm" issues 😄

I just came across this issue and I think I know what's going on.

I needed to deal with a very similar problem when I wrote the functionality that detects "foreign target" dependencies in our "rust2rpm" packaging tooling (and a bug in the first version of that function caused me to report #439, only to realize later that the bug was on my side).

If you're stripping dependencies from the vendor tarball based on logic that the target is x86_64-unknown-linux-gnu then you will break dependencies that are only enabled on other targets. There is nothing "dynamic" happening in build.rs, this is purely a result of target-specific dependencies in Cargo.toml.

Basically, you need the same logic that I ended up implementing in rust2rpm (i.e. dependencies MUST NOT be stripped if they are needed on any supported target platform). So this is a bug in cargo-vendor-filterer, not in rustix.

The updated code in "rust2rpm" that now produces correct results is here: https://pagure.io/fedora-rust/rust2rpm/blob/main/f/rust2rpm/cfg.py, and specifically, the "if cfg expression evaluates to True for any supported target platform" logic is at the very bottom of the file.

[cgwalters]

Hi @decathorpe thanks for commenting here. At a higher level there's some architectural overlap between rust2rpm and cargo-vendor-filterer and it may make sense to share code in the future. It feels to me like vendor-filterer is basically a more generally useful, less-tied-to-rpm tool and what may make sense in the future is to have rust2rpm e.g. parse metadata filtered through vendor-filterer or so.

If you're stripping dependencies from the vendor tarball based on logic that the target is x86_64-unknown-linux-gnu then you will break dependencies that are only enabled on other targets.

Indeed, this part is the bug in coreos-installer's config. And a bug which has now been fixed by coreos/coreos-installer@84efd15

In this case I'd say that cargo-vendor-filterer was doing exactly what it was asked to do. (But it's not the effect we wanted, we wanted again something compatible with the Linux architectures we care about).

I would guess that rust2rpm is always filtering dependencies to all of the Fedora-supported primary architectures, right? So if it had a bug, I think it'd be a different bug.

[decathorpe]

Yes, there's some amount of overlap, but the approach taken is different. IIUC cargo-vendor-filterer replaces crate sources with empty files, while rust2rpm patches Cargo.toml files (which is, IMO, the cleaner approach).

I would guess that rust2rpm is always filtering dependencies to all of the Fedora-supported primary architectures, right? So if it had a bug, I think it'd be a different bug.

That is the case now, but the original code had a very similar flaw (i.e. it only checked for compatibility with the host platform, not for any supported host platform, which resulted in a) architecture-specific patches, and b) wrong results if host platform != target platform).

These two patches fixed the issue in rust2rpm:

• https://pagure.io/fedora-rust/rust2rpm/c/d9aa58b6cba7add64b575f2fcb3dd386a741ad4b?branch=main
• https://pagure.io/fedora-rust/rust2rpm/c/d9aa58b6cba7add64b575f2fcb3dd386a741ad4b?branch=main

As far as I can tell, the second commit is basically equivalent to the change you linked for the cargo-vendor-filterer configuration in coreos-installer.

[cgwalters]

Hmmm...ah, wow I hadn't realized that rust2rpm has its own implementation of Cargo.toml evaluation! In vendor-filterer we just fork off cargo metadata --filter-platform and then unify the results. I'm not very excited about re-implementing the cargo evaluator (also if we were to do so, I'd certainly want it in Rust 😄 (why is rust2rpm in Python?)).

As far as I can tell, the second commit is basically equivalent to the change you linked for the cargo-vendor-filterer configuration in coreos-installer.

Hmmm...I'd say the effect is equivalent yes.

Anyways long story short I still feel there's something odd going on with the build.rs here, but the status quo is still true so there's no urgency.

[decathorpe]

Hmmm...ah, wow I hadn't realized that rust2rpm has its own implementation of Cargo.toml evaluation! In vendor-filterer we just fork off cargo metadata --filter-platform and then unify the results. I'm not very excited about re-implementing the cargo evaluator (also if we were to do so, I'd certainly want it in Rust 😄 (why is rust2rpm in Python?)).

No, we don't have our own evaluation. We use cargo metadata as well for reading Cargo.toml, and the functionality for patching Cargo.toml files is very limited. (I plan to make it better and more robust in the near future, and have early code for replacing the rudimentary patching code with something that's written in Rust. Side note: I don't know why rust2rpm was initially written in Python, but it certainly is easier this way - for example, to avoid bootstrap problems.)

Anyways long story short I still feel there's something odd going on with the build.rs here, but the status quo is still true so there's no urgency.

Nothing weird is going on in build.rs, as far as I can tell. All the logic for enabling and disabling dependencies lives in Cargo.toml. The only thing that build.rs script can do is to do the equivalent of passing --cfg arguments, but those only affect conditional compilation, not which dependencies are enabled.