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

Please reconsider endorsing dirs and deprecating std::env::home_dir #71684

Closed
Xaeroxe opened this issue Apr 29, 2020 · 82 comments
Closed

Please reconsider endorsing dirs and deprecating std::env::home_dir #71684

Xaeroxe opened this issue Apr 29, 2020 · 82 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-process Area: `std::process` and `std::env` C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Apr 29, 2020

(Link for convenience: https://doc.rust-lang.org/std/env/fn.home_dir.html)

Recommending a user library in favor of the standard library is not an action without precedent.

However in this instance I think it's unwise. The author of dirs has made it clear they are not interested in providing the level and variety of support expected by users of the Rust std library.

What I, and I suspect most of the Rust community, would prefer is fixing the behavior of std::env::home_dir, and bringing it back from deprecation. Choosing to deprecate it and recommend another crate implies that someone could be relying on the current incorrect behavior of std::env::home_dir. Do we have any reason to believe someone is relying on that behavior? Is there room for disagreement as to what correct behavior is? My own review of the discussion surrounding this didn't seem to ask those questions and just deprecated it seemingly without questioning that.

If nothing else, at this point it seems wise to remove the recommendation of the dirs crate, even if rust-lang wants to leave the deprecation intact. EDIT: Removing the recommendation has been done.

@jonas-schievink jonas-schievink added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Apr 29, 2020
@RalfJung
Copy link
Member

The author of dirs has made it clear they are not interested in providing the level and variety of support expected by users of the Rust std library.

I do not see how the link here confirms that statement, it just shows the GH project index page?

@wareya
Copy link

wareya commented Apr 30, 2020

The archive doesn't show it, but the dirs repository currently has the This repository has been archived by the owner. It is now read-only. marking on it.

@RalfJung
Copy link
Member

I see. But that doesn't say anything about "providing the level and variety of support expected by users of the Rust std library".

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Apr 30, 2020

@RalfJung I'm curious how marking the source repository read only could signal something other than "I'm not interested in maintaining this project anymore."

@RalfJung
Copy link
Member

RalfJung commented Apr 30, 2020

That was not your claim. You made a very specific claim for the motivation for them not wanting to maintain the project any more. I was asking why you think that "not interested providing the level and variety of support expected by users of the Rust std library" was the motivation.

You proved that they don't want to support this any more. You didn't prove your claim which said specifically why they don't want to support this any more.

@RalfJung
Copy link
Member

To be clear, I am entirely on board with the goals of this issue -- to stop recommending an unmaintained crate.

I am just disputing a particular statement on the original post, which I believed and still believe to be unfounded.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Apr 30, 2020

"maintained", to me, is a subset of "the level and variety of support expected by users of the Rust std library."

EDIT: I never meant to imply they said as much, my initial comments are my own evaluation.

@RalfJung
Copy link
Member

Most libraries are not maintained to the same standards as libstd. So I don't think "I don't want to maintain crate X any more" implies "I don't want to maintain crate X to the same standards as std".

But also, this is getting increasingly off-topic, so I will stop here.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented May 1, 2020

To be clear, I am entirely on board with the goals of this issue -- to stop recommending an unmaintained crate.

Would opening a PR to remove the recommendation be appropriate at this time?

@RalfJung
Copy link
Member

RalfJung commented May 2, 2020

I'll defer that question to @rust-lang/libs -- dirs is unmaintained ("archived" on GH), should we change/remove the deprecation message that recommends using it?

@BurntSushi
Copy link
Member

BurntSushi commented May 2, 2020

I think we should, at minimum, remove the recommendation for dirs. Probably just replacing it with text that says something like "use crates.io" would be fine. If we were compelled to provide a crate name, then probably home is the right answer. It's used by Cargo, for example. With that said, searching for "home directory" or "home dir" does have home show up in the first page of results.

It would be nice to un-deprecate and fix this, but this was certainly something that was considered. See:

Personally, I'd like to see a working home_dir function in std, but it sounds like fixing the current function is probably not a good path toward it. So perhaps a new function is warranted that does the "right" thing, whatever that is. (If home is good enough for Cargo and rustup, then maybe it is good enough for std?)

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented May 2, 2020

I've opened #71784 to remove the recommendation for now. I don't intend to halt the discussion as to whether rust-lang should recommend a crate here, or if a function should be pulled into std from crates.io, but we can act on the decision we did make, which is to not recommend this crate.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented May 2, 2020

It's pretty clear the behaviour of this deprecated function is toeing the line between bug and feature. Were it a bug, the answer would be simple, fix the problem. No deprecation necessary. If alternatively it's a misguided feature, then deprecation seems like a reasonable approach.

Frequently during the discussions you linked I see concerns being raised that this is, perhaps to some, a feature, or at the very least that changing it may cause excessive breaking changes. I suppose I understand the caution, as gathering data on the usage of this API is nearly impossible, since most of its users are going to be bin users.

One approach to gathering the data necessary to answer that question might be building telemetry into rustc itself. As far as I know there's no such precedence for such telemetry in rustc though. That would probably be a risky and controversial move in and of itself. So a simpler approach honestly might be a poll/survey. Ask the community. Imprecise, but any data is better than no data? It's certainly an interesting problem. I can't help wondering how many users of Rust programs would actually experience an observable change in program behavior from this. How many of those would consider such a change fatal to their workflow?

Additionally, one might consider such breakage fine, because if the behaviour of the code using this is really that important, then the software shop probably has rigorous QA processes that would catch this kind of impact.

In the absence of a good dataset to draw from I suppose the most cautious approach is the only good one, and the most cautious approach is leaving the behaviour unchanged and deprecating the API.

@tesuji
Copy link
Contributor

tesuji commented May 2, 2020

Hi, I created @xdg-rs for taking care of maintaining dirs crate and its friends.
Currently we have @tbu- who has publishing permission on crates.io.
I believe that Simon (soc) wouldn't remove him from crates.io owner.
But I agree that std should remove recommendation for specific crates.

@tesuji
Copy link
Contributor

tesuji commented May 2, 2020

Hey, a concern raised in deprecating std home_dir is that Python does considering
HOME var on Windows. It hasn't be like that since python 3.8:

Changed in version 3.8: No longer uses HOME on Windows.

@wareya
Copy link

wareya commented May 2, 2020

As a windows user, it bothers me a lot when applications check for standard configuration/environment features in nonstandard ways. The worst one I run into is applications looking for UI language grabbing it from the system locale instead of from the, uh, system UI language. Using environment variables to find the "home" folder on windows is essentially the same; someone might want to check environment variables to find the home folder on windows, but doing so is essentially wrong in 99%+ of cases, just like interpreting the system locale as UI language. Of course, checking the USERPROFILE environment variable is forgivable since it's hardly ever going to be wrong, but HOME just isn't set by default on Windows in the first place (at least not the version I run), so checking it is just begging to have things magically misbehave in strange environments.

Problems like this just kind of tend to happen when porting unix applications to win32 or vice versa, but, you know, windows doesn't give you all the necessary tools to work around badly-behaving applications that unix systems generally do.

Of course, I'm not trying to resurrect old issues, but I wanted to throw in an argument in favor of the current std::env::home_dir staying deprecated, since someone threw in an argument that it may be considered a featurebug.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 2, 2020
Remove recommendation for unmaintained dirs crate

See rust-lang#71684 for reasoning here
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 2, 2020
Remove recommendation for unmaintained dirs crate

See rust-lang#71684 for reasoning here
@cedricgrothues
Copy link

If we were compelled to provide a crate name, then probably home is the right answer.

@BurntSushi, it seems as if home uses env::home_dir() internally at least on unix (I'm pretty sure it calls the same methods on windows, too).

@BurntSushi
Copy link
Member

@cedricgrothues Hmm, doesn't look like it uses env::home_dir() on Windows at least: https://github.com/brson/home/blob/master/src/windows.rs

@Timmmm
Copy link
Contributor

Timmmm commented Jun 1, 2020

I would be ok with std::env::home_dir2(). It's ugly but only in proportion to this situation. :-)

@Boscop
Copy link

Boscop commented Jun 1, 2020

@Timmmm I'd prefer something like std::env::home_directory() so that in a future edition, std::env::home_dir could be removed without its successor having a weird name.

@BurntSushi
Copy link
Member

@Boscop Editions can't remove APIs from std.

@Timmmm
Copy link
Contributor

Timmmm commented Jun 2, 2020

I'd prefer something like std::env::home_directory()

I think that would just lead to more confusion (given that home_dir can't be removed). What is the difference between home_dir and home_directory? They sound like they're the same thing - which one should I use? With home_dir and home_dir2 it is much more obvious. I agree home_dir2 is an ugly name.

@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2020

That confusion is something documentation can solve. There is precedent for picking a new but sensible name: before_exec got deprecated and replaced by pre_exec.

@haug1
Copy link

haug1 commented Nov 17, 2020

Would be nice to get this sorted

  #![allow(deprecated)] // Deprecated because of Windows compatibility (I think)
  let path = std::env::home_dir()...
  ...

@seahurt
Copy link

seahurt commented Aug 15, 2022

It's really funny to retire a std function and let user choose a third-party lib.

@ghost
Copy link

ghost commented Aug 15, 2022

I don't think it's always inappropriate to do that, but in this case I think it's definitely functionality that should be in std. And if it isn't, there should at least be a specific crate blessed as the official solution, and it should be named in the deprecation notice, and it should be a crate that is >=1.0, and does not call the deprecated function itself.

Timmmm added a commit to Timmmm/rust that referenced this issue Sep 10, 2022
This improves the documentation to say *why* it was deprecated. The reason was because it reads `HOME` on Windows which is meaningless there. Note that the PR that deprecated it stated that returning an empty string if `HOME` is set to an empty string was a problem, however I can find no evidence that this is the case. `cd` handles it fine whereas if `HOME` is unset it gives an explicit `HOME not set` error.

* Original deprecation reason: https://internals.rust-lang.org/t/deprecate-or-break-fix-std-env-home-dir/7315
* Original deprecation PR: rust-lang#51656

See rust-lang#71684
@Timmmm
Copy link
Contributor

Timmmm commented Sep 10, 2022

☝️ I added a PR to at least document why it is deprecated so users can make up their own minds. I definitely care less about Cygwin and Mingw results being possibly unexpected than having to add a whole third party crate just for this.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Dec 11, 2022
Better documentation for env::home_dir()'s broken behaviour

This improves the documentation to say *why* it was deprecated. The reason was because it reads `HOME` on Windows which is meaningless there. Note that the PR that deprecated it stated that returning an empty string if `HOME` is set to an empty string was a problem, however I can find no evidence that this is the case. `cd` handles it fine whereas if `HOME` is unset it gives an explicit `HOME not set` error.

* Original deprecation reason: https://internals.rust-lang.org/t/deprecate-or-break-fix-std-env-home-dir/7315
* Original deprecation PR: rust-lang#51656

See rust-lang#71684
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 11, 2022
Better documentation for env::home_dir()'s broken behaviour

This improves the documentation to say *why* it was deprecated. The reason was because it reads `HOME` on Windows which is meaningless there. Note that the PR that deprecated it stated that returning an empty string if `HOME` is set to an empty string was a problem, however I can find no evidence that this is the case. `cd` handles it fine whereas if `HOME` is unset it gives an explicit `HOME not set` error.

* Original deprecation reason: https://internals.rust-lang.org/t/deprecate-or-break-fix-std-env-home-dir/7315
* Original deprecation PR: rust-lang#51656

See rust-lang#71684
@utkarshgupta137
Copy link
Contributor

Now that the home crate is maintained by the cargo & rustup teams, would it be okay to recommend it instead?

@utkarshgupta137
Copy link
Contributor

Now that the home crate is maintained by the cargo & rustup teams, would it be okay to recommend it instead?

I've created a PR to do the same: #110665

@tmccombs
Copy link
Contributor

I'd rather have a new function in std that does what the home crate does. Also, the home crate calls the std home_dir function on Unix.

@utkarshgupta137
Copy link
Contributor

Me too, but there is obviously some contention regarding that (especially around naming), so I would at least point people to a small & suitable crate in the meanwhile.

@soc
Copy link
Contributor

soc commented Apr 23, 2023

@utkarshgupta137 but what does it accomplish?

If you are fine with the incorrectness in env::home_dir, it would be more straightforward to un-deprecate env::home_dir instead of recommending a crate that literally calls env::home_dir itself.

@utkarshgupta137
Copy link
Contributor

@utkarshgupta137 but what does it accomplish?

If you are fine with the incorrectness in env::home_dir, it would be more straightforward to un-deprecate env::home_dir instead of recommending a crate that literally calls env::home_dir itself.

The home crate fixes the incorrectness.

@soc
Copy link
Contributor

soc commented Apr 23, 2023

The home crate fixes the incorrectness.

home

Where?

@utkarshgupta137
Copy link
Contributor

utkarshgupta137 commented Apr 23, 2023

The home crate fixes the incorrectness.

home

Where?

The 2 lines above it:
https://github.com/rust-lang/cargo/blob/master/crates/home/src/lib.rs#L68-L69

According to the documentation, the problem is that the HOME variable is respected on Windows, which the home crate doesn't.

@soc
Copy link
Contributor

soc commented Apr 23, 2023

the problem is that the HOME variable is respected on Windows

The problems on Windows where far from the only reasons why the function was deprecated.

According to the documentation

I guess another case of "incomplete documentation is worse than no documentation". 🤷

@utkarshgupta137
Copy link
Contributor

utkarshgupta137 commented Apr 23, 2023

The problems on Windows where far from the only reasons why the function was deprecated.

I must've missed some previous discussion, can you elaborate? Since cargo & rustup teams now maintain the home crate, I'm sure they would want to fix it.

@wareya
Copy link

wareya commented Apr 23, 2023

I must've missed some previous discussion, can you elaborate? Since cargo & rustup teams now maintain the home crate, I'm sure they would want to fix it.

soc deprecated home_dir and added a recommendation for the dirs crate in the documentation (what this issue thread was originally mainly about, before the recommendation was removed) in this PR: #51656 - the two main issues at the time were: one, it's wrong to use HOME on windows, and two, confusing behavior on unix systems when HOME is blank (whatever the source). The conversations around these two main issues leading up to soc's PR are scattered across the issue tracker in several different issue threads, as well as rust forum threads (like the one soc links as the first comment on their PR).

@Timmmm
Copy link
Contributor

Timmmm commented Apr 23, 2023

The problems on Windows where far from the only reasons why the function was deprecated.

I wrote that updated documentation and I may have got it wrong, but I did look back through the discussion that was had when it was deprecated and I don't recall seeing any other reason. Can you perhaps post a link?

I think recommending the home crate is very reasonable. It's just a recommendation. It can be changed in future if the situation evolves.

@utkarshgupta137
Copy link
Contributor

utkarshgupta137 commented Apr 24, 2023

I must've missed some previous discussion, can you elaborate? Since cargo & rustup teams now maintain the home crate, I'm sure they would want to fix it.

soc deprecated home_dir and added a recommendation for the dirs crate in the documentation (what this issue thread was originally mainly about, before the recommendation was removed) in this PR: #51656 - the two main issues at the time were: one, it's wrong to use HOME on windows, and two, confusing behavior on unix systems when HOME is blank (whatever the source). The conversations around these two main issues leading up to soc's PR are scattered across the issue tracker in several different issue threads, as well as rust forum threads (like the one soc links as the first comment on their PR).

Yikes, the empty HOME seems like a genuine problem. I'll raise a PR for the home crate & see what they think.

Edit: Done: rust-lang/cargo#12023

Edit 2: @ehuss pointed out that most other languages don't handle this, so I don't see why Rust (or one of the crates) should go out of the way to do so. Additionally, defining $HOME is part of the POSIX spec, so IMO, it should be fine to not handle the potential edge case.

@tmccombs
Copy link
Contributor

Regarding the name for a new function, I wonder if a better way to handle it would be to move it to a different module. Perhaps std::path or std::fs? Or perhaps even better create a std::env::dirs module which has a new, better home fn, as well as config_dir, data_dir, cache_dir, etc. functions.

@Dylan-DPC
Copy link
Member

Dylan-DPC commented Jun 7, 2024

Current state of this, the function is deprecated with:

Deprecated since 1.29.0: This function’s behavior may be unexpected on Windows. Consider using a crate from crates.io instead.

which solves this issue and part of why it was posted
Also, the function can't be undeprecated because it relies on windows behaviour and I think the best solution here is to close this.

@ehuss
Copy link
Contributor

ehuss commented Jun 7, 2024

For more context, a replacement is being proposed in rust-lang/libs-team#372.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-process Area: `std::process` and `std::env` C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.