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 install should read $HOME/.cargo/config only #6025

Closed
traviscross opened this issue Sep 14, 2018 · 6 comments · Fixed by #6026
Closed

cargo install should read $HOME/.cargo/config only #6025

traviscross opened this issue Sep 14, 2018 · 6 comments · Fixed by #6026

Comments

@traviscross
Copy link

The cargo install command should read configuration from $HOME/.cargo/config but not from other configuration files.

Normally cargo looks for configuration in $PWD/.cargo/config, $PWD/../.cargo/config, etc., and in $HOME/.cargo/config. For cargo install, however, the tree walking behavior is surprising. cargo install foo should work the same regardless of the directory from which we run it.

This surprising behavior resulted in #5850 and PR #5874. That PR causes cargo install to ignore any configured build.target value from a configuration file.

However, that change does both too little and too much. It does too much because we don't want to ignore configured values in $HOME/.cargo/config. Values set there make sense for cargo install. E.g., we might use that to set musl as our default build target. On the other hand, the change does too little because we want to ignore all configuration values from $PWD/(../)*/.cargo/config, not just build.target.

The correct behavior is that cargo install should load only $HOME/.cargo/config and should apply all values found there.

cc @alexcrichton @japaric

@dwijnand
Copy link
Member

dwijnand commented Sep 14, 2018

The walking up the filesystem and looking in ~/.cargo happens in:

cargo/src/cargo/util/config.rs

Lines 1538 to 1567 in a5d8294

fn walk_tree<F>(pwd: &Path, mut walk: F) -> CargoResult<()>
where
F: FnMut(&Path) -> CargoResult<()>,
{
let mut stash: HashSet<PathBuf> = HashSet::new();
for current in paths::ancestors(pwd) {
let possible = current.join(".cargo").join("config");
if fs::metadata(&possible).is_ok() {
walk(&possible)?;
stash.insert(possible);
}
}
// Once we're done, also be sure to walk the home directory even if it's not
// in our history to be sure we pick up that standard location for
// information.
let home = homedir(pwd).ok_or_else(|| {
format_err!(
"Cargo couldn't find your home directory. \
This probably means that $HOME was not set."
)
})?;
let config = home.join("config");
if !stash.contains(&config) && fs::metadata(&config).is_ok() {
walk(&config)?;
}
Ok(())
}

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Sep 21, 2018
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
bors added a commit that referenced this issue Sep 21, 2018
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
@marcbowes
Copy link
Contributor

How can you run cargo install with a source-replacement for crates.io after this change?

@alexcrichton
Copy link
Member

@marcbowes I believe you should be able to configure via env vars or place configuration in $HOME

@marcbowes
Copy link
Contributor

@alexcrichton the env vars don't allow you to do source replacement. https://doc.rust-lang.org/cargo/reference/config.html#environment-variables

Environment variables will take precedent over TOML configuration, and currently only integer, boolean, and string keys are supported to be defined by environment variables. This means that source replacement, which is expressed by tables, cannot be configured through environment variables.

Moving configuration in $HOME is a global thing, i.e. it can create a mess that needs to be cleaned up, can be difficult to actually do (merge, undo) and prevents parallel installs with different config (e.g. registries).

Moving $HOME to a different folder temporarily breaks rustup. That can be worked around too, of course.

A much simpler option would be to have the config load from $CARGO_HOME as well as $HOME. Then I can just change $CARGO_HOME (and already do).

Alternatively, can we have an option or env var to switch out the root config?

@ehuss
Copy link
Contributor

ehuss commented Oct 4, 2018

Would the change recommended in #6026 (comment) work? It sounds like a good idea to me.

@marcbowes
Copy link
Contributor

Not for the use-case I have in mind.

At my company, we want to control our use of 3P packages (e.g. for licensing reasons). We mirror crates.io internally and use .cargo/config to source-replace registries:

[source.crates-io]
replace-with = 'local-registry'

[source.local-registry]
local-registry = 'PATH-TO/cargo-registry'

In addition to removing some third-party packages, this registry also includes first-party packages.

Now, we want to be able to use cargo install against that registry. Before the changes in this issue, we could place the .cargo/config file in the cwd and run cargo install. Now, we can't.

The comment you refer to would help in that we could fetch the package, tar xf it, then put in the config package and run install with --path. Which is what we sort of do, except it short-circuits most of what install buys you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants