-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Support platform-defined standard directories #5183
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
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Hi everyone, I would love to get some feedback on this (and I couldn't reach anyone at #cargo). |
New Examples:
|
src/cargo/util/config.rs
Outdated
// This is written in the most straight-forward way possible, because it is | ||
// hard as-is to understand all the different options, without trying to | ||
// save lines of code. | ||
pub fn cargo_dirs() -> CargoDirs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Config::cargo_dirs
be renamed/moved to CargoDirs::new
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, new
would looks slightly better I think, though current option is OK as well!
☔ The latest upstream changes (presumably #5176) made this pull request unmergeable. Please resolve the merge conflicts. |
cc @nrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great to me @soc!
One thing I am really worried about is how are we going to test this =/
- we want to test it across at least mac/windows/linux (and probably on windows, there's also msvc/mingw/sigwin/linux subsystem axis?)
- we want to test different fallback scenarios
- we want to test this in conjunction with rustup
All this together implies to me that plain #[test]
tests ain't gonna work here at all :(
I am thinking about a really heavy weight solution, like preparing docker images for different initial state of the machines, and then writing tests as bash scripts, which install rustup, create cargo project, etc. But, one does not simply create a docker image for windows I guess 🤷♂️ ?
It's also interesting that this PR actually does two things:
- it refactors Cargo to support
CargoDirs
instead of monolithic CARGO_HOME. - it changes default locations for stuff, using
directories
and environment variables.
I wonder if it makes sense to split this over two pull request, and implement a refactoring first, while preserving current behavior fully. That way, we can separately check that the refactoring does not introduce regressions by itself, and then maximally concentrate on the fallback bits.
@rust-lang/cargo
src/cargo/ops/cargo_install.rs
Outdated
@@ -122,7 +143,7 @@ pub fn install( | |||
if installed_anything { | |||
// Print a warning that if this directory isn't in PATH that they won't be | |||
// able to run these commands. | |||
let dst = metadata(opts.config, &root)?.parent().join("bin"); | |||
let dst = metadata(opts.config, &Filesystem::new(root.config_dir))?.parent().join("bin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be root.bin_dir
I guess?
src/cargo/util/config.rs
Outdated
// This is written in the most straight-forward way possible, because it is | ||
// hard as-is to understand all the different options, without trying to | ||
// save lines of code. | ||
pub fn cargo_dirs() -> CargoDirs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, new
would looks slightly better I think, though current option is OK as well!
src/cargo/util/config.rs
Outdated
let home_dir = ::home::home_dir().ok_or_else(|| { | ||
format_err!("Cargo couldn't find your home directory. \ | ||
This probably means that $HOME was not set.") | ||
}).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unwraps may panic in some obscure, yet real world scenario, so we really need proper error handlng. Changing the return type to CargoResult<CargoDirs>
and replacing .unwrap
s with ?
should do the trick I think?
As for an example of weird scenario, there were bug in Cargo about ::std::env::current_exe
call failing, because Cargo was executed in chroot without procfs :)
src/cargo/util/config.rs
Outdated
let mut cache_dir = cargo_dirs.cache_dir().to_path_buf(); | ||
let mut config_dir = cargo_dirs.config_dir().to_path_buf(); | ||
let mut data_dir = cargo_dirs.data_dir().to_path_buf(); | ||
// fixme: executable_dir only available on Linux, use data_dir on macOS and Windows? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be figured out in the RFC thread perhaps?
src/cargo/util/config.rs
Outdated
|
||
// 3. .cargo exists | ||
let legacy_cargo_dir = home_dir.join(".cargo"); | ||
if cargo_home_env.is_none() && legacy_cargo_dir.exists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be strictly a fall-back perhaps? That is, if all variables are not set, we set all dirs
from .cargo
, in contrast with the current per-dir approach?
src/cargo/util/config.rs
Outdated
for current in paths::ancestors(pwd) { | ||
let possible = current.join(".cargo").join("config"); | ||
for current in paths::ancestors(&dirs.current_dir) { | ||
let possible = current.join(".cargo").join("config"); // fixme: what to do about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to be done here? It's an explicit feture of Cargo that it looks for .cargo/config
in call parent directories. This exists for per-project .cargo
dirs. Or am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you are right. I just wanted to be extra careful by marking all changes where I wasn't absolutely sure.
@matklad Yes, testing is also a concern for me. I think as a first step it would really help to have a list of different configurations and scenarios, so it is at least possible to test everything in an organized fashion, even if it is done manually. I'm not sure whether splitting things into two commits makes sense, I feel that having an additional transitory state would have some benefits, but would also add overhead that would outweigh the benefits. Especially because a similar PR needs to be done for rustup. With changes in one commit we would only have to test 4 different setups, {cargo: pre-change, post-change} * {rustup pre-change, post-change}. With an additional state in the middle, this would balloon up to 9 different setups, for which all configurations and scenarios need to be tested against. |
@soc have you managed to prepare a similar PR for rustup as well? I think updating rusupt would be a next step here, because we really do want to land changes to rustup and cargo simultaneously :) |
@matklad Not yet, but here is my plan:
|
@@ -235,9 +256,9 @@ fn install_one( | |||
// We have to check this again afterwards, but may as well avoid building | |||
// anything if we're gonna throw it away anyway. | |||
{ | |||
let metadata = metadata(config, root)?; | |||
let metadata = metadata(config, &Filesystem::new(dirs.config_dir.clone()))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like metadata
could perhaps be a method of CargoInstallDirs
?
pub cache_dir: Filesystem, | ||
pub config_dir: Filesystem, | ||
pub data_dir: PathBuf, | ||
pub bin_dir: PathBuf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add short docstrings here, to explain which directory stores which data.
src/cargo/util/config.rs
Outdated
@@ -32,22 +33,119 @@ use util::Filesystem; | |||
|
|||
use self::ConfigValue as CV; | |||
|
|||
#[derive(Clone, Debug)] | |||
pub struct CargoDirs { | |||
pub home_dir: Filesystem, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually use this field I think?
src/cargo/util/config.rs
Outdated
let home_dir = ::home::home_dir().ok_or_else(|| { | ||
format_err!("Cargo couldn't find your home directory. \ | ||
This probably means that $HOME was not set.") | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looks like we don't use home_dir for anything except fallback, so let's move this as far down as possible, so that we don't actually fail if HOME
is not set, but explicit directories are! We might want to test this behavior as well: working without home directory is great for reproducible and isolated builds.
src/cargo/util/config.rs
Outdated
#[cfg(target_os = "macos")] | ||
let _bin_dir = cargo_dirs.data_dir().parent().map(|p| p.join("bin")); | ||
#[cfg(target_os = "windows")] | ||
let _bin_dir = cargo_dirs.data_dir().parent().map(|p| p.join("bin")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract this into a function
#[cfg(os = )]
fn bin_dir(dirs: &ProjectDirs) -> Option<PathBuf>
src/cargo/util/config.rs
Outdated
} else if let Some(val) = self.get_path("build.target-dir")? { | ||
let val = self.cwd.join(val.val); | ||
let val = self.dirs.current_dir.join(val.val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use self.cwd()
here for future-proofing.
let home_path = self.home_path.clone().into_path_unlocked(); | ||
let credentials = home_path.join("credentials"); | ||
let config_path = self.dirs.config_dir.clone().into_path_unlocked(); | ||
let credentials = config_path.join("credentials"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, and what is the "correct" place for credentials? config
might be not the right place, because people sometimes publish it... I suggest raising this question on the RFC thread, if it wasn't raised already.
@@ -638,7 +736,7 @@ impl Config { | |||
None => false, | |||
}; | |||
let path = if maybe_relative { | |||
self.cwd.join(tool_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.cws()
tests/testsuite/login.rs
Outdated
@@ -163,7 +163,7 @@ fn new_credentials_is_used_instead_old() { | |||
execs().with_status(0), | |||
); | |||
|
|||
let config = Config::new(Shell::new(), cargo_home(), cargo_home()); | |||
let config = Config::new(Shell::new(), CargoDirs::new().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we want to point CargoDirs
to CARGO_HOME
inside test root.
One option is to provide another constructor for CargoDirs which accepts cargo_home: PathBuf
. Another option is to modify the test such that it reads the config file directly. I am slightly in favor of the second approach.
tests/testsuite/cross_compile.rs
Outdated
@@ -910,7 +910,7 @@ fn build_script_needed_for_host_and_target() { | |||
); | |||
} | |||
|
|||
#[test] | |||
//#[test] | |||
fn build_deps_for_the_right_arch() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually passes for me locally
r? @matklad |
@matklad The tests are green now. Does this look fine to you? |
Moderation note: this is a blog post-length comment with a lot of technical detail interspersed with some unconstructive critiques. I'm keeping the content here for technical reference, but collapsing it by default. --@aturon So, I understand you are doing this. Fine. I encourage you to read all of this carefully. I know it's long. I'm not here to try to persuade you against this - you have made up your mind. All my previous and carefully considered arguments are on the RFC thread, as well as in private emails that some of you have access to. This thread contains speculation about how to modify rustup for this world, as well as code review, including what I see as bugs in this patch. I suggest you think very hard about how this interacts with rustup and the ecosystem before turning this on. So far all I see on the RFC is handwaving about making rustup conform too. It's not that easy. Sure you can land this as is - and effectively no cargo installation anywhere will use it, because rustup explicitly controls rustup must support all toolchains going back forever, and it must do seamless upgrades. I personally have at one point put a lot of thought into this RFC, particularly regards to rustup compatibility, and concluded that it was not at all worth the effort or complexity. Of course I've forgotten the details, but I'm sure one of you has a private email I sent outlining my thoughts. Here's a vague spitball outline of what has to happen in rustup to make this work. In the first pass I'm just going to assume the The easiest thing to do wrt to upgrading would be for upgraded existing rustup installs to just have rustup continue using the existing legacy paths after the upgrade (rustup controls all the relevant environment variables, including the ones in this patch) - they'll need some heuristic to decide to do this; leaving only new installs to deal with the new regime. You would need to take special account of non-self-upgrade (manual) installs that are overwriting legacy installs still. Not sure offhand what the differences are, so maybe there's just no way out of coding both fresh installs and upgrades. a thing to consider re rustup custom installation and env varsToday rustup uses But during upgrades, since rustup invokes rustup to do the installation, the installer does have to read and obey all the environment variables. In new-world that will include I'm not going to describe it yet, but assume that Furthermore, under the current RFC (more-or-less) So lets talk about the easy case, a fresh install of rustup. I'll discuss actually detecting the difference between a fresh install, a legacy upgrade, and a regular upgrade later. a rustup fresh install w/ platform-specific schemes
As far as I can tell all the Just as an aside, today it requires setting both dealing with legacy toolchainsThat all seems kinda easy so far, right? Yeah, well rustup has to also support every single stable, beta and nightly going back to 1.0, and none of those support anything but So you've still got to have rustup stash away a directory somewhere to represent Now, as to how rustup should differentiate when toolchains should be using So speaking of That's kinda all I can think of right now - not too difficult - as long as you consider my suggestion below about the precedence of It's worth noting that e.g. It's also worth noting that, under this proposal Of course the frequency that a "newcargo" toolchain interacts with an "oldcargo" toolchain may be essentially never, and this doesn't matter. Another thing to consider is that some outside force might set both Now the hard case, a seamless upgrade: a rustup upgradeYou'll need a way to detect a legacy vs. new layout - and you can't count on any Rust env vars being set, or necessarily meaning anything sensible - e.g. in the case someone manually downloads an upgrade, or trying to set only You'll need to keep the legacy home-dir discovery logic in-tree for detecting the upgrade. I don't have a full suggestion offhand. Probably the safest thing is to drop a marker file somewhere during new-world installation that means 'new layout'. Could also put it into rustups settings.toml, assuming you are doing the similar rustup upgrade at the same time and symlinking Ok, yeah, deciding if it's a legacy upgrade or a fresh-install/modern-upgrade might look like
Edit: OK, the more I think about it, the more a typical self-upgrade is going to look like If that's the case, then I think even a manual over-install that says nothing about Something like that... Actual install procedure:
oh, what about making the
|
Considering Rust only supports Windows 7 or newer as the host operating system, this is a non-issue. |
@brson Thanks for the notes, they provide a lot of hints at use cases that need to be carefully considered and sorted out. Some comments on the straight-forward parts (comments on other parts as I come around to them):
I don't have write-access to the RFC, so I can't keep it in sync with the changes implemented in this PR.
That's a good idea.
I'd probably just let them use .cargo forever and adjust the logic of what happens if both
This was probably lost in translation ... it was suggested to avoid computing the system paths until it is determined that they are needed, that's also why the code is more complicated than necessary.
Yes, that's an issue. Just got some suggestions for the
The name is automatically adjusted based on platform rules, and will be
Yes, that's the plan.
Yep, paying down technical debt is never easy. It would have been way less painful if the changes were made before 1.0, as people suggested, but that ship has sailed. :-/ Again, thanks a lot for all the information and helpful suggestions! |
Thanks for the responses @soc.
I thought the whole point of this exercise was to excise
As I mentioned, afaict there's a bug in the code where some of the paths can be uninitialized as written. |
I'm going to close this because it's been stale and quiet for quite some time now unfortunately. The Cargo team is pretty tied up until after the 2018 edition, but I think we can perhaps look to help out integrating this and fixing remaining issues after the edition release. |
Hi! Rust 2018 is here, it’s a fresh new year! Time to get this rolling 😄 |
Uh, how did I manage to unassign @matklad just by commenting‽ |
&& cargo_cache_env.is_none() | ||
&& cargo_config_env.is_none() | ||
&& cargo_data_env.is_none() | ||
&& cargo_bin_env.is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to handle the case where some but not all of the environment variables are set.
@soc Let's see if we can get this revived. I posted one comment for a corner case this doesn't seem to handle. Could you please update this to fix the conflicts, and ensure that it still passes tests? And could you please add tests for the various configuration cases (existing legacy configuration, existing XDG configuration, both, no existing configuration), to make sure they all work as expected? |
I don’t like the solution for macOS. See dirs-dev/directories-rs#47. |
@joshtriplett Sorry for the late reply. I think my changes so far are flawed in the sense that all the existing logic should be retained as-is, otherwise it becomes really really hard to make sure the current behavior is the same without flipping the hypothetical switch to the new structure. Sadly, at the moment I have little time for this (and the website shenanigans didn't help either). I can offer some advice, though: As a first step, identify each an every place in cargo, rustup, etc. that uses the current structure and add an explicit branch with a check for the proposed Then test that all tools reach the right branch when the flag is set/not set. Only at this point it makes sense to even start working on the new structure |
bin_dir = legacy_cargo_dir.join("bin"); | ||
// 4. ... otherwise follow platform conventions | ||
} else { | ||
let cargo_dirs = ProjectDirs::from("", "", "Cargo"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, in Miri we will likely use ProjectDirs::from("org", "rust-lang", "miri")
to get more descriptive names on platforms that commonly do that. Might be a good idea to do the same here?
I'm going to close this for now because it's been languishing for some time, but if someone is willing to take this up again and resubmit it the Cargo team would be interested in finding a reviewer for it! |
This commit is a continuation and adaptation of rust-lang#5183, which aimed to make cargo no longer reliant on the `$HOME/.cargo` directory in user's home's, and instead uses the `directories` crate to get platform-defined standard directories for data, caches, and configs. The priority of paths cargo will check is as follows: 1. Use `$CARGO_HOME`, if it is set 2. Use `$CARGO_CACHE_DIR`, `$CARGO_CONFIG_DIR`, etc, if they are set 3. If no environment variables are set, and `$HOME/.cargo` is present, use that 4. Finally, use the platform-default directory paths
Any progress on this? |
Any updates? |
Refs. - https://rust-lang.github.io/rustup/environment-variables.html - https://doc.rust-lang.org/cargo/reference/environment-variables.html See also: - https://wiki.archlinux.org/title/XDG_Base_Directory#Partial - rust-lang/rustup#247 - rust-lang/cargo#1734 - rust-lang/rfcs#1615 - rust-lang/cargo#5183 - rust-lang/cargo#148
For anyone interested, there is currently a pre-RFC in the forums: |
This change stops cargo from violating the operating system rules
regarding the placement of config, cache, ... directories on Linux,
macOS and Windows.
Existing directories and overrides are retained.
The precedence is as follows:
CARGO_HOME
environment variable if it exists (legacy)CARGO_CACHE_DIR
,CARGO_CONFIG_DIR
etc. env vars if they existA new cargo command,
dirs
, is added, which can provide pathinformation to other command line tools.
Fixes:
#1734
#1976
rust-lang/rust#12725
Addresses:
rust-lang/rfcs#1615
#148,
#3981