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.

Sign up for GitHub

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

Jump to bottom

Only load ~/.cargo/config for cargo install #6026

Merged
merged 1 commit into from
Sep 21, 2018
Merged

Only load ~/.cargo/config for cargo install #6026

merged 1 commit into from
Sep 21, 2018

Conversation

alexcrichton
Copy link
Member

This commit tweaks how configuration is loaded for cargo install, ensuring
that we only load configuration from $HOME instead of the current working
directory. This should make installations a little more consistent in that they
probably shouldn't cover project-local configuration but should respect global
configuration!

Closes #6025

@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member Author

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned matklad Sep 14, 2018
dwijnand
dwijnand approved these changes Sep 14, 2018
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM!

Had a look at the issue when it was opened so it's interesting to see how you solved this.

Is this going to be blocked on indiv0/lazycell#90 being reviewed, merged and released, I wonder?

@alexcrichton
Copy link
Member Author

Thanks! Yeah we'll need to wait for that PR to merge, but if it doesn't happen in the next few days I'll figure out another strategy

ehuss
ehuss approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Looks good once the lazycell release is resolved.

@Mark-Simulacrum
Copy link
Member

I would (loosely) expect that cargo install with a --path argument should use the cargo config from that path since you're probably installing a local package. I could see us warning about that (rare, I believe) case when we detect a local cargo config though.

(Generally it seems like I want an option to override this PR and say "no use this cargo config")

@alexcrichton
Copy link
Member Author

@Mark-Simulacrum that seems plausible to me too! I think I'll leave that for a future PR though :)

@dwijnand
Copy link
Member

I agree, best fix the regression in #6025 before stretching for Mark's use case, imo.

@alexcrichton
Only load ~/.cargo/config for cargo install
d9534bf 
This commit tweaks how configuration is loaded for `cargo install`, ensuring
that we only load configuration from `$HOME` instead of the current working
directory. This should make installations a little more consistent in that they
probably shouldn't cover project-local configuration but should respect global
configuration!

Closes rust-lang#6025
@alexcrichton
Copy link
Member Author

@bors: r=ehuss

@bors
Copy link
Contributor

bors commented Sep 21, 2018

📌 Commit d9534bf has been approved by ehuss

@bors
Copy link
Contributor

bors commented Sep 21, 2018

⌛ Testing commit d9534bf with merge 77e02e7...

bors added a commit that referenced this pull request Sep 21, 2018
@bors
Auto merge of #6026 - alexcrichton:install-config, r=ehuss
77e02e7 
Only load `~/.cargo/config` for `cargo install`

This commit tweaks how configuration is loaded for `cargo install`, ensuring
that we only load configuration from `$HOME` instead of the current working
directory. This should make installations a little more consistent in that they
probably shouldn't cover project-local configuration but should respect global
configuration!

Closes #6025
@bors
Copy link
Contributor

bors commented Sep 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 77e02e7 to master...

@bors bors merged commit d9534bf into rust-lang:master Sep 21, 2018
This was referenced Oct 4, 2018
Closed
Closed
@alexcrichton alexcrichton deleted the install-config branch October 12, 2018 17:23
@cuviper cuviper mentioned this pull request Dec 7, 2018
Closed
@Eh2406 Eh2406 mentioned this pull request Dec 27, 2018
Closed
@xelephas xelephas mentioned this pull request Dec 29, 2018
Closed
@ehuss ehuss mentioned this pull request Apr 1, 2019
Merged
bors added a commit that referenced this pull request Apr 1, 2019
@bors
Auto merge of #6804 - ehuss:install-path-config2, r=alexcrichton
527017f 
Allow `cargo install --path P` to load config from P.

`cargo install` was changed to ignore configs except for the home directory (#6026). However, it seems like there are legitimate needs when using `--path`, so allow loading from that path, too.

Closes #6498.
Closes #6397.
@anupdhml anupdhml mentioned this pull request May 14, 2020
Merged
@phil-opp phil-opp mentioned this pull request Jul 19, 2020
Closed
@phil-opp phil-opp mentioned this pull request Aug 23, 2020
Open
@ehuss ehuss added this to the 1.31.0 milestone Feb 6, 2022
@weihanglo weihanglo mentioned this pull request Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwijnand dwijnand dwijnand approved these changes

@ehuss ehuss ehuss approved these changes

Assignees

@ehuss ehuss

Labels
None yet
Projects
None yet
Milestone
1.31.0
Development

Successfully merging this pull request may close these issues.

cargo install should read $HOME/.cargo/config only
7 participants
@alexcrichton @rust-highfive @Mark-Simulacrum @dwijnand @bors @ehuss @matklad