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

Merge node, node-cli, node-executor, node-rpc-client into one #3192

Closed
wants to merge 10 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jul 24, 2019

Close #2934

Merges as much as possible in the Substrate node. The objective is to simplify and clean up a bit the architecture.

Moves the code of node-cli and node-executor in node/src.
I moved the main executable in a src/bin subdirectory instead of leaving it in src.
I initially wanted to node-rpc-client in src/bin as well, but if you do that then cargo run requires being passed which binary to run.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Jul 24, 2019
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I would like to see the root Cargo.toml to be cleaner.

@@ -10,13 +14,70 @@ build = "build.rs"
edition = "2018"

[dependencies]
cli = { package = "node-cli", path = "node/cli" }
log = "0.4"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, don't we maybe want to clean this up and just keep the members in the root Cargo.toml.

The following should help for cargo run:
rust-lang/cargo#7032

(I'm not sure if this already included in stable or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not in stable.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just wait and postpone until this is in stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean around 2022

Copy link
Member

Choose a reason for hiding this comment

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

This is already stabilized, so I would assume that it is released with the next rust release? rust-lang/cargo#7056

@tomaka tomaka added A1-onice and removed A0-please_review Pull request needs code review. labels Jul 24, 2019
use runtime_primitives::traits::{Header as HeaderT, Hash as HashT};
use runtime_primitives::{generic::Era, ApplyOutcome, ApplyError, ApplyResult, Perbill};
use runtime_primitives::weights::{WeightMultiplier, SimpleDispatchInfo, WeighData};
use sr_primitives::traits::{Header as HeaderT, Hash as HashT};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am quite happy to see some else also changing these --not wrong but confusing-- imports; it is quite annoying that we have core/sr-primitives imported into different crates with different names; makes it even more confusing that we have an actual runtime-primitives in node.

I have some thoughts on codifying imports a bit and adding it to our style guide maybe. Will make an issue for it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge the multiple node subdirectories into one crate
3 participants