Add a feature to create automatically a random temporary directory for base path & remove `Clone` #6221

cecton · 2020-06-02T17:26:08Z

Following up of this idea:

🤔 I wonder if it would be useful to add a commandline parameter "--tmp" that would create a random temporary directory for the base path automatically for you so you can copy-paste the same command 3 times if you want to run 3 nodes; and when the node exits, it would delete that temporary directory automatically.

So for a demo you dont need to give the --base-path if you need to run multiple nodes and you don't need to purge at all

Some people liked it

cc @JoshOrndorff @apopiak

Note: there were many Clone on the structopt. I think it was used in the original implementation before I refactored sc-cli but it is not needed anymore so I removed them.

Forked at: 342caad Parent branch: origin/master

…path

parity-cla-bot · 2020-06-02T17:26:11Z

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

Many thanks,

Parity Technologies CLA Bot

bkchr · 2020-06-03T08:06:02Z

client/cli/src/commands/run_cmd.rs

+	pub tmp: bool,
+
+	#[structopt(skip)]
+	temp_base_path: RefCell<Option<TempDir>>,


Nope. This is a really bad hack.

What part you don't like exactly? (so I can find another way)

Not having the temp base path as parameter here.

We could create some custom type that we return in base_path

Ok done! Now the TempDir is hold in the configuration object instead of the SubstrateCli object, then it's move to the service and dropped with it. Also the core of the feature itself has moved from sc-cli to sc-service.

bkchr · 2020-06-03T08:07:18Z

client/cli/src/commands/run_cmd.rs

+
+	/// Run a temporary node.
+	///
+	/// A random temporary directory will be created to store the configuration and will be deleted


"random temporary" => "temporary"

Please also make the user aware that this is the "base-path" and what will be part of this path. Database, node key etc.

done in 55ed555

Forked at: 342caad Parent branch: origin/master

Parent branch: origin/master Forked at: 342caad

bkchr

Already looks much better, the only problem is now that run_service_until_exit will drop the base path before everything has stopped. This could lead to errors on stopping the node.

bkchr · 2020-06-04T17:34:25Z

client/cli/src/commands/mod.rs

 			}

-			fn base_path(&self) -> $crate::Result<::std::option::Option<::std::path::PathBuf>> {
+			fn base_path(&self) -> $crate::Result<::std::option::Option<::sc_service::config::BasePath>> {


Suggested change

fn base_path(&self) -> $crate::Result<::std::option::Option<::sc_service::config::BasePath>> {

fn base_path(&self) -> $crate::Result<::std::option::Option<sc_service::config::BasePath>> {

Actually I think these are all wrong. sc-cli should probably make a hidden re-export of sc_service and the macro should get it from there.

Yes that is right.

I will make another PR for that for the sake of having isolated commits per change

client/cli/src/params/shared_params.rs

client/service/Cargo.toml

bkchr · 2020-06-04T17:50:07Z

client/service/src/lib.rs

 	keystore: sc_keystore::KeyStorePtr,
 	marker: PhantomData<TBl>,
 	prometheus_registry: Option<prometheus_endpoint::Registry>,
+	_base_path: Option<BasePath>,


Please add a comment why we need this here.

client/service/src/config.rs

bkchr · 2020-06-04T17:53:11Z

client/service/src/config.rs

+		))
+	}
+
+	/// Create a base path based on an existing path on disk.


I think we create it if it does not exists?

Not explicitly. We create only the network_config_dir which is a subdirectory. We could explicitly create the directories. Though this doc sentence actually meant "create a BasePath struct".

Than please write it. I did not understand it like that.

You're totally right. Done in cb140b6

client/service/src/config.rs

Co-authored-by: Bastian Köcher <[email protected]>

Forked at: 342caad Parent branch: origin/master

cecton · 2020-06-05T04:25:14Z

the only problem is now that run_service_until_exit will drop the base path before everything has stopped. This could lead to errors on stopping the node.

Fair point! 😓

Already looks much better

To be honest I kinda disagree. When I see that more things need to be handled in this run_service_until_exit I feel like we are doing something wrong. We should be removing things from there, not adding.

I wonder if we should remove the Future impl of Service and replace it with a function that takes &self so things in the service will not be dropped prematurely because we are awaiting for the completion of the service.

bkchr · 2020-06-05T21:25:07Z

Yeah, the situation with dropping the service is shit. That is right ;)

Parent branch: origin/master Forked at: 342caad

cecton added 2 commits June 2, 2020 18:58

Initial commit

7fbed1b

Forked at: 342caad Parent branch: origin/master

Add a feature to create automatically a temporary directory for base …

aa518e4

…path

cecton added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 2, 2020

cecton self-assigned this Jun 2, 2020

cecton added 3 commits June 2, 2020 19:27

doc fix and todos

5338633

use parking_lot instead

baf6ad2

use refcell instead since we stay in the main thread

179dda3

cecton changed the title ~~Add a feature to create automatically a random temporary directory for base path~~ Add a feature to create automatically a random temporary directory for base path & remove Clone Jun 3, 2020

cecton added 2 commits June 3, 2020 09:23

remove Clone derives

2aea88f

add test

b6358e8

cecton marked this pull request as ready for review June 3, 2020 07:44

bkchr suggested changes Jun 3, 2020

View reviewed changes

cecton added 9 commits June 3, 2020 10:18

solving dependency issue

f81522d

clarifying doc

55ed555

conflict argument with base-path

cfc6fa4

WIP

5a5c546

Forked at: 342caad Parent branch: origin/master

Merge commit f028a50 (no conflict)

bf1da7e

Parent branch: origin/master Forked at: 342caad

revert dep deletion

68dfe89

fixing test and making base_path optional

898dcd6

hold basepath while the service is running

d26da87

fixes

9c6f71a

bkchr reviewed Jun 4, 2020

View reviewed changes

cecton and others added 5 commits June 5, 2020 05:51

Update client/cli/src/params/shared_params.rs

9e989de

Co-authored-by: Bastian Köcher <[email protected]>

Update client/service/Cargo.toml

c811cb6

Co-authored-by: Bastian Köcher <[email protected]>

Update client/cli/src/commands/mod.rs

65cb074

Co-authored-by: Bastian Köcher <[email protected]>

Update client/service/src/config.rs

fdb3f20

Co-authored-by: Bastian Köcher <[email protected]>

WIP

39bc022

Forked at: 342caad Parent branch: origin/master

cecton added 4 commits June 8, 2020 09:28

improve doc

cb140b6

Merge commit 252b146 (no conflict)

ca42803

Parent branch: origin/master Forked at: 342caad

Merge commit 9ce0774 (conflicts)

468f16a

Parent branch: origin/master Forked at: 342caad

Merge commit d68cfd7 (no conflict)

49cfe00

Parent branch: origin/master Forked at: 342caad

cecton mentioned this pull request Jun 8, 2020

Companion PR for https://github.com/paritytech/substrate/pull/6221 paritytech/polkadot#1213

Merged

cecton requested a review from bkchr June 8, 2020 12:40

cecton mentioned this pull request Jun 9, 2020

Replace Service's future impl by not-self-consuming method #6306

Closed

bkchr approved these changes Jun 10, 2020

View reviewed changes

bkchr merged commit a684e83 into master Jun 10, 2020

bkchr deleted the cecton-tmp-base-path branch June 10, 2020 11:13

bkchr added B0-silent Changes should not be mentioned in any release notes B1-clientnoteworthy labels Jun 10, 2020

aurexav mentioned this pull request Jul 23, 2020

darwinia-cli Support Temporary Directory darwinia-network/darwinia-common#201

Closed

JoshOrndorff mentioned this pull request Apr 13, 2021

Add a command to purge the relay chain only paritytech/cumulus#306

Merged

	fn base_path(&self) -> $crate::Result<::std::option::Option<::sc_service::config::BasePath>> {
	fn base_path(&self) -> $crate::Result<::std::option::Option<sc_service::config::BasePath>> {

Add a feature to create automatically a random temporary directory for base path & remove Clone #6221

Add a feature to create automatically a random temporary directory for base path & remove Clone #6221

Uh oh!

Conversation

cecton commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecton Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cecton commented Jun 5, 2020

Uh oh!

bkchr commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add a feature to create automatically a random temporary directory for base path & remove `Clone` #6221

Add a feature to create automatically a random temporary directory for base path & remove `Clone` #6221

cecton commented Jun 2, 2020 •

edited

Loading

cecton Jun 4, 2020 •

edited

Loading