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

Fix windows home dir implementation #46799

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@Diggsey
Contributor

Diggsey commented Dec 18, 2017

Fixes #28940

r? @dtolnay

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Dec 18, 2017

Member

@rust-lang/libs Check out the linked issue, especially #28940 (comment) and #28940 (comment). I believe this is going to hinge on whether fixing the incorrect behavior of home_dir is an acceptable level of breakage, or whether we need to come up with a fun new name under which to expose the correct behavior.

@rfcbot fcp merge

Member

dtolnay commented Dec 18, 2017

@rust-lang/libs Check out the linked issue, especially #28940 (comment) and #28940 (comment). I believe this is going to hinge on whether fixing the incorrect behavior of home_dir is an acceptable level of breakage, or whether we need to come up with a fun new name under which to expose the correct behavior.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 18, 2017

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Dec 18, 2017

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Dec 18, 2017

Member

Could someone explain the interaction with cygwin in more detail here? My reading of this comment seems to suggest that this might change the behavior of command line tools that are built to run in either cygwin or cmd on Windows. For example, is it possible that such command line tools might accidentally do the right thing for cygwin users today?

Member

BurntSushi commented Dec 18, 2017

Could someone explain the interaction with cygwin in more detail here? My reading of this comment seems to suggest that this might change the behavior of command line tools that are built to run in either cygwin or cmd on Windows. For example, is it possible that such command line tools might accidentally do the right thing for cygwin users today?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Dec 18, 2017

Contributor

Cygwin emulates a linux distribution on windows: the cygwin target is therefore a linux target and code compiled for that target should not use this code-path (or any cfg(windows) code really). This is different from the -gnu target which is a windows target, it's just that the mingw toolchain is often run under cygwin.

As with any change in behaviour, it's possible that this is accidentally working for someone right now, but on the other hand there are definitely applications currently using home_dir() which are accidentally broken when run from a cygwin/msys terminal. (Rustup has an entire utils module dedicated to re-implementing standard library functions which do not work optimally on windows, home_dir included)

Contributor

Diggsey commented Dec 18, 2017

Cygwin emulates a linux distribution on windows: the cygwin target is therefore a linux target and code compiled for that target should not use this code-path (or any cfg(windows) code really). This is different from the -gnu target which is a windows target, it's just that the mingw toolchain is often run under cygwin.

As with any change in behaviour, it's possible that this is accidentally working for someone right now, but on the other hand there are definitely applications currently using home_dir() which are accidentally broken when run from a cygwin/msys terminal. (Rustup has an entire utils module dedicated to re-implementing standard library functions which do not work optimally on windows, home_dir included)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 18, 2017

Member

I believe the main pushback on this issue was indeed the breaking changes that would come about with a change like this. For example Cargo and Rustup may be likely to break as tools which heavily use the home directory for caching and such. I'm not sure how many other tools rely on something like this.

Cargo now depends on a crate called home so the logic isn't in precisely Cargo, but I believe this is a breaking change for much of Cargo/rustup behavior:

I believe that rustup/Cargo are both intended to not use $HOME by default on Windows and instead prefer the "real home dir" for fresh installs and such. Despite that though I think this change would still be a breaking change for these tools as they're explicitly attempting to be backwards compatible.

I'm not entirely sure how to guage the expected breakage across the rest of the ecosystem. It may be best to have a better story for this breakage, for example "use this crate for the old behavior" or something like that would be a great way to help mitigate the breakage. (aka the thing that cargo/rustup will transition to using). If we can get our ducks in a row via something like that I'd be ok landing this.

Member

alexcrichton commented Dec 18, 2017

I believe the main pushback on this issue was indeed the breaking changes that would come about with a change like this. For example Cargo and Rustup may be likely to break as tools which heavily use the home directory for caching and such. I'm not sure how many other tools rely on something like this.

Cargo now depends on a crate called home so the logic isn't in precisely Cargo, but I believe this is a breaking change for much of Cargo/rustup behavior:

I believe that rustup/Cargo are both intended to not use $HOME by default on Windows and instead prefer the "real home dir" for fresh installs and such. Despite that though I think this change would still be a breaking change for these tools as they're explicitly attempting to be backwards compatible.

I'm not entirely sure how to guage the expected breakage across the rest of the ecosystem. It may be best to have a better story for this breakage, for example "use this crate for the old behavior" or something like that would be a great way to help mitigate the breakage. (aka the thing that cargo/rustup will transition to using). If we can get our ducks in a row via something like that I'd be ok landing this.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 18, 2017

Member

@rfcbot concern cargo-rustup-breakage

(see previous comment)

Member

alexcrichton commented Dec 18, 2017

@rfcbot concern cargo-rustup-breakage

(see previous comment)

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Dec 18, 2017

Contributor

@alexcrichton There's no way this change can break cargo or rustup, since as you pointed out, they don't call this function?

The good thing is that with this change, the unsafe code in those tools could be removed:
env::var("USERPROFILE").unwrap_or_else(env::home_dir)

Contributor

Diggsey commented Dec 18, 2017

@alexcrichton There's no way this change can break cargo or rustup, since as you pointed out, they don't call this function?

The good thing is that with this change, the unsafe code in those tools could be removed:
env::var("USERPROFILE").unwrap_or_else(env::home_dir)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 18, 2017

Member

@Diggsey as I pointed out here and here both tools in fact call std::env::home_dir, and the functionality they use that for would break.

Member

alexcrichton commented Dec 18, 2017

@Diggsey as I pointed out here and here both tools in fact call std::env::home_dir, and the functionality they use that for would break.

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Dec 18, 2017

Member

Cygwin emulates a linux distribution on windows: the cygwin target is therefore a linux target and code compiled for that target should not use this code-path (or any cfg(windows) code really)

I think this would be my concern then. Right now I distribute a single binary that is intended to work well in both cygwin and cmd. Perhaps this is "wrong," but having two different bins for cygwin and cmd would be quite user hostile IMO. Anyway, I don't want to debate that aspect too much, but would rather just say that this is a source of potential breakage.

Member

BurntSushi commented Dec 18, 2017

Cygwin emulates a linux distribution on windows: the cygwin target is therefore a linux target and code compiled for that target should not use this code-path (or any cfg(windows) code really)

I think this would be my concern then. Right now I distribute a single binary that is intended to work well in both cygwin and cmd. Perhaps this is "wrong," but having two different bins for cygwin and cmd would be quite user hostile IMO. Anyway, I don't want to debate that aspect too much, but would rather just say that this is a source of potential breakage.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 18, 2017

Contributor

I'm not even sure there's a single correct home directory under MSYS.

"Globally installed" programs like rustc/cargo from rustup that just happen to be called from MSYS shell (but can be run outside of it) should still use the "global" home directory, i.e. the Windows one.

If something is supposed to be installed locally into the MSYS's little Unix filesystem emulation, then it should use /home directory inside of it and may break if run from outside of MSYS.

The way of treating and prioritizing home directories seems to be largely application-dependent.

Contributor

petrochenkov commented Dec 18, 2017

I'm not even sure there's a single correct home directory under MSYS.

"Globally installed" programs like rustc/cargo from rustup that just happen to be called from MSYS shell (but can be run outside of it) should still use the "global" home directory, i.e. the Windows one.

If something is supposed to be installed locally into the MSYS's little Unix filesystem emulation, then it should use /home directory inside of it and may break if run from outside of MSYS.

The way of treating and prioritizing home directories seems to be largely application-dependent.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jan 9, 2018

Member

Sorry for the long delay on this!

The libs team discussed this API in our previous meeting. While it seems good to pursue this change, we are wary of changing the behavior of stable APIs, and instead would like to propose the following:

  • Experiment with an improved API in an external crate, perhaps the home crate.
  • Potentially add other useful, related APIs to that crate.
  • Eventually, deprecate the std APIs, either pointing to the external crate, or by pulling in the new APIs to std

How does that sound?

Member

aturon commented Jan 9, 2018

Sorry for the long delay on this!

The libs team discussed this API in our previous meeting. While it seems good to pursue this change, we are wary of changing the behavior of stable APIs, and instead would like to propose the following:

  • Experiment with an improved API in an external crate, perhaps the home crate.
  • Potentially add other useful, related APIs to that crate.
  • Eventually, deprecate the std APIs, either pointing to the external crate, or by pulling in the new APIs to std

How does that sound?

@Diggsey

This comment has been minimized.

Show comment
Hide comment
@Diggsey

Diggsey Jan 9, 2018

Contributor

@aturon the home crate already implements the breaking part of this change (not looking at %HOME% on windows) and that's its entire reason for being. What is left to experiment with?

Contributor

Diggsey commented Jan 9, 2018

@aturon the home crate already implements the breaking part of this change (not looking at %HOME% on windows) and that's its entire reason for being. What is left to experiment with?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 24, 2018

Member

Triage ping @rust-lang/libs, given the comment in #46799 (comment) I suppose the FCP should be canceled?

@Diggsey as you mentioned first step is already done in the home package, what about we just perform the rest by repurposing this PR (if we want to keep it open)?

  • deprecate std::env::home_dir, and/or
  • introduce a new function e.g. std::env::home_path which does the correct thing
Member

kennytm commented Jan 24, 2018

Triage ping @rust-lang/libs, given the comment in #46799 (comment) I suppose the FCP should be canceled?

@Diggsey as you mentioned first step is already done in the home package, what about we just perform the rest by repurposing this PR (if we want to keep it open)?

  • deprecate std::env::home_dir, and/or
  • introduce a new function e.g. std::env::home_path which does the correct thing
@Zoxc

This comment has been minimized.

Show comment
Hide comment
@Zoxc

Zoxc Jan 26, 2018

Contributor

I don't think there should be a function to get this directory on Windows. Typical applications should not be accessing it. Having it in the standard library makes it too convenient to do so.

Contributor

Zoxc commented Jan 26, 2018

I don't think there should be a function to get this directory on Windows. Typical applications should not be accessing it. Having it in the standard library makes it too convenient to do so.

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Jan 29, 2018

Member

@rfcbot fcp cancel

Echoing #46799 (comment) -- breaking a stable API this way is not going to work even if the current behavior is allegedly never what someone should want. Let's work on figuring out a better API in an external crate. The home crate (which currently implements home_dir() on Windows not the way this PR does, because it considers %USERPROFILE%) would be a good place to try to get this right, and less disruptive than iterating on this in the standard library.

As a first step I would recommend cataloguing as many use cases as possible among the crates that currently call home::home_dir or env::home_dir. From those we can see if there is a pattern of why they need to know the home dir. Are they trying to figure out where to save a downloaded file? Are they writing a dotfile? Are they implementing a cache? All of those are more cross-platform concepts than the concept of a "home dir" so it may make sense to serve such use cases individually, in line with what the home crate is already doing with cargo_home and rustup_home.

Member

dtolnay commented Jan 29, 2018

@rfcbot fcp cancel

Echoing #46799 (comment) -- breaking a stable API this way is not going to work even if the current behavior is allegedly never what someone should want. Let's work on figuring out a better API in an external crate. The home crate (which currently implements home_dir() on Windows not the way this PR does, because it considers %USERPROFILE%) would be a good place to try to get this right, and less disruptive than iterating on this in the standard library.

As a first step I would recommend cataloguing as many use cases as possible among the crates that currently call home::home_dir or env::home_dir. From those we can see if there is a pattern of why they need to know the home dir. Are they trying to figure out where to save a downloaded file? Are they writing a dotfile? Are they implementing a cache? All of those are more cross-platform concepts than the concept of a "home dir" so it may make sense to serve such use cases individually, in line with what the home crate is already doing with cargo_home and rustup_home.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 29, 2018

@dtolnay proposal cancelled.

rfcbot commented Jan 29, 2018

@dtolnay proposal cancelled.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Apr 16, 2018

Contributor

I know this is an ancient thread but @alexcrichton the functionality you are pointing to in home is pretty much on its way out with rust-lang/cargo#5183 - that PR doesn't acknowledge that hack at all - and it's a pretty old hack.

Contributor

brson commented Apr 16, 2018

I know this is an ancient thread but @alexcrichton the functionality you are pointing to in home is pretty much on its way out with rust-lang/cargo#5183 - that PR doesn't acknowledge that hack at all - and it's a pretty old hack.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Apr 16, 2018

Contributor

The actual 'home' definitions are legit, whereas the definitions in directories are MPL2.

Contributor

brson commented Apr 16, 2018

The actual 'home' definitions are legit, whereas the definitions in directories are MPL2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment