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

home_dir is lying about its functionality on Unix systems #22

Open
tesuji opened this issue May 2, 2020 · 5 comments
Open

home_dir is lying about its functionality on Unix systems #22

tesuji opened this issue May 2, 2020 · 5 comments

Comments

@tesuji
Copy link
Contributor

tesuji commented May 2, 2020

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

home/src/lib.rs

Lines 42 to 43 in 3a6eccd

/// Returns the value of the `HOME` environment variable if it is set
/// and not equal to the empty string. Otherwise, it tries to determine the

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:

  • Correct documentation by copying std::env::home_dir doc.
    After all, POSIX requires systems to init "HOME" env after user loging: https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html

  • Re-implement for all UNIX targets, this is harmful for maintenance. Because unlike Windows,
    there are many Unix systems worth considering to support.

    • Use dirs-sys crate for implementation part.

cc @brson for advice

@tesuji tesuji changed the title home_dir is lying about its function on Unixes home_dir is lying about its function on Unix systems May 2, 2020
@tesuji tesuji changed the title home_dir is lying about its function on Unix systems home_dir is lying about its functionality on Unix systems May 2, 2020
@brson
Copy link
Owner

brson commented May 4, 2020

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
Copy link
Owner

brson commented May 4, 2020

Thanks for continuing to maintain this crate @lzutao

@brson
Copy link
Owner

brson commented May 4, 2020

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
Copy link

tarcieri commented May 4, 2020

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
Copy link
Contributor Author

tesuji commented May 6, 2020

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.

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

No branches or pull requests

3 participants