-
Notifications
You must be signed in to change notification settings - Fork 987
Use home crate for CARGO_HOME and RUSTUP_HOME #1919
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
|
I think this is okay, but I'd like to think about it a little more before accepting it. It's very similar to a patch I wrote six months ago and then decided not to submit because I wasn't certain it had entirely the same semantics. |
|
I figured that there might be some concern about this, but it was easy enough to try out in any case. I'll compare the two and see if they are the same. They did look the same at first glance, like it was copied out of this project, but we should confirm. |
|
Thanks for doing that check. |
|
The unix semantics of dirs::home_dir() match the std::env::home function. The windows semantics match the utils::utils implemetation. Looks like the rustup_home implementation has a liitle more attached to it than the current one. After checking, I don't see much value in taking this change, and there is a (very small) risk of some semantic change if it's accepted. @kinnison: I'll leave this open so you can make a decision, but I no longer think it makes sense to accept this. If you decide against it, just go ahead and close it. |
|
The value in taking the change would be a reduction in complexity of code in rustup. What is the difference in code which you have detected? It's possible that it's no longer necessary. |
|
A fair point. I took another close look at it: Switching to the home crate adds support for finding CARGO_HOME/RUSTUP_HOME using pre-existing multirust directories. One notable exception is that Also, cargo home can be found under the incorrect |
|
Hrm, this definitely needs more thought before I can decide. I think I'll need a bit of time to think. |
|
☔ The latest upstream changes (presumably #1923) made this pull request unmergeable. Please resolve the merge conflicts. |
|
For context, I'm pretty sure that I still believe it is appropriate to move this logic out of rustup so that all tools can get the definition correct (e.g. IDE's might want to introspect Note though that |
|
@lzutao is now a maintainer of the home crate. |
|
Given @brson's explanation of the situation, and @lzutao's agreement to help with crate maintenance, I am indeed happy for this to be cleaned up for merge. Please do resolve the conflict (remove the .lock change from your commit, rebase onto master, then add the .lock change in a fresh commit) and I'll give things a final gloss check. |
Since the "home" crate provides the cross-platform functionality of the "dirs" crate, but doesn't provide CARGO_HOME or RUSTUP_HOME helpers, move to using the "home" crate instead.
|
@kinnison Resolved. CI error looks to be unrelated to my change, and passes on my ubuntu machine using |
Cargo uses the home crate to determine CARGO_HOME. Rustup could use this crate for HOME, CARGO_HOME, and RUSTUP_HOME.
Fixes #1915