cross 0.2.2 hangs forever when CARGO env var set to cross

[messense]

Checklist

• I've looked through the issues and pull requests for similar reports

Describe your issue

cross 0.2.1 works fine when executing with CARGO=cross, but 0.2.2 hangs forever.

What target(s) are you cross-compiling for?

aarch64-unknown-linux-gnu

Which operating system is the host (e.g computer cross is on) running?

• macOS
• Windows
• Linux / BSD
• other OS (specify in description)

What architecture is the host?

• x86_64 / AMD64
• arm32
• arm64 (including Mac M1)

What container engine is cross using?

• docker
• podman
• other container engine (specify in description)

cross version

cross 0.2.2

Example

0.2.2 broken: https://github.com/PyO3/setuptools-rust/runs/7051863085?check_suite_focus=true

0.2.1 works fine: https://github.com/PyO3/setuptools-rust/runs/7052877232?check_suite_focus=true

Additional information / notes

Local testing shows that it recursively executing cross metadata.

cross rustc --lib --manifest-path Cargo.toml --target aarch64-unknown-linux-gnu --release -v --features pyo3/extension-module -- --crate-type cdylib

[Emilgardis]

It's unclear how this should work. What cargo should cross try to invoke for metadata?

This was introduced as a fix in #808

I suppose we could try to detect this scenario and exit early or try to recover.

[messense]

I think you just need to remove CARGO before calling the underlying command.

I had fixed a similar issue in messense/cargo-options@52f8953

[Alexhuszagh]

I think you just need to remove CARGO before calling the underlying command.

I don't think this is correct.

I think we can have a better solution, since CARGO should be the one doing the building, but we currently don't seem to respect it either. If we do support CARGO correctly. We also support CARGO_HOME but I don't believe CARGO_TARGET_DIR, and we should probably also support RUSTUP_HOME.

We might want to actually correctly handle all of the above, and I can try adding testing for it:
https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads

[nickbabcock]

I believe setting CARGO=cross has become commonplace, and something that should be considered preserving.

I'm not sure where it originated from but I know core crates like regex use it, and as a temporary workaround, resorted to pinning to cross v0.2.1 (cc @BurntSushi, this is the cause of the issues you seem to be experiencing)

[Alexhuszagh]

I believe setting CARGO=cross has become commonplace, and something that should be considered preserving.

Right, I think there's a way to better handle this than how we currently do, since we should be able to detect at a higher level if we're using cross or not, and if so, fallback to cargo itself. But since we use cargo extensively for metadata, ignoring CARGO is also a bad idea. I don't think exiting is a good strategy, and absolutely breaking.

I'll work on this.

[Alexhuszagh]

This will take a little while since I want to support all of the following, or make it clear we do not support some of these flags:

• CARGO_HOME
• CARGO_TARGET_DIR
• RUSTC
• RUSTC_WRAPPER
• RUSTC_WORKSPACE_WRAPPER
• Should be auto-passed through
  • RUSTDOC
  • RUSTDOCFLAGS
  • RUSTFLAGS
  • CARGO_INCREMENTAL
  • CARGO_CACHE_RUSTC_INFO
  • HTTPS_PROXY
  • HTTP_TIMEOUT
  • TERM
  • BROWSER
  • RUSTFMT

And then also the following:
https://doc.rust-lang.org/cargo/reference/environment-variables.html#configuration-environment-variables

This will mean playing with multiple rustup installs on the same machine (I'll do it in Docker), so for a good solution, it won't be immediate. I'll try to handle as little as possible that we can, and just make sure everything else is passed through to the container.

It also seems that CARGO is set by cargo itself, but I think this is the one value we should respect, and only ignore it if we're using cross itself (we need to detect if the binary is cross even if we have two different installs of cross, probably in a cached variable, and then only use cargo if CARGO is set the cross.

We can probably break this into 2 PRs so we can get CARGO behavior handles correctly first, then everything else handled correctly afterwards.

[Alexhuszagh]

Ok cargo itself ignores any previous values and just sets CARGO, so we can safely ignore it. A simple test that shows this:

main.rs

use std::env;

fn main() {
    println!("env::var_os(\"CARGO\")={:?}", env::var_os("CARGO"));
    println!("env::var(\"CARGO\")={:?}", env::var("CARGO"));
}

Output

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/envvar`
env::var_os("CARGO")=Some("~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo")
env::var("CARGO")=Ok("~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo")

$ CARGO=cross cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/envvar`
env::var_os("CARGO")=Some("~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo")
env::var("CARGO")=Ok("~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo")

[lu-zero]

cross should have a CROSS_CARGO env var and then upon being called replace CARGO with CROSS_CARGO if CARGO is set to self maybe?

[Emilgardis]

What's the intended outcome for that?

[lu-zero]

being able to tell cross the right cargo to call while avoiding the problem mentioned in this issue :)

[Alexhuszagh]

I think respecting CARGO_HOME and expecting cargo to be on the path is sufficient: we do use the cargo provided in the sysroot. This way, we use the expected cargo of the toolchain, but also allow custom registries/credentials/config to be used.