Skip to content

Remove windows-sys from home#16918

Merged
arlosi merged 2 commits intorust-lang:masterfrom
Jake-Shadle:master
Apr 28, 2026
Merged

Remove windows-sys from home#16918
arlosi merged 2 commits intorust-lang:masterfrom
Jake-Shadle:master

Conversation

@Jake-Shadle
Copy link
Copy Markdown
Contributor

@Jake-Shadle Jake-Shadle commented Apr 21, 2026

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 the test_with_without unit test to confirm it works the same. home doesn'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.

@rustbot rustbot added A-environment-variables Area: environment variables A-home Area: the `home` crate S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 21, 2026

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ehuss, @epage, @weihanglo
  • @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
  • Random selection from ehuss, epage, weihanglo

@epage
Copy link
Copy Markdown
Contributor

epage commented Apr 21, 2026

Isn't the MSRV high enough now that we can just drop our own implementations and instead use the std versions?

@Jake-Shadle
Copy link
Copy Markdown
Contributor Author

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.

@epage
Copy link
Copy Markdown
Contributor

epage commented Apr 21, 2026

So long as they have the same behavior, which I believe was the intention of the recent std changes, we should be good

@Jake-Shadle
Copy link
Copy Markdown
Contributor Author

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.

@epage
Copy link
Copy Markdown
Contributor

epage commented Apr 21, 2026

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?

@ChrisDenton
Copy link
Copy Markdown
Member

Would it be worth deprecating home::home_dir now? If the only reason someone is using the home crate is because of that then they probably should switch to std::env::home_dir, no?

@epage
Copy link
Copy Markdown
Contributor

epage commented Apr 21, 2026

I think I'd hold off on deprecation for now since depending on home@0.5 could be nice for people with an extended MSRV.

@arlosi
Copy link
Copy Markdown
Contributor

arlosi commented Apr 23, 2026

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 std function.

The other option here is to bump the minor version and consider this a breaking change.

@epage
Copy link
Copy Markdown
Contributor

epage commented Apr 23, 2026

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?

homes MSRV right now is 1.92. Without --ignore-rust-version, Cargo will fail to build home on anything older. So if we make this change, the users have to be explicitly opting in to an unsupported state to have a problem.

Copy link
Copy Markdown
Contributor

@arlosi arlosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this won't build on versions of the rust toolchain that are older than the change to fix std::env::home_dir in std, this seems safe to merge.

View changes since this review

Copy link
Copy Markdown
Contributor

@arlosi arlosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation here looks good, but I'd like to get the docs updated before merging.

View changes since this review

Comment thread crates/home/src/lib.rs Outdated
Comment thread crates/home/src/lib.rs
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 23, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Apr 23, 2026
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
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 27, 2026

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Jake-Shadle
Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Apr 27, 2026
@arlosi arlosi added this pull request to the merge queue Apr 28, 2026
Merged via the queue into rust-lang:master with commit 38dba85 Apr 28, 2026
31 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-environment-variables Area: environment variables A-home Area: the `home` crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants