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

New default paths#11641

Merged
dvdplm merged 1 commit into
masterfrom
vorot93/new-path
Apr 24, 2020
Merged

New default paths#11641
dvdplm merged 1 commit into
masterfrom
vorot93/new-path

Conversation

@vorot93
Copy link
Copy Markdown

@vorot93 vorot93 commented Apr 20, 2020

This PR changes the default paths to reflect the new project name while retaining support for the old paths if they are still in use.

Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

I don't think this goes the whole way but having a few illustrative unit tests showing what the intent is might clear it up.

Comment thread util/dir/src/lib.rs Outdated
pub fn default_data_path() -> String {
let app_info = AppInfo { name: PRODUCT, author: AUTHOR };
get_app_root(AppDataType::UserData, &app_info).map(|p| p.to_string_lossy().into_owned()).unwrap_or_else(|_| "$HOME/.parity".to_owned())
default_path(AppDataType::UserData, PRODUCT).map(|p| p.to_string_lossy().into_owned()).unwrap_or_else(|| "$HOME/.openethereum".to_owned())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm reading this wrong but it seems like you're going to end up with a path containing io.parity.ethereum/Parity on *nix/windows, and then OpenEthereum added to that. That isn't what we want I think?
When I run the test here it passes but printing it I see: Directories { base: "/home/dvdplm/.local/share/io.parity.ethereum", db: "/home/dvdplm/.local/share/io.parity.ethereum/chains", cache: "/home/dvdplm/.local/share/io.parity.ethereum/cache", keys: "/home/dvdplm/.local/share/io.parity.ethereum/keys", signer: "/home/dvdplm/.local/share/io.parity.ethereum/signer", secretstore: "/home/dvdplm/.local/share/io.parity.ethereum/secretstore" }

Copy link
Copy Markdown
Author

@vorot93 vorot93 Apr 20, 2020

Choose a reason for hiding this comment

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

I retained that code as the old path build mechanism. If the old constructed path does not exist it is discarded. It probably prints the old path because you have it on your disk already?

Copy link
Copy Markdown
Author

@vorot93 vorot93 Apr 20, 2020

Choose a reason for hiding this comment

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

macOS

Directories { base: "/Users/vorot93/Library/Application Support/OpenEthereum", db: "/Users/vorot93/Library/Application Support/OpenEthereum/chains", cache: "/Users/vorot93/Library/Application Support/OpenEthereum/cache", keys: "/Users/vorot93/Library/Application Support/OpenEthereum/keys", signer: "/Users/vorot93/Library/Application Support/OpenEthereum/signer", secretstore: "/Users/vorot93/Library/Application Support/OpenEthereum/secretstore" }

if I create the legacy folder manually

Directories { base: "/Users/vorot93/Library/Application Support/io.parity.ethereum", db: "/Users/vorot93/Library/Application Support/io.parity.ethereum/chains", cache: "/Users/vorot93/Library/Application Support/io.parity.ethereum/cache", keys: "/Users/vorot93/Library/Application Support/io.parity.ethereum/keys", signer: "/Users/vorot93/Library/Application Support/io.parity.ethereum/signer", secretstore: "/Users/vorot93/Library/Application Support/io.parity.ethereum/secretstore" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It probably prints the old path because you have it on your disk already?

Ah, didn't realize the test was not sandboxed.

k, so maybe we can extend the unit tests to make sure we're doing the right thing on different platforms?

@vorot93 vorot93 added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. labels Apr 21, 2020
Comment thread accounts/ethstore/cli/src/main.rs Outdated
Comment thread util/dir/src/lib.rs Outdated
Comment thread util/dir/src/lib.rs
}

let mut root = data_root(t).ok()?;
root.push(if LOWERCASE { "openethereum" } else { "OpenEthereum" });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not use lowercase everywhere?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think the convention for AppData and Application Support is to use the original case.

@vorot93 vorot93 force-pushed the vorot93/new-path branch 2 times, most recently from 1ef9ab7 to e1f631e Compare April 23, 2020 11:23
Copy link
Copy Markdown
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Would be nice to have some sandbox tests as @dvdplm suggested, but I don't want to block the release. Tested this on linux, works as expected.

@ordian ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 23, 2020
@dvdplm dvdplm merged commit 74e9293 into master Apr 24, 2020
@dvdplm dvdplm deleted the vorot93/new-path branch April 24, 2020 18:05
dvdplm added a commit that referenced this pull request Apr 28, 2020
…rade-to-rocksdb-0.14

* dp/fix/memory-leak-when-warp-syncing:
  Drain the transaction overlay
  New default paths (#11641)
  Update EWF's chains with Istanbul transition block numbers (#11482)
  add openethereum supplementary bootnodes, those are not active right now, but will be in case the network needs more power (#11650)
  Remove Parity bootnodes (#11644)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants