CLI API refactoring and improvement #4692

cecton · 2020-01-20T17:20:05Z

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.
deprecates the use of the CLI without subcommand to run the node, the user should now use the subcommand "run" but the old behavior still work and will display a warning:
The fact that running the node is not a subcommand makes it difficult with structopt to extend properly. Using a proper subcommand for running the node simplifies the code and can allow a better separation of global arguments vs subcommand arguments
increased the version of structopt and tokei
no more use of structopt internal's API
less use of generics

Impacts to #4663: that ticket shouldn't be started before this one is finished. I already know the code so I know what will (not) work for the implementation. It's more complicated than it looks.

Related to #4643 and paritytech/cumulus#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 paritytech/cumulus#24 and paritytech/cumulus#34 : it will now be possible to make a configuration struct for polkadot with some overrides of the default parameters much more easily.

Your PR adheres the style guide

There is a bit of clean-up to do but the formatting should be correct. The clean-up is more in the name of the generics...

You bumped the runtime version if there are breaking changes in the runtime.

I'm not sure. Can the reviewer help me on that and check?

You updated any rustdocs which may have changed

I still need to add doc to the new exposed functions.

Has the PR altered the external API or interfaces used by Polkadot? Do you have the corresponding Polkadot PR ready?

Definitely alternates the CLI API. I updated node and node-template here but cumulus and polkadot will need to be updated too.

parity-cla-bot · 2020-01-20T17:20:10Z

It looks like @cecton signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

bkchr · 2020-01-20T17:42:57Z

deprecates the use of the CLI without subcommand to run the node, the user should now use the subcommand "run" but the old behavior still work and will display a warning:
The fact that running the node is not a subcommand makes it difficult with structopt to extend properly. Using a proper subcommand for running the node simplifies the code and can allow a better separation of global arguments vs subcommand arguments

This is intended behavior and was discussed already before. If the user does not specifies any subcommand, it is automatically run. And run should not be accepted as valid sub command.

tomaka · 2020-01-20T19:35:07Z

client/cli/src/lib.rs

+) -> error::Result<()>
+where
+	F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
+	F2: FnOnce(Configuration<G, E>) -> Result<T1, sc_service::error::Error>,


I'm really not a fan of this callback style where we're hard-coding the fact that new_full and new_light take a configuration and return a service.

The reason for not using callbacks originally is so that users can straight up do a function call to new_full or new_light, which to me is much more intuitive than having to pass a closure that is then called by something else. It also makes it more obvious what exactly happens and in which order.

If later we want to pass another parameter to new_full, what are we going to do? Pass this parameter to run so that it is then passed to the callback?

Yes this PR reduces code duplication, but IMO it's not a good thing here.

But... but... ☝️ It was already a closure/callback before. I didn't change that ❓ what do you mean❓ https://github.com/paritytech/substrate/blob/master/bin/node/cli/src/cli.rs#L101

Oh, just because it's in master doesn't mean I like it 😅
Of course that's just my opinion, but as the one who has done the previous refactoring here, I was trying to remove these callbacks-like structure. The one you linked to was hard to remove at the time, for a reason I can't recall.

haha 😁 ok! Actually I just tried to remove the callback and use a trait instead (so you can override functions in the trait) but it's actually very complicated because of the return types: new_full and new_start return 2 different things in bin/node: one is Service and the other is impl AbstractService...

I couldn't find a solution to that but in any case: I'm trying also to reduce the complexity of the signatures.

If later we want to pass another parameter to new_full, what are we going to do? Pass this parameter to run so that it is then passed to the callback?

See #4692 (comment) as I think it also answer that particular issue

tomaka · 2020-01-20T19:44:58Z

I believe this change is incompatible with #4688, as we have to create the tokio runtime and modify the Configuration before creating the Service.

cecton · 2020-01-21T06:43:10Z

I believe this change is incompatible with #4688, as we have to create the tokio runtime and modify the Configuration before creating the Service.

I think you're right. It's also useful for cumulus because we have to start 2 nodes so we need a better control over the runtime. I will try to improve this.

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

cecton · 2020-01-30T07:57:01Z

(Yet another conflict resolved. Also something that was already fixed with this PR....)

Let's finish this PR already please. What remains to get your approval?

client/cli/src/lib.rs

client/service/src/config.rs

bkchr

Looks good, some last nitpicks.

bkchr · 2020-01-30T09:41:26Z

(Yet another conflict resolved. Also something that was already fixed with this PR....)

Let's finish this PR already please. What remains to get your approval?

Before asking such question, I expect that every review feedback is either worked in or a comment is added why not.

Found at least 3 comments that you ignored.

cecton · 2020-01-30T09:49:20Z

What remains to get your approval?

Before asking such question, I expect that every review feedback is either worked in or a comment is added why not.

Found at least 3 comments that you ignored.

Obviously I didn't ignore them (I'm not "that" evil) but rather I didn't see them. I check on github + on the emails for the hidden ones but I guess it is possible I missed some. 🙏 thanks for your review

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

tomaka · 2020-01-30T10:09:35Z

I haven't taken a look at the changes since my previous review, but it L'edGTM at the time, so +1 for merging.

Said LGTM already

@bkchr

These are a few changes I missed during the refactoring. 1. Initialization issue and boilerplate Most importantly: part of the `Configuration` initialization was done in `sc_cli::init`. This means the user can not benefit from this initialization boilerplate if they have multiple `Configuration` since `sc_cli::init` can only be called once. 2. Boilerplate for `VersionInfo` and `Configuration` I'm also answering to the critic of @bkchr on the initialization using version: https://github.com/paritytech/substrate/pull/4692/files/bea809d4c14a2ede953227ac885e3b3f9771c548#r372047238 This will allow initializing a `Configuration` and provide the version by default. 3. Loading the `chain_spec` explicitly In the past it was done automatically but in some cases we want to delay this. I moved the code to `Configuration.load_spec()` so it can be called later on. `chain_spec` can also be written directly to the `Configuration` without using this `load_spec` helper. 4. [deleted] 5. Fixing issue that prevents the user to override the port In the refactoring I introduced a bug by mistake that could potentially prevent the CLI user to override the ports if defaults where provided for these ports (only on cumulus). 6. Change task_executor from Box to Arc This is useful for cumulus where we have 2 nodes with 2 separate Configuration that need to spawn tasks to the same runtime. 7. Renamed TasksExecutorRequired to TaskExecutor For consistency. This is related to paritytech/cumulus#24 This is the continuation (and hopefully the end of) #4692

cecton added 17 commits January 13, 2020 11:39

Expose a method that allows converting RunCmd to Configuration

5721bab

WIP

e533791

WIP

ecaeb7f

WIP

f6c249b

WIP

9405756

WIP

99159d2

WIP

904b292

WIP

5c684a0

WIP

85b2d11

WIP

c8581ad

WIP

1a9c594

WIP

dae1972

WIP

214076d

WIP

f98c3a4

WIP

963d33a

WIP

3a1bc54

WIP

fda5a4c

cecton requested review from tomaka, bkchr and expenses January 20, 2020 17:20

cecton self-assigned this Jan 20, 2020

cecton added the I7-refactor Code needs refactoring. label Jan 20, 2020

tomaka added the B2-breaksapi label Jan 20, 2020

This was referenced Jan 20, 2020

(some) node subcommands are broken #4623

Closed

Splitting relaychain and parachain command-line arguments paritytech/cumulus#42

Merged

tomaka reviewed Jan 20, 2020

View reviewed changes

cecton and others added 9 commits January 29, 2020 14:39

Update client/cli/src/params.rs

292ec6e

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

ec78b9c

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

505f574

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

3ffe543

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

03cc75f

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

d126cb5

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/params.rs

54ac33d

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

WIP

4fd3edf

WIP

6276b76

cecton mentioned this pull request Jan 29, 2020

Add binaries to Github Releases #2534

Open

7 tasks

cecton added 2 commits January 29, 2020 16:54

Merge remote-tracking branch 'origin/master' into cecton-cli-refactoring

6c62acc

Merge remote-tracking branch 'origin/master' into cecton-cli-refactoring

e543ebe

bkchr reviewed Jan 30, 2020

View reviewed changes

client/cli/src/lib.rs Outdated Show resolved Hide resolved

client/cli/src/lib.rs Outdated Show resolved Hide resolved

client/cli/src/lib.rs Outdated Show resolved Hide resolved

client/cli/src/lib.rs Outdated Show resolved Hide resolved

client/service/src/config.rs Show resolved Hide resolved

bkchr approved these changes Jan 30, 2020

View reviewed changes

cecton and others added 3 commits January 30, 2020 10:49

Update client/cli/src/lib.rs

6424b53

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update client/cli/src/lib.rs

7774621

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

WIP

9c09d17

cecton merged commit 26e37ff into master Jan 30, 2020

cecton deleted the cecton-cli-refactoring branch January 30, 2020 10:40

expenses mentioned this pull request Feb 1, 2020

Fix cli/browser.rs and allow selecting a chainspec on wasm. paritytech/polkadot#815

Merged

cecton mentioned this pull request Feb 3, 2020

CLI improvements & fixes #4812

Merged

cecton added B1-apinoteworthy and removed A5-grumble labels Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI API refactoring and improvement #4692

CLI API refactoring and improvement #4692

cecton commented Jan 20, 2020 •

edited

Loading

parity-cla-bot commented Jan 20, 2020

bkchr commented Jan 20, 2020

tomaka Jan 20, 2020 •

edited

Loading

cecton Jan 20, 2020

tomaka Jan 21, 2020

cecton Jan 21, 2020

cecton Jan 21, 2020 •

edited

Loading

tomaka commented Jan 20, 2020

cecton commented Jan 21, 2020

cecton commented Jan 30, 2020

bkchr left a comment

bkchr commented Jan 30, 2020

cecton commented Jan 30, 2020

tomaka commented Jan 30, 2020 •

edited

Loading

CLI API refactoring and improvement #4692

CLI API refactoring and improvement #4692

Conversation

cecton commented Jan 20, 2020 • edited Loading

parity-cla-bot commented Jan 20, 2020

bkchr commented Jan 20, 2020

tomaka Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

cecton Jan 20, 2020

Choose a reason for hiding this comment

tomaka Jan 21, 2020

Choose a reason for hiding this comment

cecton Jan 21, 2020

Choose a reason for hiding this comment

cecton Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

tomaka commented Jan 20, 2020

cecton commented Jan 21, 2020

cecton commented Jan 30, 2020

bkchr left a comment

Choose a reason for hiding this comment

bkchr commented Jan 30, 2020

cecton commented Jan 30, 2020

tomaka commented Jan 30, 2020 • edited Loading

cecton commented Jan 20, 2020 •

edited

Loading

tomaka Jan 20, 2020 •

edited

Loading

cecton Jan 21, 2020 •

edited

Loading

tomaka commented Jan 30, 2020 •

edited

Loading