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

Avoid using $HOME if not necessary #9273

Merged
merged 2 commits into from
Aug 3, 2018

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Aug 2, 2018

When Parity is used within an Android application, the $HOME environment variable isn't set. This will cause Parity to panic.

This commit fixes it by avoiding to load the value of $HOME if it is not required.

@tomaka tomaka added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 2, 2018
@@ -105,7 +105,9 @@ fn upgrade_from_version(previous_version: &Version) -> Result<usize, Error> {
fn with_locked_version<F>(db_path: Option<&str>, script: F) -> Result<usize, Error>
where F: Fn(&Version) -> Result<usize, Error>
{
let mut path = db_path.map_or({
let mut path = db_path.map_or_else(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me like the db_path is always is_some() (with_locked_version is only used in upgrade, and db_path in upgrade is always Some). Maybe we can remove map_or_else altogether, or am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right.

@tomaka
Copy link
Contributor Author

tomaka commented Aug 2, 2018

It turns out that this code doesn't work in practice (we get a panic on Android), so I'll work a bit more on it.

@tomaka tomaka added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 2, 2018
@tomaka tomaka added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Aug 2, 2018
@tomaka
Copy link
Contributor Author

tomaka commented Aug 2, 2018

The code is now working. Parity runs even if you don't have $HOME set, provided you pass all the necessary --no-* options.
Also fixed the remark.

@@ -205,6 +201,10 @@ fn upgrade_user_defaults(dirs: &DatabaseDirectories) {
}

pub fn upgrade_data_paths(base_path: &str, dirs: &DatabaseDirectories, pruning: Algorithm) {
if env::home_dir().is_none() {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make make sense to print a warning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The function only does something if $HOME/.parity exists, and doesn't print any warning if it doesn't. If $HOME isn't defined we behave exactly as if $HOME/.parity doesn't exist.

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 2, 2018
@niklasad1 niklasad1 merged commit 25604dc into openethereum:master Aug 3, 2018
@tomaka tomaka deleted the dont-assume-home branch August 3, 2018 09:07
ordian added a commit to ordian/parity that referenced this pull request Aug 10, 2018
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  ethcore: add transition flag for transaction permission contract (openethereum#9275)
  Remove all dapp permissions related settings (openethereum#9120)
  Improve return data truncate logic (openethereum#9254)
  Update wasm-tests hash (openethereum#9295)
  Implement KIP4: create2 for wasm (openethereum#9277)
  Fix loop start value (openethereum#9285)
  Avoid using $HOME if not necessary (openethereum#9273)
  Fix path to parity.h (openethereum#9274)
@5chdn 5chdn added this to the 2.1 milestone Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants