Add '-C' flag for changing current dir before build #10952

compenguy · 2022-08-08T22:23:59Z

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, 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 #11696 was created to track this.

rust-highfive · 2022-08-08T22:24:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

compenguy · 2022-08-08T22:28:13Z

The note about the trailing slash under "additional information" is in question. Subsequent retesting seems to suggest something else is going on. I am continuing to look into it.

epage · 2022-08-09T01:32:48Z

FYI I modified the following line in the PR description because it would cause github to close the issue when this PR is merged which we probably don't want to have happen

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

src/bin/cargo/cli.rs

src/cargo/util/config/mod.rs

src/bin/cargo/cli.rs

This will be a help for cases like rust-lang#10952 which I would expect would assert that the config is not loaded before changing the current_dir.

refactor(cli): Lazy load config This is trying to clarify `-C` support when it is implemented in #10952 Cargo currently has two initialization states for Config, `Config::default` (process start) and `config.configure` (after parsing args). The most help we provide for a developer touching this code is a giant `CAUTION` comment in one of the relevant functions. Currently, #10952 adds another configuration state in the middle where the `current_dir` has been set. The goal of this PR is to remove that third configuration state by - Lazy loading `Config::default` so it can be called after parsing `-C` - Allowing `-C` support to assert that the config isn't loaded yet to catch bugs with it The hope is this will make the intent of the code clearer and reduce the chance for bugs. In doing this, there are two intermediate refactorings - Make help behave like other subcommands - Before, we had hacks to intercept raw arguments and to intercept clap errors and assume what their intention was to be able to implement our help system. - This flips it around and makes help like any other subcommand, simplifying cargo initialization. - We had to upgrade clap because this exposed a bug where `Command::print_help` wasn't respecting `disable_colored_help(true)` - Delay fix's access to config Personally, I also find both changes make the intent of the code clearer. To review this, I recommend looking at the individual commits. As this is just refactors, this has no impact on testing.

bors · 2022-09-01T06:55:32Z

☔ The latest upstream changes (presumably #11029) made this pull request unmergeable. Please resolve the merge conflicts.

This will be a help for cases like rust-lang#10952 which I would expect would assert that the config is not loaded before changing the current_dir.

weihanglo · 2022-12-06T14:30:48Z

Ping @compenguy. Just checking in to see if you are still interested in working on this, or if you had any questions we could help with.

compenguy · 2022-12-06T16:34:02Z

Ping @compenguy. Just checking in to see if you are still interested in working on this, or if you had any questions we could help with.

I'm still on this. It's mostly held up on getting the cli tests implemented, which I'm not entirely clear how to get started on, but if my day job can cut me a break at some point I should be able to put in the time to figure it out.

weihanglo · 2022-12-08T15:21:11Z

Thanks for the response and take your time!

Ed already set up a good lazy load mechanism in #11029 for you to move on. So basically we could have a directory changed before Config being loaded. You may use this method to assert the Config has yet been loaded (it was removed anyway but can be restored).

For writing tests, you could follow here or existing tests. You could create a new place like tests/testsuite/change_directory.rs to put your tests. I have no preference to functional or UI tests, but functional tests might be easier because you'll need to assert things before and after changing the directory.

I'd suggest adding tests for at least the following scenarios:

Multiple nested .cargo/config.toml.
cargo check for build commands. Also its interaction with Cargo workspaces
cargo new for non-build commands.
cargo fix, as it has a special management of subprocesses.
cargo help <alias> to make sure aliases loaded correctly

You also need to add a doc for the new flag. You can find the doc of global options here. Note that every time you modify docs for cargo commands, you'll need to do this to rebuild manpages. I usually just run bash ./src/doc/build-man.sh from the project root of Cargo.

I am only suggesting, so please say it out loud if you have a better idea! And feel free to ask anything :)

epage · 2022-12-08T15:26:56Z

And in case this is a concern, most of our tests are calling into the cargo binary so you don't havee to worry about mucking with the current process's working directory.

FrankYang0529 · 2023-02-01T10:09:25Z

Hi @compenguy, just checking in to see if you are still working on this issue. If not, may I handle the remaining parts? Thank you.

weihanglo · 2023-02-10T15:55:15Z

Thank you!

@rfcbot resolved bikeshed-flag-name

rfcbot · 2023-02-10T15:55:22Z

🔔 This is now entering its final comment period, as per the review above. 🔔

arlosi

Thanks! Could you update the PR title and description to account for the change to only use -C?

epage · 2023-02-10T20:41:35Z

As the rfcbot was more used as a straw poll, this was pre-approved before the PR was made, and the flag situation has significant buy-in on zulip, I'm going to merge now rather than wait the 10 days

(I updated the title and description)

@bors r+

bors · 2023-02-10T20:41:37Z

📌 Commit 6510cc5 has been approved by epage

It is now in the queue for this repository.

bors · 2023-02-10T20:41:44Z

⌛ Testing commit 6510cc5 with merge 7404367...

bors · 2023-02-10T21:30:29Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 7404367 to master...

10 commits in 82c3bb79e3a19a5164e33819ef81bfc2c984bc56..39c13e67a5962466cc7253d41bc1099bbcb224c3 2023-02-04 22:52:16 +0000 to 2023-02-12 02:01:08 +0000 - chore: Update to toml v0.6, toml_edit v0.18 (rust-lang/cargo#11618) - doc: more doc comments and intra-doc links (rust-lang/cargo#11703) - Deny warnings in CI, not locally (rust-lang/cargo#11699) - add comment to lto.rs (rust-lang/cargo#11701) - Re-export cargo_new::NewProjectKind as public (rust-lang/cargo#11700) - Add '-C' flag for changing current dir before build (rust-lang/cargo#10952) - `-Zrustdoc-scrape-example` must fail with bad build script (rust-lang/cargo#11694) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11690) - Update 1password to the version 2 CLI (rust-lang/cargo#11692) - chore: autolabel more for `A-*` (rust-lang/cargo#11679)

Update cargo 10 commits in 82c3bb79e3a19a5164e33819ef81bfc2c984bc56..39c13e67a5962466cc7253d41bc1099bbcb224c3 2023-02-04 22:52:16 +0000 to 2023-02-12 02:01:08 +0000 - chore: Update to toml v0.6, toml_edit v0.18 (rust-lang/cargo#11618) - doc: more doc comments and intra-doc links (rust-lang/cargo#11703) - Deny warnings in CI, not locally (rust-lang/cargo#11699) - add comment to lto.rs (rust-lang/cargo#11701) - Re-export cargo_new::NewProjectKind as public (rust-lang/cargo#11700) - Add '-C' flag for changing current dir before build (rust-lang/cargo#10952) - `-Zrustdoc-scrape-example` must fail with bad build script (rust-lang/cargo#11694) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11690) - Update 1password to the version 2 CLI (rust-lang/cargo#11692) - chore: autolabel more for `A-*` (rust-lang/cargo#11679) r? `@ghost`

rust-highfive assigned ehuss Aug 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2022

epage reviewed Aug 9, 2022

View reviewed changes

epage mentioned this pull request Aug 11, 2022

Add flag that set working directory for cargo run #10974

Closed

compenguy force-pushed the cargo-change-dir-10098 branch from 85ebc79 to 5daf7a8 Compare August 16, 2022 15:29

epage added a commit to epage/cargo that referenced this pull request Aug 29, 2022

refactor(cli): Lazily do first-pass config loading

4e56636

This will be a help for cases like rust-lang#10952 which I would expect would assert that the config is not loaded before changing the current_dir.

This was referenced Aug 29, 2022

refactor(cli): Lazy load config #11029

Merged

cargo install ignores the rust-toolchain.toml or uses the wrong one #11036

Open

epage added a commit to epage/cargo that referenced this pull request Sep 1, 2022

refactor(cli): Lazily do first-pass config loading

eda1652

This will be a help for cases like rust-lang#10952 which I would expect would assert that the config is not loaded before changing the current_dir.

Muscraft mentioned this pull request Sep 13, 2022

Remove dead code #11080

Merged

epage added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2022

weihanglo mentioned this pull request Oct 16, 2022

Cargo overwrites project target when used as a library #11245

Closed

compenguy force-pushed the cargo-change-dir-10098 branch 5 times, most recently from d1956e4 to 7dea900 Compare February 1, 2023 16:42

compenguy added 2 commits February 10, 2023 07:42

Updating doc sources for new cargo option '-C'

5406faf

Regenerated documentation with updated '-C' cargo option

6510cc5

compenguy force-pushed the cargo-change-dir-10098 branch from b82d946 to 6510cc5 Compare February 10, 2023 15:44

rustbot added A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation labels Feb 10, 2023

rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Feb 10, 2023

arlosi approved these changes Feb 10, 2023

View reviewed changes

epage changed the title ~~Add '--directory,-C' flag for changing current dir before build~~ Add '-C' flag for changing current dir before build Feb 10, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 10, 2023

epage mentioned this pull request Feb 10, 2023

cargo -C should have a long name #11696

Open

bors merged commit 7404367 into rust-lang:master Feb 10, 2023

weihanglo mentioned this pull request Feb 13, 2023

Update cargo rust-lang/rust#107993

Merged

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Feb 20, 2023

ehuss added this to the 1.69.0 milestone Mar 9, 2023

mikeleany mentioned this pull request Apr 11, 2023

cargo -C dir does not use the rust-toolchain.toml file within the given directory. #11957

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add '-C' flag for changing current dir before build #10952

Add '-C' flag for changing current dir before build #10952

compenguy commented Aug 8, 2022 •

edited by epage

Loading

rust-highfive commented Aug 8, 2022

compenguy commented Aug 8, 2022

epage commented Aug 9, 2022

bors commented Sep 1, 2022

weihanglo commented Dec 6, 2022

compenguy commented Dec 6, 2022

weihanglo commented Dec 8, 2022

epage commented Dec 8, 2022

FrankYang0529 commented Feb 1, 2023

weihanglo commented Feb 10, 2023

rfcbot commented Feb 10, 2023

arlosi left a comment

epage commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

Add '-C' flag for changing current dir before build #10952

Add '-C' flag for changing current dir before build #10952

Conversation

compenguy commented Aug 8, 2022 • edited by epage Loading

How should we test and review this PR?

Additional information

rust-highfive commented Aug 8, 2022

compenguy commented Aug 8, 2022

epage commented Aug 9, 2022

bors commented Sep 1, 2022

weihanglo commented Dec 6, 2022

compenguy commented Dec 6, 2022

weihanglo commented Dec 8, 2022

epage commented Dec 8, 2022

FrankYang0529 commented Feb 1, 2023

weihanglo commented Feb 10, 2023

rfcbot commented Feb 10, 2023

arlosi left a comment

Choose a reason for hiding this comment

epage commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

compenguy commented Aug 8, 2022 •

edited by epage

Loading