Fix CLI setup again (#4851)

* Fix CLI setup again

We need to set `config_dir` and `database_path` for almost every
command.
This fixes `purge-chain` and also adds a test to make sure we don't
break it again.

* Adds missing test files

* Split methods

bin/node/cli/src/command.rs

@@ -41,7 +41,7 @@ where
 		),
 		Some(Subcommand::Factory(cli_args)) => {
 			sc_cli::init(&cli_args.shared_params, &version)?;
-			sc_cli::load_spec(&mut config, &cli_args.shared_params, load_spec)?;
+			sc_cli::init_config(&mut config, &cli_args.shared_params, &version, load_spec)?;
 			sc_cli::fill_import_params(
 				&mut config,
 				&cli_args.import_params,

bin/node/cli/tests/common.rs

@@ -0,0 +1,34 @@
+// Copyright 2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+use std::{process::{Child, ExitStatus}, thread, time::Duration};
+
+/// Wait for the given `child` the given ammount of `secs`.
+///
+/// Returns the `Some(exit status)` or `None` if the process did not finish in the given time.
+pub fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
+	for _ in 0..secs {
+		match child.try_wait().unwrap() {
+			Some(status) => return Some(status),
+			None => thread::sleep(Duration::from_secs(1)),
+		}
+	}
+	eprintln!("Took to long to exit. Killing...");
+	let _ = child.kill();
+	child.wait().unwrap();
+
+	None
+}

bin/node/cli/tests/purge_chain_works.rs

@@ -0,0 +1,53 @@
+// Copyright 2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+use assert_cmd::cargo::cargo_bin;
+use std::{convert::TryInto, process::Command, thread, time::Duration, fs, path::PathBuf};
+
+mod common;
+
+#[test]
+#[cfg(unix)]
+fn purge_chain_works() {
+	use nix::sys::signal::{kill, Signal::SIGINT};
+	use nix::unistd::Pid;
+
+	let base_path = "purge_chain_test";
+
+	let _ = fs::remove_dir_all(base_path);
+	let mut cmd = Command::new(cargo_bin("substrate"))
+		.args(&["--dev", "-d", base_path])
+		.spawn()
+		.unwrap();
+
+	// Let it produce some blocks.
+	thread::sleep(Duration::from_secs(30));
+	assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
+
+	// Stop the process
+	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
+	assert!(common::wait_for(&mut cmd, 30).map(|x| x.success()).unwrap_or_default());
+
+	let status = Command::new(cargo_bin("substrate"))
+		.args(&["purge-chain", "--dev", "-d", base_path, "-y"])
+		.status()
+		.unwrap();
+	assert!(status.success());
+
+	// Make sure that the `dev` chain folder exists, but the `db` is deleted.
+	assert!(PathBuf::from(base_path).join("chains/dev/").exists());
+	assert!(!PathBuf::from(base_path).join("chains/dev/db").exists());
+}

bin/node/cli/tests/running_the_node_and_interrupt.rs

@@ -15,38 +15,28 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use assert_cmd::cargo::cargo_bin;
-use std::convert::TryInto;
-use std::process::{Child, Command, ExitStatus};
-use std::thread::sleep;
-use std::time::Duration;
+use std::{convert::TryInto, process::Command, thread, time::Duration, fs};
+
+mod common;
 
 #[test]
 #[cfg(unix)]
 fn running_the_node_works_and_can_be_interrupted() {
 	use nix::sys::signal::{kill, Signal::{self, SIGINT, SIGTERM}};
 	use nix::unistd::Pid;
 
-	fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
-		for _ in 0..secs {
-			match child.try_wait().unwrap() {
-				Some(status) => return Some(status),
-				None => sleep(Duration::from_secs(1)),
-			}
-		}
-		eprintln!("Took to long to exit. Killing...");
-		let _ = child.kill();
-		child.wait().unwrap();
-
-		None
-	}
-
 	fn run_command_and_kill(signal: Signal) {
-		let mut cmd = Command::new(cargo_bin("substrate")).spawn().unwrap();
-		sleep(Duration::from_secs(30));
+		let _ = fs::remove_dir_all("interrupt_test");
+		let mut cmd = Command::new(cargo_bin("substrate"))
+			.args(&["--dev", "-d", "interrupt_test"])
+			.spawn()
+			.unwrap();
+
+		thread::sleep(Duration::from_secs(30));
 		assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
 		kill(Pid::from_raw(cmd.id().try_into().unwrap()), signal).unwrap();
 		assert_eq!(
-			wait_for(&mut cmd, 30).map(|x| x.success()),
+			common::wait_for(&mut cmd, 30).map(|x| x.success()),
 			Some(true),
 			"the pocess must exit gracefully after signal {}",
 			signal,

client/cli/src/lib.rs

@@ -234,7 +234,7 @@ where
 	SF: AbstractService + Unpin,
 {
 	init(&run_cmd.shared_params, version)?;
-	load_spec(&mut config, &run_cmd.shared_params, spec_factory)?;
+	init_config(&mut config, &run_cmd.shared_params, version, spec_factory)?;
 	run_cmd.run(config, new_light, new_full, version)
 }
 
@@ -257,19 +257,18 @@ where
 	<BB as BlockT>::Hash: std::str::FromStr,
 {
 	let shared_params = subcommand.get_shared_params();
-
 	init(shared_params, version)?;
-	load_spec(&mut config, shared_params, spec_factory)?;
+	init_config(&mut config, shared_params, version, spec_factory)?;
 	subcommand.run(config, builder)
 }
 
 /// Initialize substrate. This must be done only once.
 ///
 /// This method:
 ///
-/// 1.  set the panic handler
-/// 2.  raise the FD limit
-/// 3.  initialize the logger
+/// 1. Set the panic handler
+/// 2. Raise the FD limit
+/// 3. Initialize the logger
 pub fn init(shared_params: &SharedParams, version: &VersionInfo) -> error::Result<()> {
 	let full_version = sc_service::config::full_version_from_strs(
 		version.version,
@@ -283,6 +282,37 @@ pub fn init(shared_params: &SharedParams, version: &VersionInfo) -> error::Resul
 	Ok(())
 }
 
+/// Initialize the given `config`.
+///
+/// This will load the chain spec, set the `config_dir` and the `database_dir`.
+pub fn init_config<G, E, F>(
+	config: &mut Configuration<G, E>,
+	shared_params: &SharedParams,
+	version: &VersionInfo,
+	spec_factory: F,
+) -> error::Result<()> where
+	F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
+	G: RuntimeGenesis,
+	E: ChainSpecExtension,
+{
+	load_spec(config, shared_params, spec_factory)?;
+
+	if config.config_dir.is_none() {
+		config.config_dir = Some(base_path(&shared_params, version));
+	}
+
+	if config.database.is_none() {
+		config.database = Some(DatabaseConfig::Path {
+			path: config
+				.in_chain_config_dir(DEFAULT_DB_CONFIG_PATH)
+				.expect("We provided a base_path/config_dir."),
+			cache_size: None,
+		});
+	}
+
+	Ok(())
+}
+
 /// Run the node
 ///
 /// Builds and runs either a full or a light node, depending on the `role` within the `Configuration`.
@@ -514,26 +544,10 @@ where
 pub fn update_config_for_running_node<G, E>(
 	mut config: &mut Configuration<G, E>,
 	cli: RunCmd,
-	version: &VersionInfo,
 ) -> error::Result<()>
 where
 	G: RuntimeGenesis,
 {
-	if config.config_dir.is_none() {
-		config.config_dir = Some(base_path(&cli.shared_params, version));
-	}
-
-	if config.database.is_none() {
-		// NOTE: the loading of the DatabaseConfig is voluntarily delayed to here
-		//       in case config.config_dir has been customized
-		config.database = Some(DatabaseConfig::Path {
-			path: config
-				.in_chain_config_dir(DEFAULT_DB_CONFIG_PATH)
-				.expect("We provided a base_path/config_dir."),
-			cache_size: None,
-		});
-	}
-
 	fill_config_keystore_password_and_path(&mut config, &cli)?;
 
 	let keyring = cli.get_keyring();
@@ -790,7 +804,6 @@ mod tests {
 			update_config_for_running_node(
 				&mut node_config,
 				run_cmds.clone(),
-				TEST_VERSION_INFO,
 			).unwrap();
 
 			let expected_path = match keystore_path {
@@ -843,8 +856,14 @@ mod tests {
 		let cli = RunCmd::from_iter(args);
 
 		let mut config = Configuration::new(TEST_VERSION_INFO);
-		config.chain_spec = Some(chain_spec);
-		update_config_for_running_node(&mut config, cli, TEST_VERSION_INFO).unwrap();
+		init(&cli.shared_params, &TEST_VERSION_INFO).unwrap();
+		init_config(
+			&mut config,
+			&cli.shared_params,
+			&TEST_VERSION_INFO,
+			|_| Ok(Some(chain_spec)),
+		).unwrap();
+		update_config_for_running_node(&mut config, cli).unwrap();
 
 		assert!(config.config_dir.is_some());
 		assert!(config.database.is_some());

client/cli/src/params.rs

@@ -933,11 +933,7 @@ impl RunCmd {
 	{
 		assert!(config.chain_spec.is_some(), "chain_spec must be present before continuing");
 
-		crate::update_config_for_running_node(
-			&mut config,
-			self,
-			&version,
-		)?;
+		crate::update_config_for_running_node(&mut config, self)?;
 
 		crate::run_node(config, new_light, new_full, &version)
 	}