Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Move config path generation into the service config for reusability #3978

Merged
merged 8 commits into from
Nov 1, 2019
101 changes: 26 additions & 75 deletions core/cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,18 +307,33 @@ pub struct ParseAndPrepareBuildSpec<'a> {

impl<'a> ParseAndPrepareBuildSpec<'a> {
/// Runs the command and build the chain specs.
pub fn run<G, S, E>(
pub fn run<C, G, S, E>(
self,
spec_factory: S
) -> error::Result<()> where
S: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
C: Default,
G: RuntimeGenesis,
E: ChainSpecExtension,
{
info!("Building chain spec");
let raw_output = self.params.raw;
let mut spec = load_spec(&self.params.shared_params, spec_factory)?;
with_default_boot_node(&mut spec, self.params, self.version)?;

if spec.boot_nodes().is_empty() && !self.params.disable_default_bootnode {
let base_path = base_path(&self.params.shared_params, self.version);
let config = service::Configuration::<C,_,_>::default_with_spec_and_base_path(spec.clone(), base_path);
let node_key = node_key_config(self.params.node_key_params, &Some(config.network_path()))?;
let keys = node_key.into_keypair()?;
let peer_id = keys.public().into_peer_id();
let addr = build_multiaddr![
Ip4([127, 0, 0, 1]),
Tcp(30333u16),
P2p(peer_id)
];
spec.add_boot_node(addr)
}

let json = service::chain_ops::build_spec(spec, raw_output)?;

print!("{}", json);
Expand Down Expand Up @@ -558,16 +573,13 @@ fn fill_transaction_pool_configuration<C, G, E>(
/// Fill the given `NetworkConfiguration` by looking at the cli parameters.
fn fill_network_configuration(
cli: NetworkConfigurationParams,
base_path: &Path,
chain_spec_id: &str,
config_path: PathBuf,
config: &mut NetworkConfiguration,
client_id: String,
is_dev: bool,
) -> error::Result<()> {
config.boot_nodes.extend(cli.bootnodes.into_iter());
config.config_path = Some(
network_path(&base_path, chain_spec_id).to_string_lossy().into()
);
config.config_path = Some(config_path.to_string_lossy().into());
config.net_config_path = config.config_path.clone();
config.reserved_nodes.extend(cli.reserved_nodes.into_iter());

Expand Down Expand Up @@ -642,7 +654,8 @@ where
S: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
{
let spec = load_spec(&cli.shared_params, spec_factory)?;
let mut config = service::Configuration::default_with_spec(spec.clone());
let base_path = base_path(&cli.shared_params, &version);
let mut config = service::Configuration::default_with_spec_and_base_path(spec.clone(), base_path);

fill_config_keystore_password(&mut config, &cli)?;

Expand All @@ -666,14 +679,10 @@ where
)?
}

let base_path = base_path(&cli.shared_params, version);

config.keystore_path = cli.keystore_path.unwrap_or_else(
|| keystore_path(&base_path, config.chain_spec.id())
);
config.keystore_path = cli.keystore_path.unwrap_or_else(|| config.in_chain_config_dir("keystore"));

config.database = DatabaseConfig::Path {
path: db_path(&base_path, config.chain_spec.id()),
path: config.database_path(),
Copy link
Contributor

@tomaka tomaka Nov 1, 2019

Choose a reason for hiding this comment

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

I don't understand the change yet. I'm commenting in a hurry because you're about to merge.

This line is kind of a red flag. We store config.database_path() in config.database.path.

Copy link
Contributor Author

@gnunicorn gnunicorn Nov 1, 2019

Choose a reason for hiding this comment

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

the general change of this PR is to move the generation of relative paths from the internal *_path(&base_path-style onto the configuration object. database_path() there is only a handy alias for self.base_path.clone().join("database") on config.

What exactly do you consider the red flag?

cache_size: cli.database_cache_size,
};
config.state_cache_size = cli.state_cache_size;
Expand Down Expand Up @@ -740,8 +749,7 @@ where
let client_id = config.client_id();
fill_network_configuration(
cli.network_config,
&base_path,
spec.id(),
config.network_path(),
&mut config.network,
client_id,
is_dev,
Expand Down Expand Up @@ -792,39 +800,6 @@ where
Ok(config)
}

//
// IANA unassigned port ranges that we could use:
// 6717-6766 Unassigned
// 8504-8553 Unassigned
// 9556-9591 Unassigned
// 9803-9874 Unassigned
// 9926-9949 Unassigned

fn with_default_boot_node<G, E>(
spec: &mut ChainSpec<G, E>,
cli: BuildSpecCmd,
version: &VersionInfo,
) -> error::Result<()>
where
G: RuntimeGenesis,
E: ChainSpecExtension,
{
if spec.boot_nodes().is_empty() && !cli.disable_default_bootnode {
let base_path = base_path(&cli.shared_params, version);
let storage_path = network_path(&base_path, spec.id());
let node_key = node_key_config(cli.node_key_params, &Some(storage_path))?;
let keys = node_key.into_keypair()?;
let peer_id = keys.public().into_peer_id();
let addr = build_multiaddr![
Ip4([127, 0, 0, 1]),
Tcp(30333u16),
P2p(peer_id)
];
spec.add_boot_node(addr)
}
Ok(())
}

/// Creates a configuration including the database path.
pub fn create_config_with_db_path<C, G, E, S>(
spec_factory: S, cli: &SharedParams, version: &VersionInfo,
Expand All @@ -838,9 +813,9 @@ where
let spec = load_spec(cli, spec_factory)?;
let base_path = base_path(cli, version);

let mut config = service::Configuration::default_with_spec(spec.clone());
let mut config = service::Configuration::default_with_spec_and_base_path(spec.clone(), base_path);
config.database = DatabaseConfig::Path {
path: db_path(&base_path, spec.id()),
path: config.database_path(),
cache_size: None,
};

Expand All @@ -866,30 +841,6 @@ fn parse_address(
Ok(address)
}

fn keystore_path(base_path: &Path, chain_id: &str) -> PathBuf {
let mut path = base_path.to_owned();
path.push("chains");
path.push(chain_id);
path.push("keystore");
path
}

fn db_path(base_path: &Path, chain_id: &str) -> PathBuf {
let mut path = base_path.to_owned();
path.push("chains");
path.push(chain_id);
path.push("db");
path
}

fn network_path(base_path: &Path, chain_id: &str) -> PathBuf {
let mut path = base_path.to_owned();
path.push("chains");
path.push(chain_id);
path.push("network");
path
}

fn init_logger(pattern: &str) {
use ansi_term::Colour;

Expand Down
2 changes: 1 addition & 1 deletion core/cli/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ impl<CC, RP> GetLogFilter for CoreParams<CC, RP> where CC: GetLogFilter {

/// A special commandline parameter that expands to nothing.
/// Should be used as custom subcommand/run arguments if no custom values are required.
#[derive(Clone, Debug)]
#[derive(Clone, Debug, Default)]
pub struct NoCustom {}

impl StructOpt for NoCustom {
Expand Down
29 changes: 27 additions & 2 deletions core/service/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ pub struct Configuration<C, G, E = NoExtension> {
pub transaction_pool: transaction_pool::txpool::Options,
/// Network configuration.
pub network: NetworkConfiguration,
/// Path to the base configuration directory.
pub config_dir: PathBuf,
/// Path to key files.
pub keystore_path: PathBuf,
/// Configuration for the database.
Expand Down Expand Up @@ -118,13 +120,14 @@ impl<C, G, E> Configuration<C, G, E> where
G: RuntimeGenesis,
E: Extension,
{
/// Create default config for given chain spec.
pub fn default_with_spec(chain_spec: ChainSpec<G, E>) -> Self {
/// Create a default config for given chain spec and path to configuration dir
pub fn default_with_spec_and_base_path(chain_spec: ChainSpec<G, E>, config_dir: PathBuf) -> Self {
let mut configuration = Configuration {
impl_name: "parity-substrate",
impl_version: "0.0.0",
impl_commit: "",
chain_spec,
config_dir,
name: Default::default(),
roles: Roles::FULL,
transaction_pool: Default::default(),
Expand Down Expand Up @@ -161,6 +164,9 @@ impl<C, G, E> Configuration<C, G, E> where
configuration
}

}

impl<C, G, E> Configuration<C, G, E> {
/// Returns full version string of this configuration.
pub fn full_version(&self) -> String {
full_version_from_strs(self.impl_version, self.impl_commit)
Expand All @@ -170,6 +176,25 @@ impl<C, G, E> Configuration<C, G, E> where
pub fn client_id(&self) -> String {
format!("{}/v{}", self.impl_name, self.full_version())
}

/// Generate a PathBuf to sub in the chain configuration directory
pub fn in_chain_config_dir(&self, sub: &str) -> PathBuf {
let mut path = self.config_dir.clone();
path.push("chains");
path.push(self.chain_spec.id());
path.push(sub);
path
}

/// The basepath for the db directory
pub fn database_path(&self) -> PathBuf {
self.in_chain_config_dir("db")
}

/// The basepath for the network file
pub fn network_path(&self) -> PathBuf {
self.in_chain_config_dir("network")
}
}

/// Returns platform info
Expand Down
1 change: 1 addition & 0 deletions core/service/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ fn node_config<G, E: Clone> (
network: network_config,
keystore_path: root.join("key"),
keystore_password: None,
config_dir: root.clone(),
database: DatabaseConfig::Path {
path: root.join("db"),
cache_size: None
Expand Down
2 changes: 1 addition & 1 deletion node-template/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn run<I, T, E>(args: I, exit: E, version: VersionInfo) -> error::Result<()>
),
}
}),
ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
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<_>|
Expand Down
2 changes: 1 addition & 1 deletion node/cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub fn run<I, T, E>(args: I, exit: E, version: substrate_cli::VersionInfo) -> er
),
}
}),
ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
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<_, _>|
Expand Down