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

Use platform-defined directories for cargo state #8063

Closed
wants to merge 1 commit into from

Conversation

spacekookie
Copy link
Member

@spacekookie spacekookie commented Apr 1, 2020

This commit is a partial continuation and adaptation of #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

Notes

This is a work-in-progress PR, and there's some stuff missing/ not
working. Also generally I think I'd like some review on the code
because it's touching on a lot of things that I don't really know/
understand.

  1. Cargo panics on this assertion, and I don't understand why.
  2. Do we want more debug output as to which directories were chosen?
    Maybe a subcommand similar to what the original PR introduced?

Thanks! :)

cc for review: @joshtriplett, @alexcrichton

@rust-highfive
Copy link

r? @Eh2406

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2020
@spacekookie spacekookie force-pushed the xdg-directories branch 2 times, most recently from 32304c0 to 07f687b Compare April 1, 2020 15:22
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
@alexcrichton
Copy link
Member

Thanks for the PR! Unfortuantely the Cargo team is pretty strapped right now and we don't have a ton of spare cycles to help implement this feature. We can try to help out where we can, but we may not be super available :(

I'm not entirely sure what's going on with that assertion, and I unfortunately don't have a ton of time right now to dig in.

@ehuss
Copy link
Contributor

ehuss commented Apr 10, 2020

In addition to what Alex mentioned, I think it would be good to restart the RFC. It's not really clear to me (without reading hundreds of comments) where things stand. Per rust-lang/rfcs#1615 (comment), I think someone would need to review the feedback and possibly start an new RFC incorporating all the feedback. Like, one example, it looks like there is some disagreement on how macOS should behave. It's also not clear what the rollout story would be with rustup.

It may be fine to work on a prototype in tandem. This will need to be gated on nightly somehow.

@ehuss ehuss removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2020
@spacekookie
Copy link
Member Author

spacekookie commented Apr 25, 2020

That sounds like a reasonable idea. I'd be happy to take a look. Anyone able to co-author a new RFC with me? Especially because I don't have a lot of context on previous discussions.

@lnicola
Copy link
Member

lnicola commented Apr 29, 2020

Sorry for the random remark, but the directories repository seems to have been archived (also). That should probably be clarified before merging this PR -- maybe @soc can chime in?

By the way, thank you @spacekookie and @soc for pushing for this.

@lnicola
Copy link
Member

lnicola commented Apr 29, 2020

@spacekookie I think there's an issue in CargoDirs::new. The home_dir parameter comes from https://github.com/brson/home/blob/master/src/lib.rs#L110 and is, by default, ~/.cargo. On https://github.com/rust-lang/cargo/pull/8063/files#diff-d9c2f30b83a2efad1fde5427833cfd37R51, however, you add another .cargo so on my system I end up with:

statx(AT_FDCWD, "/home/me/.cargo/.cargo", AT_STATX_SYNC_AS_STAT, STATX_ALL, 0x7ffc3db07a50) = -1 ENOENT (No such file or directory)

and cargo thinks the legacy path doesn't exist. It works fine when CARGO_HOME is set.

Regarding the assertion, I've seen it fire once, but I can't reproduce it, so I'm not sure if it's related or not.

@lnicola
Copy link
Member

lnicola commented Apr 29, 2020

Okay, I got the assertion to fire -- it needs a project with an external crate.

It looks like this:

ret: /home/me/.local/share/cargo/registry/index/github.com-1ecc6299db9ec823
self.dirs.cache_dir: /home/me/.cache/cargo
thread 'main' panicked at 'assertion failed: ret.starts_with(self.dirs.cache_dir.as_path_unlocked())', src/cargo/util/config/mod.rs:1205:9

So the assertion here is that ret should be a subdirectory of the cache dir. I'm not sure what's the index directory (it looks like a clone of the index, but has no files except for a hidden .cache), but either:

  • it's a cache kind of thing, so it should end up under ~/.cache
  • it's not, so the assertion is incorrect

@spacekookie
Copy link
Member Author

Sorry for the random remark, but the directories repository seems to have been archived

I guess one could easily consider a project like directories "done", but yea, I hadn't seen the archiving of the repo yet

By the way, thank you @spacekookie and @soc for pushing for this.

No problem, one day I will be able to make my ~ read-only and all the tools will use xdg_paths instead. Until then, gotta keep patching 😉

Thanks for your investigation into some of the cache issues. I will try to take a look at them on the weekend.

I guess the most pressing issue would be to figure out a way for legacy versions to be updated, and how to coordinate all the tools to keep working (what would break, how do we stop it from breaking)

@bors
Copy link
Collaborator

bors commented May 30, 2020

☔ The latest upstream changes (presumably #8287) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label May 30, 2020
@joshtriplett
Copy link
Member

I would still love to see this happen. Given the age of this PR, and the current conflicts, we felt that it makes sense to close this PR for now. We would still be happy to see a fresh PR implementing this (which may be based on this one)

Please don't take this in any way as discouragement from working on it; quite the contrary. We're just trying to keep the list of open PRs manageable.

@ehuss
Copy link
Contributor

ehuss commented Jan 13, 2021

Closing for now, since this has been inactive for a long while. As Josh mentioned, we're still interested in this, though we don't have a lot of bandwidth to provide much assistance on figuring out all the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants