Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Replace std::env::home_dir() with home crate impl. #9293

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

c0gent
Copy link
Contributor

@c0gent c0gent commented Aug 6, 2018

  • Import the home crate in util/dir.

  • Replace uses of env::home_dir() with home::home_dir().

    • home uses a 'correct' impl. on windows and the stdlib impl. of ::home_dir otherwise.
  • Reexport home::home_dir from util/dir.

  • Bump util/dir to 0.1.2.

Addresses deficiencies of #9077 and #9281. Tested on Linux and Windows.

@parity-cla-bot
Copy link

It looks like @c0gent signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@c0gent c0gent force-pushed the c0gent-home-dir branch 5 times, most recently from 48ce57e to 3a87d3b Compare August 7, 2018 00:00
@c0gent
Copy link
Contributor Author

c0gent commented Aug 7, 2018

@tomaka Please let me know what you think about this.

I'm guessing v1::tests::mocked::signing::should_check_status_of_request_when_its_resolved can spuriously fail?

@dvdplm
Copy link
Collaborator

dvdplm commented Aug 8, 2018

@niklasad1 this is very similar to your PR from early July that was later reversed because of Android issues I think? I don't remember the conclusion of that though, to hold off?

@tomaka
Copy link
Contributor

tomaka commented Aug 8, 2018

Would merging this PR move the base directory of people using Windows?

@c0gent
Copy link
Contributor Author

c0gent commented Aug 8, 2018

What this does is make the home directory more consistent. It's possible that someone running Parity on msys/cygwin could now have a different home folder.

I could add some logic on startup to detect this potential condition (under windows) and provide a helpful error message or something. Or just silently move the folder (if none exists in the 'new' location). What do you think?

@niklasad1
Copy link
Collaborator

@dvdplm This is slightly different instead of using dirs crate as rustc suggest. This PR is using the home crate which is using a different Windows implementation but the same implementation as the stdlib for UNIX.

So, my educated guess is that this will work fine on Android (UNIX impl) because it doesn't have any conditional compilation guards to prevent platforms other than Linux, OSX and Windows. (I think the home_dir will be root folder on Android not sure though)

But this must absolutely be tested on Android before merging!

@tomaka
Copy link
Contributor

tomaka commented Aug 8, 2018

But this must absolutely be tested on Android before merging!

Well, home_dir() will return None on Android because the HOME directory doesn't really make sense.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 14, 2018
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Compiles on android good enough for me!

But, the PR needs to be rebased/merged to master

@niklasad1 niklasad1 requested a review from tomaka August 20, 2018 12:03
* Import the `home` crate in `util/dir`.

* Replace uses of `env::home_dir()` with `home::home_dir()`.
  * `home` uses a 'correct' impl. on windows and the stdlib impl.
    of `::home_dir` otherwise.

* Reexport `home::home_dir` from `util/dir`.

* Bump `util/dir` to 0.1.2.
@dvdplm
Copy link
Collaborator

dvdplm commented Aug 21, 2018

@c0gent can you sort out the merge conflicts so we can merge this please? :)

@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 21, 2018
@c0gent
Copy link
Contributor Author

c0gent commented Aug 21, 2018

Done.

@niklasad1
Copy link
Collaborator

@c0gent

README.MD should not be deleted, can you revert that?

@niklasad1 niklasad1 added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 21, 2018
@dvdplm dvdplm merged commit 139a2b7 into openethereum:master Aug 21, 2018
@ordian
Copy link
Collaborator

ordian commented Aug 21, 2018

Please revert README.MD deletion in the next PR.

@5chdn 5chdn mentioned this pull request Aug 21, 2018
@5chdn 5chdn added this to the 2.1 milestone Aug 21, 2018
@c0gent
Copy link
Contributor Author

c0gent commented Aug 21, 2018

Sorry, not sure how that happened...

@c0gent c0gent deleted the c0gent-home-dir branch September 1, 2018 17:31
@ghost ghost mentioned this pull request Sep 19, 2018
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants