rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.66k stars 2.4k forks source link

CARGO is not Cargo when build script is invoked through cargo used as a library #10119

Closed jonhoo closed 1 year ago

jonhoo commented 2 years ago

Problem

When a program using cargo as a library initiates a build, the CARGO environment variable to build scripts is set to be the path to the current executable, even though that command may look nothing like Cargo. This, in turn, means that build scripts invoked as part as such builds may fail if they try to use CARGO as though it were Cargo, like for example cbindgen does: https://github.com/eqrion/cbindgen/blob/93c06c5c9d319f481788c9670700097b4e46d270/src/bindgen/cargo/cargo_metadata.rs#L233-L235

Steps

#!/bin/bash

set -euo pipefail

rm -rf cargo-build-rs
mkdir cargo-build-rs
cd cargo-build-rs

cargo new cargo-something
pushd cargo-something
echo 'cargo = "0.57"' >> Cargo.toml
cat >src/main.rs <<EOF
use cargo::core::compiler::{BuildConfig, CompileMode};
use cargo::core::resolver::{CliFeatures};
use cargo::core::Workspace;
use cargo::ops::{CompileFilter, CompileOptions, Packages};
use cargo::Config;

fn main() {
  // This is the smallest working emulation of `cargo build` I could come up with.
  let mut config = Config::default().unwrap();
  config
    .configure(0, false, None, false, false, false, &None, &[], &[])
    .unwrap();
  let root = cargo::util::important_paths::find_root_manifest_for_wd(config.cwd()).unwrap();
  let ws = Workspace::new(&root, &config).unwrap();
  let build_config = BuildConfig::new(&config, None, &[], CompileMode::Build).unwrap();
  let cli_features = CliFeatures::from_command_line(&[], false, true).unwrap();
  let compile_opts = CompileOptions {
      build_config,
      cli_features,
      spec: Packages::from_flags(false, Vec::new(), Vec::new()).unwrap(),
      filter: CompileFilter::from_raw_arguments(
          false,
      Vec::new(),
          false,
          Vec::new(),
          false,
          Vec::new(),
          false,
          Vec::new(),
          false,
          false,
      ),
      target_rustdoc_args: None,
      target_rustc_args: None,
      local_rustdoc_args: None,
      rustdoc_document_private_items: false,
      honor_rust_version: true,
  };
  cargo::ops::compile(&ws, &compile_opts).unwrap();
}
EOF
cargo build
popd

cargo new --lib has-buildrs
cd has-buildrs
cat >build.rs <<EOF
fn main() {
    println!("{}", std::env::var("CARGO").unwrap());
    assert!(false);
}
EOF
env PATH="$PWD/../cargo-something/target/debug:$PATH" cargo something

Yields

~/cargo-build-rs/cargo-something/target/debug/cargo-something

Note that a command like $CARGO metadata (like cbindgen executes) would fail in this case, since cargo-something != cargo.

Possible Solution(s)

This is a tough one. In the particular case where the binary invoking a build through cargo-as-a-library is a cargo external subcommand, perhaps the initiating cargo invocation could set CARGO (by some other name) that then gets picked up transparently by the build logic. But in the more general case, it seems like this needs to be settable through cargo::Config somewhere so that library users at least have a way to correct CARGO if they do invoke a build.

Notes

No response

Version

cargo 1.56.0 (4ed5d137b 2021-10-04)
release: 1.56.0
commit-hash: 4ed5d137baff5eccf1bae5a7b2ae4b57efad4a7d
commit-date: 2021-10-04
jonhoo commented 2 years ago

It could be that one significant improvement here is to make the logic that sets CARGO should propagate an existing CARGO if it's already set. That way, if cargo-something is invoked as cargo something, the CARGO that the "outer" Cargo sets (which is truly Cargo) will carry through to the inner build script. It won't fix cases where Cargo isn't involved in the outer execution, but I think that's certainly the common case.

jonhoo commented 2 years ago

With https://github.com/rust-lang/cargo/pull/10115 closed (which I think is reasonable), I think it's even more important to figure out a solution to this, ideally using a single mechanism to address both cases.

jonhoo commented 2 years ago

To leave breadcrumbs to related issues, a very specific (but also slightly different) instantiation of this problem is https://github.com/rust-lang/cargo/issues/10113. Ideally a solution addresses both use-cases.

jonhoo commented 1 year ago

Also, just for convenience for readers of this issue, the place where Cargo discovers the current Cargo executable is https://github.com/rust-lang/cargo/blob/3ff044334f0567ce1481c78603aeee7211b91623/src/cargo/util/config/mod.rs#L406

It's used here for external authentication commands: https://github.com/rust-lang/cargo/blob/f955ddc9eb7309b64f8be8d1db1dbc7ed2ad5da9/src/cargo/ops/registry/auth.rs#L126

Here for build scripts: https://github.com/rust-lang/cargo/blob/1ac43cf64928cb27c96b28fc0e4e3d5789435f54/src/cargo/core/compiler/compilation.rs#L316

And here for fingerprinting: https://github.com/rust-lang/cargo/blob/25c5ced766396015f32ab89b0581876411756187/src/cargo/core/compiler/fingerprint.rs#L728

jonhoo commented 1 year ago

I also just realized that Cargo already sets CARGO when it invokes subcommands: https://github.com/rust-lang/cargo/blob/c71f344cbc801af944126806d2da76d5088b8ad5/src/bin/cargo/main.rs#L193-L195

So I think there's a reasonable argument for saying that Cargo should prefer re-using an existing value for CARGO over overwriting it with what current_exe returns.

jonhoo commented 1 year ago

I filed https://github.com/rust-lang/cargo/pull/11285 to fix this by forwarding CARGO if already set