rust-lang / cargo

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

Cargo does not always search for .cargo/config file in project root #2930

Open bennofs opened 8 years ago

bennofs commented 8 years ago

Currently, the search path for .cargo/config files is dependent on the current working directory at the time when cargo is executed. I find this behavior very surprising: I would expect that cargo at least always reads the .cargo/config file at the root of the project it builds.

It is important to note though that for some applications, this is probably the right thing to do (for example, cargo new does not have any associated project root to work with).

Examples
$ cargo build --manifest-path foo/Cargo.toml # will not read foo/.cargo/config
$ cargo build -p foo # in a workspace with foo, will not read foo's .cargo/config
Test case demonstrating the problem
#[test]
fn search_config_relative_to_project_root() {
    let foo = project("foo")
        .file("Cargo.toml", r#"
            [package]
            name = "foo"
            authors = []
            version = "0.0.0"
            build = "build.rs"
        "#)
        .file("src/lib.rs", "")
        .file("build.rs", r#"
            use std::env;
            fn main() {
                if env::var("NUM_JOBS").unwrap() == "5673" {
                    panic!("Config value `jobs` taken from config file located in working directory \
                            and not from the config file at foo's project root.");
                }

                if env::var("NUM_JOBS").unwrap() != "2423" {
                    panic!("Config value `jobs` not taken from config file located at foo's project root \
                            but config file in working directory was not read either.");
                }
            }
        "#)
        .file(".cargo/config", r#"
          [build]
          jobs = 2423
        "#);

    let bar = project("bar")
        .file(".cargo/config", r#"
          [build]
          jobs = 5673
        "#);

    foo.build();
    assert_that(bar.cargo_process("build")
                   .arg("--manifest-path").arg(foo.root().join("Cargo.toml")),
                execs().with_status(0));
}
eberkund commented 6 years ago

Possibly related: https://forum.snapcraft.io/t/bug-rust-plugin-not-using-correct-pkg-config-path/7302/2

benma commented 4 years ago

I agree, the config resolution should not be based on the current working directory, but starting at the project (where Cargo.toml is, i.e. manifest-path).

duskmoon314 commented 3 years ago

Is there any progress? I wonder how can I organize more than one crate in a directory having same dependencies but different config.toml ?

It seems that now I can only manage their cargo.toml and config.toml by hand.

dvdhrm commented 2 years ago

Changing this will break backwards-compatibility. I work with projects that rely on this behavior to easily inject configuration into cargo without modifying the source directory (by simply invoking cargo from a temporary directory with a custom configuration there). This is very similar to injecting configuration values via environment variables, but works even for those options that have no environment-equivalent (e.g., the [source.*] group).

Note that this use-case could become obsolete with a stable --config <key>=<value> option, but it is still unstable as of today.

HTGAzureX1212 commented 2 years ago

I have just encountered this issue when trying to change the output directory for one specific binary crate as a part of a large project. Telling users to specify --out-dir=out seems too much of a hassle to the users in a sense that a simple cargo build -p binary_crate should be sufficient enough and cargo should be able to read the configuration file for that specific binary crate to compile...

compenguy commented 2 years ago

Changing this will break backwards-compatibility. I work with projects that rely on this behavior to easily inject configuration into cargo without modifying the source directory (by simply invoking cargo from a temporary directory with a custom configuration there). This is very similar to injecting configuration values via environment variables, but works even for those options that have no environment-equivalent (e.g., the [source.*] group).

Note that this use-case could become obsolete with a stable --config <key>=<value> option, but it is still unstable as of today.

The backwards-compatibility behavior is broken for every client of bitbake, preventing them from easily injecting configuration into cargo by modifying the source tree, a use case documented by cargo to work, but does not. Your ability to do what you're doing is actually contrary to the cargo config documentation. At least as documented, it looks more like you're dependent on a bug.

The meta-rust layer for OpenEmbedded, used very widely in industry for building software for embedded systems, invokes cargo from a directory not in the source tree, passes the --manifest-path flag pointing to the source tree, and sets an environment variable to direct the build output to a directory outside of the source tree. This is a standard model for build systems, and is used in a wide range of ecosystems, but behaves contrary to the documented behavior in the case of Cargo because of the unusual (for a build system) insistence on only searching cwd.

As a way forward, I can see adding support for the -C flag (as in #10098) to tell cargo to change working directory to the specified directory before starting the build. As mentioned in that issue, it's a common convention, it explicitly means "change the called process's cwd to the specified directory", and it wouldn't disrupt anyone currently depending on the --manifest-path behavior.

epage commented 2 years ago

@compenguy would bitbake be able to take advantage of cargo --config <path> which looks like it'll be released in 1.63?

compenguy commented 2 years ago

@compenguy would bitbake be able to take advantage of cargo --config <path> which looks like it'll be released in 1.63?

It's a really weird fit. The meta-build system (in this case meta-rust+bitbake) shouldn't need to know or care if the fetched source for build has a .cargo/config.toml or not. So does it need to add detection and then conditionalize the inclusion of the --config flag? Or is it safe to specify the --config flag, but point it to a path that may or may not actually have the file? Will/should cargo guarantee non-failing behavior in the case that one doesn't actually exist there? I'd suggest probably not.

So if --config is the solution that cargo wants to put forward, it can work, and it's not like it's onerous, per se, for a meta-build system to conditionally check for the existing of .cargo/config.toml and conditionally add the requisite --config flag. But what's the cue for a meta-build system that that's even a consideration in the first place? What makes them aware of the need when implementing cargo integration? There's a high degree of surprise here, and it's a bit brittle. After all, notice I said to check for .cargo/config.toml. For this to work correctly, they actually have to check for either .cargo/config.toml or .cargo/config.

The '-C' flag solution proposed in #10098 is probably the best match for (meta)build systems' expectations, it eliminates all the surprises I mentioned, it's a common convention, and it's straightforward to document.

epage commented 2 years ago

If it doesn't work, thats ok. I wasn't sure how the system was put together, if --config could be used as a workaround until another solution comes along (e.g. if a tool was orchestrating cargo calls directly it could do its own lookups and pass --config as needed).

epage commented 2 years ago

Quoting @ehuss from #10974

As for the config file discovery issue itself, I think we should continue to scrutinize the use cases that require config files. If there are common tasks that are not environment-specific, those should likely be pushed to Cargo.toml or a build script. For example, one use case that some people wanted package-specific config files was to set the target which is now covered by per-package-target.

(since I've not seen this mentioned in this issue)

epage commented 1 year ago

As an update, we now have #12738 for tracking finding ways of supporting config-based workflows with packages.

Due to the compatibility concerns, I'm not sure what more there is we can do for this. I'm proposing to the cargo team that we close this.