brson / home

Canonical definitions of home_dir, CARGO_HOME, and RUSTUP_HOME
Apache License 2.0
63 stars 13 forks source link

`home_dir` is lying about its functionality on Unix systems #22

Open tesuji opened 4 years ago

tesuji commented 4 years ago

home uses std::env::home_dir for non Windows systems and states that

https://github.com/brson/home/blob/3a6eccd53357624442eeb03f28abde1941dc433c/src/lib.rs#L42-L43

but after https://github.com/rust-lang/rust/pull/51656/files#diff-b596503c7c33ce457b6d047e351ac12bR516, Rust changes home_dir doc on Unixes.

There are two way to move forward now:

cc @brson for advice

brson commented 4 years ago

It's tricky.

It's not obvious that using an empty HOME is incorrect, given my reading of your posix definition link. If HOME is set, even to a poor value, I am not sure why we should second guess the environment and assume it is incorrect.

So it seems reasonable (technically) to use an empty HOME. On the other hand, this crate currently ignores an empty USERPROFILE. So at the least, unless there is official documentation saying otherwise, I think we should treat empty HOME and empty USERPROFILE the same - either obey it or ignore it.

The std considers an empty result from getpwuid_r to be valid, while dirs-sys ignores an empty result.

I can imagine use cases for setting HOME to empty, but they don't seem great. And the discussion on the PR really doesn't seem conclusive to me about the validity of empty home directories, just that the std implementation is confused enough to deprecate.

All that said, empty home directories seems like a potential source of errors, so I'm inclined to agree with the approach of dirs-sys and treat empty home directories as None. For example, I don't think cargo or rustup, the primary clients of this crate was made for, would ever want to deal with an empty value here.

The unix implementation of home_dir is almost trivial for all unixes, with only a single variation for those that don't support getpwuid_r:

https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/os.rs#L561

and in dirs-sys:

https://github.com/soc/dirs-sys-rs/blob/master/src/lib.rs#L31

Though note that

a) dirs-sys has a special treatment for redox b) vxworks appears to be a unix for which the "obey HOME but don't getpwuid_r" rule applies - it is just written differently in std.

Also note that this crate and dirs-sys have different code for the windows SHGet(Known)FolderPath call that I have not looked closely at:

https://github.com/brson/home/blob/master/src/windows.rs#L19

https://github.com/soc/dirs-sys-rs/blob/master/src/lib.rs#L153

It would probably be worth reviewing.

With the awkward maintainership situation in dirs-sys I am inclined to take on the correct code for unix home_dir here, but as you are the maintainer @lzutao you should make that decision. Since dirs-sys works today even in its unmaintained state I am fine with importing it as a dependency until it is determined to have a bug.

cc @tarcieri I've seen you have opinions about this crate

brson commented 4 years ago

Thanks for continuing to maintain this crate @lzutao

brson commented 4 years ago

If this crate made its own unix home_dir definition it would still need a final fallback path for non-unix, non-windows. Almost any solution seems fine: defering to std, returning None, or panicking / compiler-erroring. Other platform implementors can submit PRs.

tarcieri commented 4 years ago

I'd be fine with this crate using dirs-sys.

I will say one of the things that drew me to it in the first place was the small number of dependencies. Adding dirs-sys doesn't seem too bad though.

My main use case was finding home::cargo_home (for a cargo subcommand) and for that purpose this crate seems to be the best option.

tesuji commented 4 years ago

Thanks for thorough advice Brian. I may spend some time later to decide. Currently I and others in @xdgr-rs is talking about maintaining dirs crates and its friends.