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

Added documentation for the configuration discovery of cargo install to the man pages #11763

Merged
merged 1 commit into from
Feb 26, 2023

Conversation

jofas
Copy link
Contributor

@jofas jofas commented Feb 24, 2023

Fixes #11660.

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Command-uninstall S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've written something pleasing to read! Thank you! However, this is probably a breaking change for someone depending on old cargo uninstall behaviour. Could we split this PR into two, so we can approve cargo install part and discuss the other half afterward?

BTW, I think we may want to first warn the user who depends on the behaviour of implicit config discovery in cargo uninstall. After a transition period, we can then turn the warning into an error.

src/doc/man/cargo-install.md Show resolved Hide resolved
@jofas
Copy link
Contributor Author

jofas commented Feb 25, 2023

However, this is probably a breaking change for someone depending on old cargo uninstall behaviour. Could we split this PR into two, so we can approve cargo install part and discuss the other half afterward?

Of course. Never contributed to a project that can't make a breaking change, release a new version and call it a day 😅.

I'll split the breaking changes and make a new PR. Maybe we could use that PR to also tackle the fact that install starts configuration discovery at $CARGO_HOME/config.toml, whereas I think it would be more natural to anchor configuration discovery at $CARGO_HOME/config.toml, not start it, like you already pointed out in the issue:

// TODO: Consider calling set_search_stop_path(home).
config.reload_rooted_at(config.home().clone().into_path_unlocked())?;


BTW, I think we may want to first warn the user who depends on the behaviour of implicit config discovery in cargo uninstall. After a transition period, we can then turn the warning into an error.

Can you point me to a location in the source where cargo already has such a warning? Then I could create a PR for the warning as well (of course only if that is something you'd want).

@weihanglo
Copy link
Member

Can you point me to a location in the source where cargo already has such a warning? Then I could create a PR for the warning as well (of course only if that is something you'd want).

You'll find them by searching “hard error” or “transition period” in this repository, such as:

ws.config().shell().warn(&format!(
"license-file `{}` does not appear to exist{}.\n\
Please update the license-file setting in the manifest at `{}`\n\
This may become a hard error in the future.",
license_path.display(),
rel_msg,
pkg.manifest_path().display()
))?;

@jofas jofas changed the title cargo uninstall should ignore local configuration chain and added documentation for that behaviour to cargo install and cargo uninstall. Added documentation for that configuration discovery of cargo install to the man pages Feb 26, 2023
@jofas jofas changed the title Added documentation for that configuration discovery of cargo install to the man pages Added documentation for the configuration discovery of cargo install to the man pages Feb 26, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! Thanks for the update :)

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 26, 2023

📌 Commit eaf055f has been approved by weihanglo

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2023
@bors
Copy link
Collaborator

bors commented Feb 26, 2023

⌛ Testing commit eaf055f with merge e578c67...

bors added a commit that referenced this pull request Feb 26, 2023
Added documentation for the configuration discovery of `cargo install` to the man pages

Fixes #11660.
@weihanglo
Copy link
Member

BTW, @jofas could you squash these commits so that we won't have things added and removed making the history confusing? No problem if you're unable to do that.

@jofas
Copy link
Contributor Author

jofas commented Feb 26, 2023

Sure, I can squash the commits into a single one. Can you stop bors from merging?

@weihanglo
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2023
@weihanglo
Copy link
Member

BTW, if the bot detects more commits being pushed, it will cancel the merge automatically.

@jofas
Copy link
Contributor Author

jofas commented Feb 26, 2023

Thanks, good to know. I squashed the commits.

@weihanglo
Copy link
Member

Thanks a lot!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 26, 2023

📌 Commit e011901 has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 26, 2023
@bors
Copy link
Collaborator

bors commented Feb 26, 2023

⌛ Testing commit e011901 with merge 447ccb4...

@bors
Copy link
Collaborator

bors commented Feb 26, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 447ccb4 to master...

@bors bors merged commit 447ccb4 into rust-lang:master Feb 26, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 28, 2023
10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951
2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000

- bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767)
- Addition of support for -F as an alias for --features (rust-lang/cargo#11774)
- Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763)
- Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756)
- Fix warning with tempfile (rust-lang/cargo#11771)
- Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643)
- Fix tests with nondeterministic ordering (rust-lang/cargo#11766)
- Make some blocking tests non-blocking (rust-lang/cargo#11650)
- Suggest cargo add when installing library crate (rust-lang/cargo#11410)
- chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759)
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2023
Update cargo

10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951 2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000

- bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767)
- Addition of support for -F as an alias for --features (rust-lang/cargo#11774)
- Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763)
- Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756)
- Fix warning with tempfile (rust-lang/cargo#11771)
- Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643)
- Fix tests with nondeterministic ordering (rust-lang/cargo#11766)
- Make some blocking tests non-blocking (rust-lang/cargo#11650)
- Suggest cargo add when installing library crate (rust-lang/cargo#11410)
- chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759)

r? `@ghost`
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 Command-install S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo install does not respect .cargo/config.toml
5 participants