Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build probes confused by cargo always setting TARGET, even for build scripts of host dependencies #11138

Closed
RalfJung opened this issue Sep 23, 2022 · 20 comments
Labels
A-build-scripts Area: build.rs scripts

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 23, 2022

I just spent a about an hour debugging a build failure in the rustc workspace that boils down to this issue:

  • the anyhow build script uses a build probe to determine if it can use std::backtracke::Backtrace.
  • if the TARGET env var is set, it runs the build probe with --target passed to rustc, meaning this is conceptually a target build (this is the right thing to do, it is needed to make build probes work in cross-compilation settings when anyhow is used by the target code)
  • however, since anyhow is a dependency of a build dependency, we actually need to know if the build probe would succeed on the host

In the case of bootstrap, the target sysroot has the backtrace feature stable but the host (beta) sysroot does not. So this means that anyhow enables the backtrace feature and later fails to use the backtrace feature.

I think it is a cargo bug that the TARGET env var is set for a build script that does not run on the target. Cargo should unset that env var for host build script builds.

@RalfJung
Copy link
Member Author

This seems related to #5755, but there it sounds like a feature request to provide more information -- I think cargo is actively providing false information right now by setting that TARGET env var when this is in reality a build for the host.

@weihanglo
Copy link
Member

Sorry I had a hard time reproducing this issue. Could you tell me which command you ran failed?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 8, 2022

I ran x.py test src/tools/miri --stage 0 in rustc, in the master branch from around the time the issue was reported.

To reproduce, you'll need a proc-macro that has a dependency that has a build script.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 8, 2022

Here's an example project. I would expect the proc macro build script to not have a TARGET env var, since it always is a host build. cargo build should therefore works. But it currently fails, showing that the TARGET env var is present.

The real problem isn't the proc macro build script itself, but build scripts in proc macro dependencies -- those can't know that they are actually being built for the host.

Though to be fair, I realized the TARGET variable is set to the host architecture even for cross-builds. In fact TARGET is set even for non-cross regular builds. However there are situations where "host and target" have the same architecture but still need to be distinguished, in particular for rustc bootstrapping (where "host" is the downloaded beta toolchain and "target" is the newly built compiler). Usually cargo (maybe by accident) supplies enough information to make that work: target crates are built with rustc --target (explicitly indicating the host architecture, so the flag is a NOP for rustc, but it is present and interpreted by the rustc wrappers that bootstrap injects), host crates do not get a --target parameter, therefore bootstrap knows which toolchain to set. But this information is lost when you add build scripts to the mix, since the TARGET env var is always set. Miri uses a similar technique to distinguish between crates that need to be built and run on the host and crates that need to be interpreted: the target string is the same in both cases, but the mere presence of --target lets Miri distinguish "host" and "target".

This is definitely kind of a hack, but AFAIK cargo provides no alternative way to do this kind of bootstraping.

@bjorn3
Copy link
Member

bjorn3 commented Oct 8, 2022

However there are situations where "host and target" have the same architecture but still need to be distinguished, in particular for rustc bootstrapping

I think it would make more sense to add a new env var for this usecase.

@RalfJung
Copy link
Member Author

The problem with using a new env var is that then we'll also have to adjust anyhow, autocfg, and many other crates that have logic for "build a test file with rustc to see if we can use some feature".

@weihanglo
Copy link
Member

Back to the original issue. Did you mean that TARGET of anyhow shows a wrong specified --target info? It is expected to show host info as it will be run on host, right?

Here is a repro example. I cannot reproduce. Not sure what I missed. 😞

@jschwe
Copy link
Contributor

jschwe commented Oct 14, 2022

Back to the original issue. Did you mean that TARGET of anyhow shows a wrong specified --target info? It is expected to show host info as it will be run on host, right?

@weihanglo The original issue RalfJung raised was that TARGET should not be set at all for a build script.

I think it is a cargo bug that the TARGET env var is set for a build script that does not run on the target. Cargo should unset that env var for host build script builds.

@weihanglo
Copy link
Member

@jschwe That's a proposal they gave, not the problem they encountered.

I think it is a cargo bug that the TARGET env var is set for a build script that does not run on the target.

I interpret it as: TARGET set on anyhow should be TARGET=<HOST> but they got TARGET=<from --target>, no?

@bjorn3
Copy link
Member

bjorn3 commented Oct 14, 2022

What is currently happening is that build scripts will get a TARGET set to whatever target the associated crate will be built for. What @RalfJung want is that it is unset if the associated crate is built for the host. This would be a breaking change. For example abi-cafe depends on it always being set to access the triple used for building abi-cafe to run rustc at runtime with the same triple as target.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 16, 2022

I interpret it as: TARGET set on anyhow should be TARGET=<HOST> but they got TARGET=<from --target>, no?

No. TARGET on anyhow should be unset.

That is needed to make the existing logic work, anyway. If TARGET needs to be always set then we need to first provide some other way to convey this information from cargo to the build script (that is #5755) and then we need to change all the crates out there that run a 'probe build' to see if they can use some feature. :/ (anyhow, autocfg, thiserror, eyre -- we'll never be able to identify them all)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 16, 2022

@bjorn3 TARGET would be always set for the 'target' part of the build (which I think is the whole build when --target is not set, since cargo then does not even properly distinguish build and runtime deps AFAIK?), so this is only a breaking change if abi-cafe is ever used as a build-dependency or from a proc macro (and someone cross-compiles that entire thing). Which I doubt is the intended usecase for that crate anyway. So I don't think that is actually a breaking change here.

@bjorn3
Copy link
Member

bjorn3 commented Oct 16, 2022

I'm not sure cargo distinguishes betweeb host and target deps in any way other than need to/need not to pass --target to rustc. When you don't pass --target, cargo doesn't pass it to rustc either. As such abi-cafe would be considered a host crate by that logic and thus not get TARGET. Another thing is that the same compiled library can be used both as build script and as target dep if --target is used. Should the build script get TARGET set in that case or not? Both options are wrong for one of the usages of the compiled crate.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 16, 2022

Cargo definitely makes a distinction that it does not expose to build scripts -- even with cargo build --target $HOST, it passes --target to some rustc invocations and not to others. That distinction is absolutely crucial for rustc bootstraping and for Miri. The issue is that cargo does not forward enough information to build scripts that would let them create a rustc invocation that matches the one cargo uses.

Maybe the better approach would be to have cargo set an env var like CARGO_ENCODED_RUSTC that tells the build script how cargo will invoke rustc to build the crate this build script is for. That should set all the RUSTFLAGS, take into account RUSTC and RUSTC_WRAPPER, and also have --target if and only if cargo will later set that flag for rustc, so that the build script can perfectly match what cargo does. (It's an 'encoded' env var since this is conceptually an array of strings that needs to be encoded into a single string.)

Another thing is that the same compiled library can be used both as build script and as target dep if --target is used.

AFAIK it gets compiled twice then, for two different targets, so which TARGET value does the build script see? This question already arises in current cargo. (I would expect the build script to be run twice.)

@bjorn3
Copy link
Member

bjorn3 commented Oct 16, 2022

Another thing is that the same compiled library can be used both as build script and as target dep if --target is used.

AFAIK it gets compiled twice then, for two different targets, so which TARGET value does the build script see? This question already arises in current cargo. (I would expect the build script to be run twice.)

I meant if --target is not used.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 16, 2022

Honestly I don't care what it does then, the cargo consumers that care about this always set --target because they rely on this distinction cargo only makes when that flag is set.

That's why I had a parenthetical above, saying if --target is not set we can consider the entire build to be a target-build and always set the TARGET env var (for backcompat reasons).

@bjorn3
Copy link
Member

bjorn3 commented Oct 16, 2022

Another case that will break if TARGET is unset is when the cc crate is used in the build script of a build script dependency. The cc crate requires the TARGET env var to be set.

@RalfJung
Copy link
Member Author

I think I anyway prefer the solution of having cargo supply the rustc invocation, rather than each and every build script having to re-implement the same logic.

So closing this in favor of #11244.

@RalfJung RalfJung closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
@thomcc
Copy link
Member

thomcc commented Jan 8, 2023

What is currently happening is that build scripts will get a TARGET set to whatever target the associated crate will be built for. What @RalfJung want is that it is unset if the associated crate is built for the host. This would be a breaking change.

Yeah, just want to say: failing to provide TARGET to build scripts in any situation would cause huge ecosystem breakage. A ton of code, including many very widely used crates, just unwrap it directly. The ones that don't unwrap will usually behave incorrectly still.

And so on.

(This isn't to say it's not a problem and that things couldn't be improved -- and even several of these cases will behave incorrectly in multi-level build-script situations similar to the one described initially... That said, just I wanted to note that the breakage would be very, very bad if cargo stopped providing that var).

@RalfJung RalfJung changed the title Cargo sets TARGET env var for host-only build scripts build probes confused by cargo always setting TARGET, even for build scripts of host dependencies Aug 15, 2023
@RalfJung
Copy link
Member Author

Yeah, just want to say: failing to provide TARGET to build scripts in any situation would cause huge ecosystem breakage.

Okay, that's fair. But not providing build scripts with crucial information about the crate this is built for is also causing breakage -- just in subtle ways that take hours to debug.

Turns out this was already pointed out many years ago, but sadly the issue is still open: #5755.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts
Projects
None yet
Development

No branches or pull requests

6 participants