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

env::home_dir() should use SHGetKnownFolderPath() on windows. #28940

Closed
Diggsey opened this issue Oct 10, 2015 · 23 comments
Closed

env::home_dir() should use SHGetKnownFolderPath() on windows. #28940

Diggsey opened this issue Oct 10, 2015 · 23 comments
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Oct 10, 2015

At the moment, it first looks at %HOME%, and then at %USERPROFILE%. This is bad for two reasons:

  1. The home directory will be different when run from an msys or cygwin terminal. Unless the program is specifically compiled to run in an msys/cygwin environment (they have their own target triples) then it should not be affected by %HOME% (only %USERPROFILE% stores the home directory on windows).

  2. Environment variables may be overridden. SHGetKnownFolderPath() will always give the correct path to the user's home folder.

@FaultyRAM
Copy link

Adding to this, env::temp_dir() also parses environment variables where SHGetKnownFolderPath() would be more appropriate.

@barosl
Copy link
Contributor

barosl commented Oct 10, 2015

By looking at the code, it seems that home_dir already utilizes GetUserProfileDirectory, which is a bit more portable option than SHGetKnownFolderPath (albeit Rust basically targets Vista, so this won't be much a concern). The thing is, %HOME% and %USERPROFILE% are explicitly checked first before using the Windows API. I guess this is an intended behavior to enable the exact overriding behavior you described (so that you could get the "incorrect" path by setting %HOME% manually), but I'm not very sure. Could you confirm, @alexcrichton?

As for temp_dir, it is already using GetTempPath to return the appropriate path. The reason why the Rust documentation says it checks the environment variables is that it is exactly the behavior described in MSDN. It states: "The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found". And also, SHGetKnownFolderPath seems to not support the temporary directory.

@nagisa
Copy link
Member

nagisa commented Oct 10, 2015

Relevant PR which contains some related discussion.

@FaultyRAM
Copy link

@barosl Oops, you're right. In that case, I'd say this is bad documentation - not mentioning GetUserProfileDirectory() seems to be an oversight, and we should be describing the actual behaviour of env::get_temp() on Windows even if the effective behaviour is the same as what's described now, because that's not what I expected, and certainly not what others will expect. That and, of course, GetTempPath() could always change or be deprecated in the future, and then the docs would be wrong anyway.

@barosl
Copy link
Contributor

barosl commented Oct 11, 2015

@FaultyRAM I sent a PR that resolves the documentation issue, please review it: #28960.

barosl added a commit to barosl/rust that referenced this issue Oct 11, 2015
@FaultyRAM
Copy link

@barosl Looks good!

bors added a commit that referenced this issue Oct 11, 2015
@alexcrichton
Copy link
Member

Believe it or not, this behavior has been here since literally the beginning (dafd649) of this module. The fallback to GetUserProfileDirectory was added relatively recently (70ed3a4) after it was suggested. I think it's safe to say that this was not highly scrutinized to the point of vetting the implementation as to what it should be.

I'll also note that this basically matches what python does as well as what ruby does. My personal opinion is that it's working as intended.

@Diggsey
Copy link
Contributor Author

Diggsey commented Oct 12, 2015

I'll also note that this basically matches what python does as well as what ruby does. My personal opinion is that it's working as intended.

And in every case it just makes the user experience worse on windows. The convention for getting the home directory on windows is (obviously) to use the helpfully provided OS functions to do so. That's what .net, java and other software does, that has a runtime which is actually written for windows, and is not just a barely functioning wrapper in order to provide a unix-like interface.

At best, the python and ruby interpreters can be slightly let off the hook as they are scripting languages, and so you might expect behaviour to be a little "fuzzy" with respect to the environment. At worst, they're shoving unix-like conventions onto a platform where they don't work or belong.

It's also one of the reasons why msys doesn't try to override the %USERPROFILE% environment variable: native windows software isn't supposed to change behaviour when run from msys: the only way msys functions properly is if native windows software is oblivious to it.

If you want a unix-like environment on windows, then target msys or cygwin - it's what they're there for after all... If you don't, then don't start looking for unix environment variables!

@retep998
Copy link
Member

I am personally in favor of disregarding the environment variables entirely and just calling the OS function directly and leaving it at that.

@huonw huonw added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 23, 2015
@steveklabnik
Copy link
Member

nominating so libs can make a call on what's the right behavior here

@brson
Copy link
Contributor

brson commented Mar 14, 2017

I believe this definition is incorrect as both cargo and rustup do not use it, because considering HOME on windows makes the behavior unpredictable depending on whether the program is running under a mingw/cygwin.

I think we should deprecate the current function and define a new one that cargo and rustup use, thus proving it is usable for real applications.

@aturon
Copy link
Member

aturon commented Mar 14, 2017

I agree with @brson. Anyone want to take on making a PR for this?

@brson brson removed the I-nominated label Mar 14, 2017
@retep998
Copy link
Member

define a new one

What name should the new one have? user_profile_dir?

@aturon
Copy link
Member

aturon commented Mar 14, 2017

How about user_dir? Not idiomatic for any of our major platforms, but hopefully gets the idea across?

@nagisa
Copy link
Member

nagisa commented Mar 14, 2017

See also #40320

@Diggsey
Copy link
Contributor Author

Diggsey commented Mar 14, 2017

It seems a shame to have to pick slightly worse names for things each time we deprecate something.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 24, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 15, 2017

On the spectrum of acceptable breakage, how bad is changing home_dir on Windows to do the right thing? It seems like we are saying pretty much anything calling home_dir on Windows right now is buggy behavior. Is that fair?

@droundy
Copy link
Contributor

droundy commented Nov 15, 2017

My vote, for what it's worth, would be to just treat the current behavior on windows as a bug, and fix it. Is anyone aware of rust code that relies on the current Windows behavior?

@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 19, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2017

Let's get a PR that fixes the bug and go from there.

@dtolnay
Copy link
Member

dtolnay commented Jan 29, 2018

We decided against making this change in #46799. #46799 (comment) proposes possible next steps for handling this better in an external crate.

@dtolnay dtolnay closed this as completed Jan 29, 2018
@soc
Copy link
Contributor

soc commented Jun 13, 2018

std::env::home_dir is also broken on Linux – is there any chance to get anything fixed at all in this case, or does it make more sense to give up and tell people to roll their own implementation (which I fear will lead to many implementations that lack the platform coverage of std::env::home_dir)?

@tjkirch
Copy link

tjkirch commented Jul 30, 2018

@soc - how is it broken on Linux? I'm trying to figure out why home_dir was deprecated when it's working great for me on Linux.

@soc
Copy link
Contributor

soc commented Jul 30, 2018

@tjkirch #51656 hope this helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests