CLI API refactoring and improvement (#4692)

It changes the way we extended the CLI functionalities of substrate to allow more flexibility. (If this was not clear, here is another version: it changes the `sc_cli` API to allow more flexibility).

This touches a few important things:
 - the startup of the async task with tokei:
    This was in node and node-template and I moved it to substrate. The idea is to have 1 time the code that handles unix signals (SIGTERM and SIGINT) properly. It is however possible to make this more generic to wait for a future instead and provide only a helper for the basic handling of SIGTERM and SIGINT.
 - increased the version of structopt and tokei
 - no more use of structopt internal's API
 - less use of generics

Related to #4643 and https://github.com/paritytech/cumulus/pull/42: the implementation of "into_configuration" and "get_config" are similar but with better flexibility so it is now possible in cumulus to have the command-line arguments only of the run command for polkadot if we want

Related to https://github.com/paritytech/cumulus/issues/24 and https://github.com/paritytech/cumulus/issues/34 : it will now be possible to make a configuration struct for polkadot with some overrides of the default parameters much more easily.

substrate/bin/node/cli/Cargo.toml

@@ -31,7 +31,7 @@ hex-literal = "0.2.1"
 jsonrpc-core = "14.0.3"
 log = "0.4.8"
 rand = "0.7.2"
-structopt = "=0.3.7"
+structopt = { version = "0.3.8", optional = true }
 
 # primitives
 sp-authority-discovery = { version = "2.0.0",  path = "../../../primitives/authority-discovery" }
@@ -81,9 +81,7 @@ node-primitives = { version = "2.0.0", path = "../primitives" }
 node-executor = { version = "2.0.0", path = "../executor" }
 
 # CLI-specific dependencies
-tokio = { version = "0.2", features = ["rt-threaded"], optional = true }
 sc-cli = { version = "0.8.0", optional = true, path = "../../../client/cli" }
-ctrlc = { version = "3.1.3", features = ["termination"], optional = true }
 node-transaction-factory = { version = "0.8.0", optional = true, path = "../transaction-factory" }
 
 # WASM-specific dependencies
@@ -99,10 +97,19 @@ futures = "0.3.1"
 tempfile = "3.1.0"
 
 [build-dependencies]
-sc-cli = { version = "0.8.0", package = "sc-cli", path = "../../../client/cli" }
 build-script-utils = { version = "2.0.0", package = "substrate-build-script-utils", path = "../../../utils/build-script-utils" }
-structopt = "=0.3.7"
-vergen = "3.0.4"
+structopt = { version = "0.3.8", optional = true }
+node-transaction-factory = { version = "0.8.0", optional = true, path = "../transaction-factory" }
+
+[build-dependencies.sc-cli]
+version = "0.8.0"
+package = "sc-cli"
+path = "../../../client/cli"
+optional = true
+
+[build-dependencies.vergen]
+version = "3.0.4"
+optional = true
 
 [features]
 default = ["cli", "wasmtime"]
@@ -114,10 +121,10 @@ browser = [
 cli = [
 	"sc-cli",
 	"node-transaction-factory",
-	"tokio",
-	"ctrlc",
 	"sc-service/rocksdb",
 	"node-executor/wasmi-errno",
+	"vergen",
+	"structopt",
 ]
 wasmtime = [
 	"cli",

substrate/bin/node/cli/bin/main.rs

@@ -18,31 +18,8 @@
 
 #![warn(missing_docs)]
 
-use futures::channel::oneshot;
-use futures::{future, FutureExt};
 use sc_cli::VersionInfo;
 
-use std::cell::RefCell;
-
-// handles ctrl-c
-struct Exit;
-impl sc_cli::IntoExit for Exit {
-	type Exit = future::Map<oneshot::Receiver<()>, fn(Result<(), oneshot::Canceled>) -> ()>;
-	fn into_exit(self) -> Self::Exit {
-		// can't use signal directly here because CtrlC takes only `Fn`.
-		let (exit_send, exit) = oneshot::channel();
-
-		let exit_send_cell = RefCell::new(Some(exit_send));
-		ctrlc::set_handler(move || {
-			if let Some(exit_send) = exit_send_cell.try_borrow_mut().expect("signal handler not reentrant; qed").take() {
-				exit_send.send(()).expect("Error sending exit notification");
-			}
-		}).expect("Error setting Ctrl-C handler");
-
-		exit.map(|_| ())
-	}
-}
-
 fn main() -> Result<(), sc_cli::error::Error> {
 	let version = VersionInfo {
 		name: "Substrate Node",
@@ -52,7 +29,8 @@ fn main() -> Result<(), sc_cli::error::Error> {
 		author: "Parity Technologies <admin@parity.io>",
 		description: "Generic substrate node",
 		support_url: "https://github.com/paritytech/substrate/issues/new",
+		copyright_start_year: 2017,
 	};
 
-	node_cli::run(std::env::args(), Exit, version)
+	node_cli::run(std::env::args(), version)
 }

substrate/bin/node/cli/build.rs

@@ -14,39 +14,48 @@
 // 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::{fs, env, path::Path};
-use structopt::{StructOpt, clap::Shell};
-use sc_cli::{NoCustom, CoreParams};
-use vergen::{ConstantsFlags, generate_cargo_keys};
-
 fn main() {
-	build_shell_completion();
-	generate_cargo_keys(ConstantsFlags::all()).expect("Failed to generate metadata files");
-
-	build_script_utils::rerun_if_git_head_changed();
+	#[cfg(feature = "cli")]
+	cli::main();
 }
 
-/// Build shell completion scripts for all known shells
-/// Full list in https://github.com/kbknapp/clap-rs/blob/e9d0562a1dc5dfe731ed7c767e6cee0af08f0cf9/src/app/parser.rs#L123
-fn build_shell_completion() {
-	for shell in &[Shell::Bash, Shell::Fish, Shell::Zsh, Shell::Elvish, Shell::PowerShell] {
-		build_completion(shell);
+#[cfg(feature = "cli")]
+mod cli {
+	include!("src/cli.rs");
+
+	use std::{fs, env, path::Path};
+	use sc_cli::{structopt::clap::Shell};
+	use vergen::{ConstantsFlags, generate_cargo_keys};
+
+	pub fn main() {
+		build_shell_completion();
+		generate_cargo_keys(ConstantsFlags::all()).expect("Failed to generate metadata files");
+
+		build_script_utils::rerun_if_git_head_changed();
+	}
+
+	/// Build shell completion scripts for all known shells
+	/// Full list in https://github.com/kbknapp/clap-rs/blob/e9d0562a1dc5dfe731ed7c767e6cee0af08f0cf9/src/app/parser.rs#L123
+	fn build_shell_completion() {
+		for shell in &[Shell::Bash, Shell::Fish, Shell::Zsh, Shell::Elvish, Shell::PowerShell] {
+			build_completion(shell);
+		}
+	}
+
+	/// Build the shell auto-completion for a given Shell
+	fn build_completion(shell: &Shell) {
+		let outdir = match env::var_os("OUT_DIR") {
+			None => return,
+			Some(dir) => dir,
+		};
+		let path = Path::new(&outdir)
+			.parent().unwrap()
+			.parent().unwrap()
+			.parent().unwrap()
+			.join("completion-scripts");
+
+		fs::create_dir(&path).ok();
+
+		Cli::clap().gen_completions("substrate-node", *shell, &path);
 	}
 }
-
-/// Build the shell auto-completion for a given Shell
-fn build_completion(shell: &Shell) {
-	let outdir = match env::var_os("OUT_DIR") {
-		None => return,
-		Some(dir) => dir,
-	};
-	let path = Path::new(&outdir)
-		.parent().unwrap()
-		.parent().unwrap()
-		.parent().unwrap()
-		.join("completion-scripts");
-
-	fs::create_dir(&path).ok();
-
-	CoreParams::<NoCustom, NoCustom>::clap().gen_completions("substrate-node", *shell, &path);
-}

substrate/bin/node/cli/src/browser.rs

@@ -38,13 +38,13 @@ async fn start_inner(wasm_ext: Transport) -> Result<Client, Box<dyn std::error::
 	let chain_spec = ChainSpec::FlamingFir.load()
 		.map_err(|e| format!("{:?}", e))?;
 
-	let config: Configuration<(), _, _> = browser_configuration(wasm_ext, chain_spec)
+	let config: Configuration<_, _> = browser_configuration(wasm_ext, chain_spec)
 		.await?;
 
 	info!("Substrate browser node");
 	info!("  version {}", config.full_version());
 	info!("  by Parity Technologies, 2017-2020");
-	info!("Chain specification: {}", config.chain_spec.name());
+	info!("Chain specification: {}", config.expect_chain_spec().name());
 	info!("Node name: {}", config.name);
 	info!("Roles: {:?}", config.roles);
 

substrate/bin/node/cli/src/cli.rs

@@ -14,21 +14,26 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-pub use sc_cli::VersionInfo;
-use tokio::runtime::{Builder as RuntimeBuilder, Runtime};
-use sc_cli::{IntoExit, NoCustom, SharedParams, ImportParams, error};
-use sc_service::{AbstractService, Roles as ServiceRoles, Configuration};
-use log::info;
+use sc_cli::{SharedParams, ImportParams, RunCmd};
 use structopt::StructOpt;
-use sc_cli::{display_role, parse_and_prepare, GetSharedParams, ParseAndPrepare};
-use crate::{service, ChainSpec, load_spec};
-use crate::factory_impl::FactoryState;
-use node_transaction_factory::RuntimeAdapter;
-use futures::{channel::oneshot, future::{select, Either}};
 
-/// Custom subcommands.
 #[derive(Clone, Debug, StructOpt)]
-pub enum CustomSubcommands {
+#[structopt(settings = &[
+	structopt::clap::AppSettings::GlobalVersion,
+	structopt::clap::AppSettings::ArgsNegateSubcommands,
+	structopt::clap::AppSettings::SubcommandsNegateReqs,
+])]
+pub struct Cli {
+	#[structopt(subcommand)]
+	pub subcommand: Option<Subcommand>,
+	#[structopt(flatten)]
+	pub run: RunCmd,
+}
+
+#[derive(Clone, Debug, StructOpt)]
+pub enum Subcommand {
+	#[structopt(flatten)]
+	Base(sc_cli::Subcommand),
 	/// The custom factory subcommmand for manufacturing transactions.
 	#[structopt(
 		name = "factory",
@@ -38,14 +43,6 @@ pub enum CustomSubcommands {
 	Factory(FactoryCmd),
 }
 
-impl GetSharedParams for CustomSubcommands {
-	fn shared_params(&self) -> Option<&SharedParams> {
-		match self {
-			CustomSubcommands::Factory(cmd) => Some(&cmd.shared_params),
-		}
-	}
-}
-
 /// The `factory` command used to generate transactions.
 /// Please note: this command currently only works on an empty database!
 #[derive(Debug, StructOpt, Clone)]
@@ -87,125 +84,3 @@ pub struct FactoryCmd {
 	#[structopt(flatten)]
 	pub import_params: ImportParams,
 }
-
-/// Parse command line arguments into service configuration.
-pub fn run<I, T, E>(args: I, exit: E, version: sc_cli::VersionInfo) -> error::Result<()> where
-	I: IntoIterator<Item = T>,
-	T: Into<std::ffi::OsString> + Clone,
-	E: IntoExit,
-{
-	type Config<A, B> = Configuration<(), A, B>;
-
-	match parse_and_prepare::<CustomSubcommands, NoCustom, _>(&version, "substrate-node", args) {
-		ParseAndPrepare::Run(cmd) => cmd.run(load_spec, exit,
-		|exit, _cli_args, _custom_args, mut config: Config<_, _>| {
-			info!("{}", version.name);
-			info!("  version {}", config.full_version());
-			info!("  by Parity Technologies, 2017-2020");
-			info!("Chain specification: {}", config.chain_spec.name());
-			info!("Node name: {}", config.name);
-			info!("Roles: {}", display_role(&config));
-			let runtime = RuntimeBuilder::new()
-				.thread_name("main-tokio-")
-				.threaded_scheduler()
-				.enable_all()
-				.build()
-				.map_err(|e| format!("{:?}", e))?;
-			config.tasks_executor = {
-				let runtime_handle = runtime.handle().clone();
-				Some(Box::new(move |fut| { runtime_handle.spawn(fut); }))
-			};
-			match config.roles {
-				ServiceRoles::LIGHT => run_until_exit(
-					runtime,
-					service::new_light(config)?,
-					exit
-				),
-				_ => run_until_exit(
-					runtime,
-					service::new_full(config)?,
-					exit
-				),
-			}
-		}),
-		ParseAndPrepare::BuildSpec(cmd) => cmd.run::<NoCustom, _, _, _>(load_spec),
-		ParseAndPrepare::ExportBlocks(cmd) => cmd.run_with_builder(|config: Config<_, _>|
-			Ok(new_full_start!(config).0), load_spec, exit),
-		ParseAndPrepare::ImportBlocks(cmd) => cmd.run_with_builder(|config: Config<_, _>|
-			Ok(new_full_start!(config).0), load_spec, exit),
-		ParseAndPrepare::CheckBlock(cmd) => cmd.run_with_builder(|config: Config<_, _>|
-			Ok(new_full_start!(config).0), load_spec, exit),
-		ParseAndPrepare::PurgeChain(cmd) => cmd.run(load_spec),
-		ParseAndPrepare::RevertChain(cmd) => cmd.run_with_builder(|config: Config<_, _>|
-			Ok(new_full_start!(config).0), load_spec),
-		ParseAndPrepare::CustomCommand(CustomSubcommands::Factory(cli_args)) => {
-			let mut config: Config<_, _> = sc_cli::create_config_with_db_path(
-				load_spec,
-				&cli_args.shared_params,
-				&version,
-				None,
-			)?;
-			sc_cli::fill_import_params(
-				&mut config,
-				&cli_args.import_params,
-				ServiceRoles::FULL,
-				cli_args.shared_params.dev,
-			)?;
-
-			match ChainSpec::from(config.chain_spec.id()) {
-				Some(ref c) if c == &ChainSpec::Development || c == &ChainSpec::LocalTestnet => {},
-				_ => panic!("Factory is only supported for development and local testnet."),
-			}
-
-			let factory_state = FactoryState::new(
-				cli_args.mode.clone(),
-				cli_args.num,
-				cli_args.rounds,
-			);
-
-			let service_builder = new_full_start!(config).0;
-			node_transaction_factory::factory::<FactoryState<_>, _, _, _, _, _>(
-				factory_state,
-				service_builder.client(),
-				service_builder.select_chain()
-					.expect("The select_chain is always initialized by new_full_start!; QED")
-			).map_err(|e| format!("Error in transaction factory: {}", e))?;
-
-			Ok(())
-		}
-	}
-}
-
-fn run_until_exit<T, E>(
-	mut runtime: Runtime,
-	service: T,
-	e: E,
-) -> error::Result<()>
-where
-	T: AbstractService,
-	E: IntoExit,
-{
-	let (exit_send, exit) = oneshot::channel();
-
-	let informant = sc_cli::informant::build(&service);
-
-	let handle = runtime.spawn(select(exit, informant));
-
-	// we eagerly drop the service so that the internal exit future is fired,
-	// but we need to keep holding a reference to the global telemetry guard
-	let _telemetry = service.telemetry();
-
-	let exit = e.into_exit();
-	let service_res = runtime.block_on(select(service, exit));
-
-	let _ = exit_send.send(());
-
-	if let Err(e) = runtime.block_on(handle) {
-		log::error!("Error running node: {:?}", e);
-	}
-
-	match service_res {
-		Either::Left((res, _)) => res.map_err(error::Error::Service),
-		Either::Right((_, _)) => Ok(())
-	}
-}

substrate/bin/node/cli/src/command.rs

@@ -0,0 +1,82 @@
+// Copyright 2017-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 sc_cli::{VersionInfo, error};
+use sc_service::{Roles as ServiceRoles};
+use node_transaction_factory::RuntimeAdapter;
+use crate::{Cli, service, ChainSpec, load_spec, Subcommand, factory_impl::FactoryState};
+
+/// Parse command line arguments into service configuration.
+pub fn run<I, T>(args: I, version: sc_cli::VersionInfo) -> error::Result<()>
+where
+	I: Iterator<Item = T>,
+	T: Into<std::ffi::OsString> + Clone,
+{
+	let args: Vec<_> = args.collect();
+	let opt = sc_cli::from_iter::<Cli, _>(args.clone(), &version);
+
+	let mut config = sc_service::Configuration::default();
+	config.impl_name = "substrate-node";
+
+	match opt.subcommand {
+		None => sc_cli::run(
+			config,
+			opt.run,
+			service::new_light,
+			service::new_full,
+			load_spec,
+			&version,
+		),
+		Some(Subcommand::Factory(cli_args)) => {
+			sc_cli::init(&mut config, load_spec, &cli_args.shared_params, &version)?;
+
+			sc_cli::fill_import_params(
+				&mut config,
+				&cli_args.import_params,
+				ServiceRoles::FULL,
+				cli_args.shared_params.dev,
+			)?;
+
+			match ChainSpec::from(config.expect_chain_spec().id()) {
+				Some(ref c) if c == &ChainSpec::Development || c == &ChainSpec::LocalTestnet => {},
+				_ => panic!("Factory is only supported for development and local testnet."),
+			}
+
+			let factory_state = FactoryState::new(
+				cli_args.mode.clone(),
+				cli_args.num,
+				cli_args.rounds,
+			);
+
+			let service_builder = new_full_start!(config).0;
+			node_transaction_factory::factory::<FactoryState<_>, _, _, _, _, _>(
+				factory_state,
+				service_builder.client(),
+				service_builder.select_chain()
+					.expect("The select_chain is always initialized by new_full_start!; QED")
+			).map_err(|e| format!("Error in transaction factory: {}", e))?;
+
+			Ok(())
+		},
+		Some(Subcommand::Base(subcommand)) => sc_cli::run_subcommand(
+			config,
+			subcommand,
+			load_spec,
+			|config: service::NodeConfiguration| Ok(new_full_start!(config).0),
+			&version,
+		),
+	}
+}

substrate/bin/node/cli/src/lib.rs

@@ -39,11 +39,15 @@ mod browser;
 mod cli;
 #[cfg(feature = "cli")]
 mod factory_impl;
+#[cfg(feature = "cli")]
+mod command;
 
 #[cfg(feature = "browser")]
 pub use browser::*;
 #[cfg(feature = "cli")]
 pub use cli::*;
+#[cfg(feature = "cli")]
+pub use command::*;
 
 /// The chain specification option.
 #[derive(Clone, Debug, PartialEq)]

substrate/bin/node/cli/src/service.rs

@@ -274,10 +274,10 @@ type ConcreteTransactionPool = sc_transaction_pool::BasicPool<
 >;
 
 /// A specialized configuration object for setting up the node..
-pub type NodeConfiguration<C> = Configuration<C, GenesisConfig, crate::chain_spec::Extensions>;
+pub type NodeConfiguration = Configuration<GenesisConfig, crate::chain_spec::Extensions>;
 
 /// Builds a new service for a full client.
-pub fn new_full<C: Send + Default + 'static>(config: NodeConfiguration<C>)
+pub fn new_full(config: NodeConfiguration)
 -> Result<
 	Service<
 		ConcreteBlock,
@@ -299,7 +299,7 @@ pub fn new_full<C: Send + Default + 'static>(config: NodeConfiguration<C>)
 }
 
 /// Builds a new service for a light client.
-pub fn new_light<C: Send + Default + 'static>(config: NodeConfiguration<C>)
+pub fn new_light(config: NodeConfiguration)
 -> Result<impl AbstractService, ServiceError> {
 	type RpcExtension = jsonrpc_core::IoHandler<sc_rpc::Metadata>;
 	let inherent_data_providers = InherentDataProviders::new();