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

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

Open
bennofs opened this issue Jul 29, 2016 · 11 comments
Open

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

bennofs opened this issue Jul 29, 2016 · 11 comments
Labels
A-configuration Area: cargo config files and env vars C-bug Category: bug S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@bennofs
Copy link
Contributor

bennofs commented Jul 29, 2016

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
Copy link

@benma
Copy link

benma commented Jun 28, 2020

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).

benma added a commit to benma/bitbox02-firmware that referenced this issue Jun 28, 2020
It was in the root before because .cargo/config would not be resolved:

rust-lang/cargo#2930

Changing the working directory fixes it.

Putting those files to src/rust is good to keep it close to the rust
code.

This also allows to add another Rust project in e.g. /tools, with a
different config.
iankronquist added a commit to iankronquist/cargo that referenced this issue Jul 9, 2021
This behavior is described in this bug:
rust-lang#2930

I think we should document limitations of the tool which exist at present if they do not bind our hands in the future.
If this bug is fixed, this note should be removed.
bors added a commit that referenced this issue Jul 15, 2021
Document cargo limitation w/ workspaces & configs

This behavior is described in this bug:
#2930

I think we should document limitations of the tool which exist at present if they do not bind our hands in the future.
If this bug is fixed, this note should be removed.
@duskmoon314
Copy link

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
Copy link
Contributor

dvdhrm commented May 27, 2022

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.

kwshi added a commit to USPhysicsTeam/2022-future-circular-collider that referenced this issue Jun 10, 2022
@HTGAzureX1212
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

epage commented Aug 4, 2022

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

@compenguy
Copy link
Contributor

@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
Copy link
Contributor

epage commented Aug 4, 2022

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).

compenguy pushed a commit to compenguy/cargo that referenced this issue Aug 8, 2022
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Aug 8, 2022
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
@epage
Copy link
Contributor

epage commented Aug 15, 2022

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)

compenguy pushed a commit to compenguy/cargo that referenced this issue Aug 16, 2022
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 1, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy pushed a commit to compenguy/cargo that referenced this issue Feb 2, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
compenguy added a commit to compenguy/cargo that referenced this issue Feb 10, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
bors added a commit that referenced this issue Feb 10, 2023
Add '-C' flag for changing current dir before build

This implements the suggestion in #10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to `--manifest-path` that resolves the issue described
in #2930.

The behavior of this new flag makes cargo build function exactly the same when run at the root of the project as if run elsewhere outside of the project.  This is in contrast to `--manifest-path`, which, for example, results in a different series of directories being searched for `.cargo/config.toml` between the two cases.

Fixes #10098
Reduces impact of #2930 for many, possibly all impacted, by switching to this new cli argument.

### How should we test and review this PR?

The easiest way to reproduce the issue described in #2930 is to create an invalid `.cargo/config.toml` file in the root of a cargo project, for example `!` as the contents of the file.  Running cargo with the current working directory underneath the root of that project will quickly fail with an error, showing that the config file was processed.  This is correct and expected behavior.

Running the the same build with the current working directory set outside of the project, e.g. /tmp, and passing the `--manifest-path /path/to/project/Cargo.toml`.  The build will proceed without erroring as a result of reading the project's `.cargo/config.toml`, because config file searching is done from cwd (e.g. in `/tmp` and `/`) without including the project root, which is a surprising result in the context of the [cargo config documentation](https://doc.rust-lang.org/cargo/reference/config.html), which suggests that a `.cargo/config.toml` file checked into the root of a project's revision control will be processed during the build of that project.

Finally to demonstrate that this PR results in the expected behavior, run cargo similar to the previous run, from /tmp or similar, but instead of `--manifest-path /path/to/project/Cargo.toml`, pass `-C/path/to/project` to cargo (note the missing `Cargo.toml` at the end).  The build will provide the exact same (expected error) behavior as when running it within the project root directory.

### Additional information

~Passing a path with a trailing '/' will result in failure.  It is unclear whether this is a result of improper input sanitization, or whether the config.cwd value is being improperly handled on output.  In either case this needs to be resolved before this PR is merge-ready.~

(the above issue appears to have been a side effect of local corruption of my rustup toolchain, and unrelated to this change)

Because a `struct Config` gets created before command line arguments are processed, a config will exist with the actual cwd recorded, and it must then be replaced with the new value after command line arguments are processed but before anything tries to use the stored cwd value or any other value derived from it for anything.  This change effectively creates a difficult-to-document requirement during cargo initialization regarding the order of events.  For example, should a setting stored in a config file discovered via cwd+ancestors search be wanted before argument processing happens, this could result in unpleasant surprises in the exact use case this feature is being added to fix.

A long flag was deferred out to not block this on deciding what to name it.  A follow up issue will be created.
Ethan-000 pushed a commit to Ethan-000/cargo that referenced this issue Feb 11, 2023
This implements the suggestion in rust-lang#10098 to make cargo change cwd before
completing config processing and starting the build. It is also an
alternative to --manifest-path that resolves the issue described
in rust-lang#2930.
@epage
Copy link
Contributor

epage commented Sep 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars C-bug Category: bug S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests

9 participants