CLI improvements & fixes

bin/node-template/src/command.rs

@@ -25,8 +25,7 @@ pub fn run(version: VersionInfo) -> error::Result<()>
 {
 	let opt = sc_cli::from_args::<Cli>(&version);
 
-	let mut config = sc_service::Configuration::default();
-	config.impl_name = "node-template";
+	let mut config = sc_service::Configuration::new(&version);
 
 	match opt.subcommand {
 		Some(subcommand) => sc_cli::run_subcommand(

bin/node/cli/src/command.rs

@@ -28,8 +28,7 @@ where
 	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";
+	let mut config = sc_service::Configuration::new(&version);
 
 	match opt.subcommand {
 		None => sc_cli::run(
@@ -41,8 +40,8 @@ where
 			&version,
 		),
 		Some(Subcommand::Factory(cli_args)) => {
-			sc_cli::init(&mut config, load_spec, &cli_args.shared_params, &version)?;
-
+			sc_cli::init(&cli_args.shared_params, &version)?;
+			sc_cli::load_spec(&mut config, &cli_args.shared_params, load_spec)?;
 			sc_cli::fill_import_params(
 				&mut config,
 				&cli_args.import_params,

client/cli/src/lib.rs

@@ -35,6 +35,7 @@ use sc_service::{
 	RuntimeGenesis, ChainSpecExtension, PruningMode, ChainSpec,
 	AbstractService, Roles as ServiceRoles,
 };
+pub use sc_service::config::{VersionInfo};
 use sc_network::{
 	self,
 	multiaddr::Protocol,
@@ -79,26 +80,18 @@ const DEFAULT_KEYSTORE_CONFIG_PATH : &'static str =  "keystore";
 /// The maximum number of characters for a node name.
 const NODE_NAME_MAX_LENGTH: usize = 32;
 
-/// Executable version. Used to pass version information from the root crate.
-#[derive(Clone)]
-pub struct VersionInfo {
-	/// Implementaiton name.
-	pub name: &'static str,
-	/// Implementation version.
-	pub version: &'static str,
-	/// SCM Commit hash.
-	pub commit: &'static str,
-	/// Executable file name.
-	pub executable_name: &'static str,
-	/// Executable file description.
-	pub description: &'static str,
-	/// Executable file author.
-	pub author: &'static str,
-	/// Support URL.
-	pub support_url: &'static str,
-	/// Copyright starting year (x-current year)
-	pub copyright_start_year: i32,
-}
+#[cfg(test)]
+#[doc(hidden)]
+pub const TEST_VERSION_INFO: &'static VersionInfo = &VersionInfo {
+	name: "node-test",
+	version: "0.1.0",
+	commit: "some_commit",
+	executable_name: "node-test",
+	description: "description",
+	author: "author",
+	support_url: "http://example.org",
+	copyright_start_year: 2020,
+};
 
 fn get_chain_key(cli: &SharedParams) -> String {
 	match cli.chain {
@@ -120,8 +113,12 @@ fn generate_node_name() -> String {
 	result
 }
 
-/// Load spec give shared params and spec factory.
-pub fn load_spec<F, G, E>(cli: &SharedParams, factory: F) -> error::Result<ChainSpec<G, E>> where
+/// Load spec to `Configuration` from shared params and spec factory.
+pub fn load_spec<'a, G, E, F>(
+	mut config: &'a mut Configuration<G, E>,
+	cli: &SharedParams,
+	factory: F,
+) -> error::Result<&'a ChainSpec<G, E>> where
 	G: RuntimeGenesis,
 	E: ChainSpecExtension,
 	F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
@@ -131,7 +128,13 @@ pub fn load_spec<F, G, E>(cli: &SharedParams, factory: F) -> error::Result<Chain
 		Some(spec) => spec,
 		None => ChainSpec::from_json_file(PathBuf::from(chain_key))?
 	};
-	Ok(spec)
+
+	config.network.boot_nodes = spec.boot_nodes().to_vec();
+	config.telemetry_endpoints = spec.telemetry_endpoints().clone();
+
+	config.chain_spec = Some(spec);
+
+	Ok(config.chain_spec.as_ref().unwrap())
 }
 
 fn base_path(cli: &SharedParams, version: &VersionInfo) -> PathBuf {
@@ -243,8 +246,8 @@ where
 	SL: AbstractService + Unpin,
 	SF: AbstractService + Unpin,
 {
-	init(&mut config, spec_factory, &run_cmd.shared_params, version)?;
-
+	init(&run_cmd.shared_params, version)?;
+	load_spec(&mut config, &run_cmd.shared_params, spec_factory)?;
 	run_cmd.run(config, new_light, new_full, version)
 }
 
@@ -266,30 +269,21 @@ where
 	<<<BB as BlockT>::Header as HeaderT>::Number as std::str::FromStr>::Err: std::fmt::Debug,
 	<BB as BlockT>::Hash: std::str::FromStr,
 {
-	init(&mut config, spec_factory, &subcommand.get_shared_params(), version)?;
+	let shared_params = subcommand.get_shared_params();
 
+	init(shared_params, version)?;
+	load_spec(&mut config, shared_params, spec_factory)?;
 	subcommand.run(config, builder)
 }
 
-/// Initialize substrate and its configuration
+/// Initialize substrate. This must be done only once.
 ///
 /// This method:
 ///
 /// 1.  set the panic handler
 /// 2.  raise the FD limit
 /// 3.  initialize the logger
-/// 4.  update the configuration provided with the chain specification, config directory,
-///     information (version, commit), database's path, boot nodes and telemetry endpoints
-pub fn init<G, E, F>(
-	mut config: &mut Configuration<G, E>,
-	spec_factory: F,
-	shared_params: &SharedParams,
-	version: &VersionInfo,
-) -> error::Result<()>
-where
-	G: RuntimeGenesis,
-	E: ChainSpecExtension,
-	F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
+pub fn init(shared_params: &SharedParams, version: &VersionInfo) -> error::Result<()>
 {
 	let full_version = sc_service::config::full_version_from_strs(
 		version.version,
@@ -300,21 +294,6 @@ where
 	fdlimit::raise_fd_limit();
 	init_logger(shared_params.log.as_ref().map(|v| v.as_ref()).unwrap_or(""));
 
-	config.chain_spec = Some(load_spec(shared_params, spec_factory)?);
-	config.config_dir = Some(base_path(shared_params, version));
-	config.impl_commit = version.commit;
-	config.impl_version = version.version;
-
-	config.database = DatabaseConfig::Path {
-		path: config
-			.in_chain_config_dir(DEFAULT_DB_CONFIG_PATH)
-			.expect("We provided a base_path/config_dir."),
-		cache_size: None,
-	};
-
-	config.network.boot_nodes = config.expect_chain_spec().boot_nodes().to_vec();
-	config.telemetry_endpoints = config.expect_chain_spec().telemetry_endpoints().clone();
-
 	Ok(())
 }
 
@@ -496,10 +475,8 @@ pub fn fill_import_params<G, E>(
 where
 	G: RuntimeGenesis,
 {
-	match config.database {
-		DatabaseConfig::Path { ref mut cache_size, .. } =>
-			*cache_size = Some(cli.database_cache_size),
-		DatabaseConfig::Custom(_) => {},
+	if let Some(DatabaseConfig::Path { ref mut cache_size, .. }) = config.database {
+		*cache_size = Some(cli.database_cache_size);
 	}
 
 	config.state_cache_size = cli.state_cache_size;
@@ -549,14 +526,30 @@ where
 	Ok(())
 }
 
-/// Update and prepare a `Configuration` with command line parameters of `RunCmd`
+/// Update and prepare a `Configuration` with command line parameters of `RunCmd` and `VersionInfo`
 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();
@@ -625,16 +618,16 @@ where
 			}
 		});
 
-	if config.rpc_http.is_none() {
+	if config.rpc_http.is_none() || cli.rpc_port.is_some() {
 		let rpc_interface: &str = interface_str(cli.rpc_external, cli.unsafe_rpc_external, cli.validator)?;
 		config.rpc_http = Some(parse_address(&format!("{}:{}", rpc_interface, 9933), cli.rpc_port)?);
 	}
-	if config.rpc_ws.is_none() {
+	if config.rpc_ws.is_none() || cli.ws_port.is_some() {
 		let ws_interface: &str = interface_str(cli.ws_external, cli.unsafe_ws_external, cli.validator)?;
 		config.rpc_ws = Some(parse_address(&format!("{}:{}", ws_interface, 9944), cli.ws_port)?);
 	}
 
-	if config.grafana_port.is_none() {
+	if config.grafana_port.is_none() || cli.grafana_port.is_some() {
 		let grafana_interface: &str = if cli.grafana_external { "0.0.0.0" } else { "127.0.0.1" };
 		config.grafana_port = Some(
 			parse_address(&format!("{}:{}", grafana_interface, 9955), cli.grafana_port)?
@@ -693,7 +686,8 @@ fn interface_str(
 	}
 }
 
-fn parse_address(
+/// Parse an address and optionally set the port to `port` if provided in argument.
+pub fn parse_address(
 	address: &str,
 	port: Option<u16>,
 ) -> Result<SocketAddr, String> {
@@ -805,6 +799,7 @@ mod tests {
 			update_config_for_running_node(
 				&mut node_config,
 				run_cmds.clone(),
+				TEST_VERSION_INFO,
 			).unwrap();
 
 			let expected_path = match keystore_path {

client/cli/src/params.rs

@@ -936,6 +936,7 @@ impl RunCmd {
 		crate::update_config_for_running_node(
 			&mut config,
 			self,
+			&version,
 		)?;
 
 		crate::run_node(config, new_light, new_full, &version)
@@ -1003,7 +1004,7 @@ impl ExportBlocksCmd {
 
 		crate::fill_config_keystore_in_memory(&mut config)?;
 
-		if let DatabaseConfig::Path { ref path, .. } = &config.database {
+		if let DatabaseConfig::Path { ref path, .. } = config.expect_database() {
 			info!("DB path: {}", path.display());
 		}
 		let from = self.from.as_ref().and_then(|f| f.parse().ok()).unwrap_or(1);
@@ -1124,7 +1125,7 @@ impl PurgeChainCmd {
 
 		crate::fill_config_keystore_in_memory(&mut config)?;
 
-		let db_path = match config.database {
+		let db_path = match config.expect_database() {
 			DatabaseConfig::Path { path, .. } => path,
 			_ => {
 				eprintln!("Cannot purge custom database implementation");

client/service/src/builder.rs

@@ -196,7 +196,7 @@ fn new_full_parts<TBl, TRtApi, TExecDisp, TGen, TCSExt>(
 			state_cache_child_ratio:
 			config.state_cache_child_ratio.map(|v| (v, 100)),
 			pruning: config.pruning.clone(),
-			source: match &config.database {
+			source: match config.expect_database() {
 				DatabaseConfig::Path { path, cache_size } =>
 					sc_client_db::DatabaseSettingsSrc::Path {
 						path: path.clone(),
@@ -307,7 +307,7 @@ where TGen: RuntimeGenesis, TCSExt: Extension {
 				state_cache_child_ratio:
 					config.state_cache_child_ratio.map(|v| (v, 100)),
 				pruning: config.pruning.clone(),
-				source: match &config.database {
+				source: match config.expect_database() {
 					DatabaseConfig::Path { path, cache_size } =>
 						sc_client_db::DatabaseSettingsSrc::Path {
 							path: path.clone(),