cargo

cargo copied to clipboard

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 --directory /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.

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.

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.

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.

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

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.

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.

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

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.

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.

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.

I appreciate the offer, but I finally got some free time, got caught up on what's changed since I last touched this, and finally found what I think is the right entrypoint for the last missing piece - tests. So I might have this in hand now.

I remember us talking about this during a cargo team meeting but can't find a reference to it, so going ahead and doing a quick poll

@rfcbot merge

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• [ ] @Eh2406
• [x] @ehuss
• [x] @epage
• [ ] @joshtriplett
• [x] @weihanglo

Concerns:

• ~~bikeshed-flag-name~~ resolved by https://github.com/rust-lang/cargo/pull/10952#issuecomment-1426009513

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot concerns bikeshed-flag-name

#10098 mentions other tools like make and git choosing -C as the short form of this flag. However, only make adopts the long form --directory. Randomly googling some other tools, go CLI only have the short -C, whereas mecurial uses --cwd.

Personally I am happy with -C but not sure about the usefulness of the long form --directory.

* Do we want to support multiple `-C` specified as how `git` and `make` do? I have no idea if that is a useful behaviour.

It seems to be that it would be pretty safe and non-breaking to add that later if it came up as being useful. I can't think of a way that adding that later would adversely impact any existing users.

* As mentioned [here](https://github.com/rust-lang/cargo/pull/10952#issuecomment-1342893986), could you document this new flag? Could also do in a follow-up PR.

Now that I think about it, it probably would have been better to update the docs in a follow-up PR, because it adds a lot of noise to the diff, but I tried to keep the commits tidy and separated so filtering by commit is still pretty clean. At the moment, the first commit adds the new behavior, the second ensures the invariant (adding the assert!()), the third changes the doc template, and the very last commit is all the automated changes to the docs.

@rfcbot concerns bikeshed-flag-name

#10098 mentions other tools like make and git choosing -C as the short form of this flag. However, only make adopts the long form --directory. Randomly googling some other tools, go CLI only have the short -C, whereas mecurial uses --cwd.

Personally I am happy with -C but not sure about the usefulness of the long form --directory.

I personally don't care too much what the long form is: --directory is fine, as is --cd. I'd also be fine with this being short-only.

Am not a fan of short flag names, especially if there is no long-form version.

I would be happy with --directory.

I don't have a strong opinion about the long name. --cwd or --cd seems more explicit as to what it does, and would lean slightly towards that, but other options seem fine. -C seems to be the most common for a short flag.

A quick survey:

• make: -C or --directory
• git: -C
• Meson: -C
• ninja: -C
• hg: --cwd
• Gradle: -p, --project-dir?
• Julia: --project?
• Swift: -working-directory?
• dub: --root
• msbuild: passed as first argument
• npm: n/a
• Ivy: n/a?
• Maven: n/a?
• Bazel: n/a?
• Buck: n/a?
• go: -C
• nuget: n/a?
• sbt: n/a?
• dart: -C or --directory

Other command line tools with similar flags:

• tar: -C or --directory (BSD tar has --cd)
• ls: -d or --directory to list directories
• rm: -d or --dir to remove directories

--cwd/--cd sounds better to me for its explicitness (and way shorter). Although --directory is also widely used in make and dart, it seems a bit common in other different scenarios regarding directories. Just trying to avoid the same situation like https://github.com/rust-lang/cargo/issues/11554. If people feel good with --directory, I can go with that as well.

We've talked about the flag name a bit on Zulip. Still have yet reached a consensus on the long flag name. @epage gave a good suggestion that the majority of the Cargo team and others thumb it up:

-C alone for now, deferring any bike shedding to an issue as -C alone has enough precedence and some progress is better than no progress

To move forward, @compenguy, could you remove the long flag --directory and keep -C only?

Thank you!

@rfcbot resolved bikeshed-flag-name

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

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+

:pushpin: Commit 6510cc5c4bbf2fc9c0daf3288d2510ac2366fc4f has been approved by epage

It is now in the queue for this repository.

:hourglass: Testing commit 6510cc5c4bbf2fc9c0daf3288d2510ac2366fc4f with merge 74043673e6824c09daf8fdf3cad9681914f4283e...

:sunny: Test successful - checks-actions Approved by: epage Pushing 74043673e6824c09daf8fdf3cad9681914f4283e to master...