Conversation
rustbot
added
A-environment-variables
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Isn't the MSRV high enough now that we can just drop our own implementations and instead use the std versions? |
|
The implementations are slightly different, SHGetKnownFolderPath vs GetUserProfileDirectoryW, unsure if that matters whatsoever for this use case, but can definitely just remove all of the extra code and call std if you want that. |
|
So long as they have the same behavior, which I believe was the intention of the recent |
|
Ok, I just removed the windows implementation entirely, but kept the windows specific unit test just to confirm that the std implementation behavior matches, which I confirmed under wine. |
|
Can we remove platform-specific logic completely? Could you have your commits reflect how this should be reviewed and merged, rather than how it was developed? |
The std implementation of std::env::home_dir uses the same logic as home's windows specific implementation, so it can be removed entirely, getting rid of the windows-sys dependency
|
Would it be worth deprecating |
|
I think I'd hold off on deprecation for now since depending on |
|
Do we need to hold off on making a change here since older versions of the Rust toolchain would still use the incorrect behavior on Windows when using this crate? MSRV-aware cargo will prevent it, but that's Cargo 1.84 (2025-01-09)+. which only gets us one version older than the 1.85 change that fixed the The other option here is to bump the minor version and consider this a breaking change. |
|
|
|
||
| #[cfg(unix)] | ||
| #[inline] | ||
| fn home_dir_inner() -> Option<PathBuf> { |
There was a problem hiding this comment.
Can we remove the home_dir_inner function now and inline it?
| /// Returns the path of the current user's home directory using environment | ||
| /// variables or OS-specific APIs. | ||
| /// | ||
| /// # Unix | ||
| /// | ||
| /// Returns the value of the `HOME` environment variable if it is set | ||
| /// **even** if it is an empty string. Otherwise, it tries to determine the | ||
| /// home directory by invoking the [`getpwuid_r`][getpwuid] function with | ||
| /// the UID of the current user. | ||
| /// | ||
| /// [getpwuid]: https://linux.die.net/man/3/getpwuid_r | ||
| /// | ||
| /// # Windows | ||
| /// | ||
| /// Returns the value of the `USERPROFILE` environment variable if it is set | ||
| /// **and** it is not an empty string. Otherwise, it tries to determine the | ||
| /// home directory by invoking the [`SHGetKnownFolderPath`][shgkfp] function with | ||
| /// [`FOLDERID_Profile`][knownfolderid]. | ||
| /// | ||
| /// [shgkfp]: https://learn.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetknownfolderpath | ||
| /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// match home::home_dir() { | ||
| /// Some(path) if !path.as_os_str().is_empty() => println!("{}", path.display()), | ||
| /// _ => println!("Unable to get your home dir!"), | ||
| /// } |
There was a problem hiding this comment.
I think we can replace these docs / examples with a reference to the std function we're calling.
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
6 participants
What does this PR try to resolve?
This replaces the windows specific implementation of home with the one from std::env::home_dir as it logically matches, allowing the removal of the windows-sys dependency.
How to test and review this PR?
Run thetest_with_withoutunit test to confirm it works the same.homedoesn't contain any tests itself as #16918 (comment) requested I remove the target specific code, presumably windows CI will break if the std implementation is different in some way.