fix: build spec experience #331

AlexD10S · 2024-11-01T15:40:27Z

Fixes #305

Changes

Add the flag --chain to specify the chain specification. Is to add this flag: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/cli/src/params/shared_params.rs#L32 when running the build-spec command of the binary.
To generate pop-node chain_spec mainnet for example can be done with:
pop build spec --release --id 4024 --type live --relay paseo --chain mainnet
(https://github.com/r0gue-io/pop-node/blob/main/node/src/command.rs#L73)
As pointed in the issue the --default-bootnode was a bit confusing because by default was set to true and to don't include localhost as the default bootnode has to be specified with --default-bootnode false.
Set by default to false and included if specified --default-bootnode.
The other approach is to change the name to --disable-default-bootnode and set it by default to false unless the flag is included.

codecov · 2024-11-01T15:54:08Z

Codecov Report

Attention: Patch coverage is 81.43564% with 150 lines in your changes missing coverage. Please review.

Project coverage is 74.29%. Comparing base (d1cf48d) to head (3cd8194).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/pop-cli/src/commands/build/spec.rs	81.49%	52 Missing and 42 partials ⚠️
crates/pop-cli/src/cli.rs	46.34%	22 Missing ⚠️
crates/pop-parachains/src/build.rs	75.86%	4 Missing and 10 partials ⚠️
crates/pop-cli/src/commands/build/mod.rs	66.66%	8 Missing and 4 partials ⚠️
crates/pop-cli/src/commands/build/parachain.rs	66.66%	0 Missing and 4 partials ⚠️
crates/pop-common/src/manifest.rs	95.08%	0 Missing and 3 partials ⚠️
crates/pop-common/src/build.rs	98.18%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   72.59%   74.29%   +1.69%     
==========================================
  Files          56       56              
  Lines       10700    11233     +533     
  Branches    10700    11233     +533     
==========================================
+ Hits         7768     8345     +577     
+ Misses       1844     1746      -98     
- Partials     1088     1142      +54

Files with missing lines	Coverage Δ
crates/pop-cli/src/commands/call/contract.rs	`83.40% <100.00%> (+0.05%)`	⬆️
crates/pop-cli/src/commands/mod.rs	`16.32% <100.00%> (ø)`
crates/pop-common/src/build.rs	`98.48% <98.18%> (-1.52%)`	⬇️
crates/pop-common/src/manifest.rs	`93.51% <95.08%> (+0.30%)`	⬆️
crates/pop-cli/src/commands/build/parachain.rs	`73.49% <66.66%> (+0.32%)`	⬆️
crates/pop-cli/src/commands/build/mod.rs	`50.00% <66.66%> (-0.71%)`	⬇️
crates/pop-parachains/src/build.rs	`88.17% <75.86%> (-0.72%)`	⬇️
crates/pop-cli/src/cli.rs	`65.46% <46.34%> (-1.90%)`	⬇️
crates/pop-cli/src/commands/build/spec.rs	`73.64% <81.49%> (+73.64%)`	⬆️

brunopgalvao · 2024-11-05T03:30:40Z

Looking good. Some notes:

--release option is confusing, what does this do? It says: "For production, always build in release mode to exclude debug features", so will the pop build spec --release build the parachain in release mode?

I think --release should be removed and the default should be so that when the user runs pop build spec it will build in release mode by default.

pop build spec --release --id 4024 --type live --relay paseo --chain mainnet

For some reason this does not work, not sure if it is a pop-cli issue or a pop-node issue, also something to keep in mind, mainnet on pop-node uses the polkadot relay, so I guess the user would have to be aware to use the appropriate flags to generate the chain spec.

AlexD10S · 2024-11-05T09:36:22Z

Looking good. Some notes:

--release option is confusing, what does this do? It says: "For production, always build in release mode to exclude debug features", so will the pop build spec --release build the parachain in release mode?

I think --release should be removed and the default should be so that when the user runs pop build spec it will build in release mode by default.
pop build spec --release --id 4024 --type live --relay paseo --chain mainnet 
For some reason this does not work, not sure if it is a pop-cli issue or a pop-node issue, also something to keep in mind, mainnet on pop-node uses the polkadot relay, so I guess the user would have to be aware to use the appropriate flags to generate the chain spec.

You are right about the --release flag been confusing, similar as the problem above with --default-bootnode flag, it has the default_value to true. I like your suggestion, I deprecated it the release flag and it builds in RELEASE mode by default.

About this: pop build spec --release --id 4024 --type live --relay paseo --chain mainnet not working, you probably have an old version of pop-node. See https://github.com/r0gue-io/pop-node/blob/main/node/src/command.rs#L73
You can check if your build has the mainnet available running:./target/release/pop-node build-spec --chain mainnet

evilrobot-01

Looks good, would just appreciate some context in terms of the deprecation of release option. Why not leave the flexibility?

Would also want @al3mart to look this PR over before being merged.

crates/pop-cli/src/commands/build/spec.rs

AlexD10S · 2024-11-05T17:30:16Z

Looks good, would just appreciate some context in terms of the deprecation of release option. Why not leave the flexibility?

Would also want @al3mart to look this PR over before being merged.

My apologies for not mentioning it in the PR description. The deprecation of the --release flag was implemented in response to a feedback comment #331 (comment) on the flag being confusing.

The changes were made in a single commit 37bb25d. so will be easy to revert if we decide to keep it. What are your thoughts, @al3mart? A better approach suggested?

crates/pop-cli/src/commands/build/spec.rs

al3mart

build spec might trigger a compilation. I feel that having the option of declaring if we want the profile to be release or debug can be convenient .

If we didn't have the flag anywhere else I think removing it might make more sense.
The flag was added mainly so the options for builds were consistent across sub commands
Maybe we can improve the doc as a way to clarify why that flag is present..

AlexD10S · 2024-11-05T20:57:48Z

build spec might trigger a compilation. I feel that having the option of declaring if we want the profile to be release or debug can be convenient .

If we didn't have the flag anywhere else I think removing it might make more sense. The flag was added mainly so the options for builds were consistent across sub commands Maybe we can improve the doc as a way to clarify why that flag is present..

You are right, It makes sense to keep it consistent across the comment. I removed the deprecation message and added more information in the docs to clarify that this command builds the node if it hasn't been built yet.

brunopgalvao · 2024-11-06T03:01:00Z

build spec might trigger a compilation. I feel that having the option of declaring if we want the profile to be release or debug can be convenient .

If we didn't have the flag anywhere else I think removing it might make more sense. The flag was added mainly so the options for builds were consistent across sub commands Maybe we can improve the doc as a way to clarify why that flag is present..

What is the difference in the outputted chain spec artifacts when using debug vs release? Can the runtime wasm be different?

brunopgalvao · 2024-11-06T08:44:01Z

About this: pop build spec --release --id 4024 --type live --relay paseo --chain mainnet not working, you probably have an old version of pop-node. See https://github.com/r0gue-io/pop-node/blob/main/node/src/command.rs#L73
You can check if your build has the mainnet available running:./target/release/pop-node build-spec --chain mainnet

Yes you are right. Since there are quite a few options for this command that are needed, it would be nice to have an error if for example --chain mainnet does not exist instead of the CLI doing nothing which is what it currently does.

AlexD10S · 2024-11-06T14:06:34Z

Yes you are right. Since there are quite a few options for this command that are needed, it would be nice to have an error if for example --chain mainnet does not exist instead of the CLI doing nothing which is what it currently does.

Yes, makes sense. Logs of the command ./target/release/pop-node build-spec are hidden, maybe we should show them if there is an error: #337

al3mart · 2024-11-06T17:48:07Z

build spec might trigger a compilation. I feel that having the option of declaring if we want the profile to be release or debug can be convenient .
If we didn't have the flag anywhere else I think removing it might make more sense. The flag was added mainly so the options for builds were consistent across sub commands Maybe we can improve the doc as a way to clarify why that flag is present..

What is the difference in the outputted chain spec artifacts when using debug vs release? Can the runtime wasm be different?

Regarding the resulting wasm I'd say that the only possible difference is that if it is not built in release it may contain some extra debug information that can be logged. Release also tries to compile with optimization. This comes to mind - https://paritytech.github.io/polkadot-sdk/master/frame_support/derive.RuntimeDebugNoBound.html

crates/pop-cli/src/commands/build/spec.rs

al3mart

I think the change on default value for --default-bootnode makes sense. I can see how that was confusing.

Love the addition of --chain. Was definitely something needed.

crates/pop-cli/src/commands/build/spec.rs

evilrobot-01

I think the multi/spinner can be improved a little quite easily.

crates/pop-cli/src/commands/build/spec.rs

evilrobot-01 · 2024-11-19T10:26:48Z

As mentioned separately, there is no consideration for builds using the production profile, which is potentially an important use case. Whilst this may have no effect on the resulting wasm, it did mean a rebuild of the node despite me already having a production build at target/production. This obviously is a waste of time and local storage and also might be frustrating to an end user.

This might be an edge case, but I think the tooling should ideally cater to both novice and professional devs.

evilrobot-01

All good, thanks!

evilrobot-01 · 2024-11-21T20:07:08Z

As mentioned separately, there is no consideration for builds using the production profile, which is potentially an important use case. Whilst this may have no effect on the resulting wasm, it did mean a rebuild of the node despite me already having a production build at target/production. This obviously is a waste of time and local storage and also might be frustrating to an end user.

Related to this, the same happens if you have a binary built already using the default debug profile and then run the build spec command - it rebuilds it all with the release profile as that is defaulted to true. Purely based on UX, might it be best to allow the user to either specify via --debug (-d), --release (-r), --profile name (-p) and if not specified, simply add a prompt with these same options?

al3mart

LGTM.

Looking forward to merging this one!

crates/pop-cli/src/commands/build/spec.rs

al3mart

The new commits after my last approval seem to make sense. Thanks for addressing those!

* fix: add chain to specify the chain specification * fix: default_bootnode by default to true * chore: fmt * chore: deprecate flag --release in build specs * fix: clean output (#334) * fix: undo deprecation of --release flag * refactor: small fix * style: remove extra space * fix(spec): better handling of spinner * style: use spinner instead of multispinner * docs: help message to include build * feat: reuse existing chain spec * refactor: remove clone * refactor: opt in to edit provided chain spec * docs: improve * refactor: flow flag input * fix: prepare_output_path * refactor: resolve small improvements * fix: protocol id prompt * fix: spinner * fix: docs * test: test cli * chore: refactor * chore: amend test * feat: production profile * refactor: improve profile experience * chore: feedback and rebase * chore: add profile tests * fix(test): parachain_lifecycle * style: fmt * fix: clippy * fix: cli required changes introduced by PR * fix: test * fix: clippy * docs: deprecation message --------- Co-authored-by: Alejandro Martinez Andres <[email protected]> Co-authored-by: Daanvdplas <[email protected]>

AlexD10S requested a review from brunopgalvao November 4, 2024 10:47

AlexD10S marked this pull request as ready for review November 5, 2024 09:37

evilrobot-01 requested a review from al3mart November 5, 2024 12:50

evilrobot-01 reviewed Nov 5, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

al3mart reviewed Nov 5, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Show resolved Hide resolved

al3mart reviewed Nov 5, 2024

View reviewed changes

AlexD10S requested review from al3mart and evilrobot-01 November 5, 2024 22:16

AlexD10S mentioned this pull request Nov 6, 2024

chore: improve error handling for pop build specs #337

Open

evilrobot-01 reviewed Nov 6, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

brunopgalvao approved these changes Nov 7, 2024

View reviewed changes

AlexD10S force-pushed the fix/build-spec branch from a27cb19 to a1abb01 Compare November 7, 2024 22:10

al3mart approved these changes Nov 8, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

AlexD10S requested a review from evilrobot-01 November 11, 2024 08:35

evilrobot-01 requested changes Nov 19, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

evilrobot-01 approved these changes Nov 21, 2024

View reviewed changes

AlexD10S mentioned this pull request Nov 27, 2024

feat: reuse existing chain spec #351

Merged

AlexD10S added 2 commits December 4, 2024 13:40

fix: add chain to specify the chain specification

4fbb674

fix: default_bootnode by default to true

eaf1f94

Daanvdplas and others added 15 commits December 4, 2024 13:41

fix: prepare_output_path

d164332

refactor: resolve small improvements

db65ebf

fix: protocol id prompt

505535f

fix: spinner

47ed21b

fix: docs

5fd5cb5

test: test cli

1c01334

chore: refactor

faf3038

chore: amend test

b80b5bb

feat: production profile

fab0584

refactor: improve profile experience

91fa45e

chore: feedback and rebase

c3898b0

chore: add profile tests

7deea0d

fix(test): parachain_lifecycle

05c0fcc

style: fmt

0fca3d9

fix: clippy

39b26ed

Daanvdplas force-pushed the fix/build-spec branch from 6ba8d0f to f0bc925 Compare December 4, 2024 14:19

fix: cli required changes introduced by PR

204296a

Daanvdplas force-pushed the fix/build-spec branch from f0bc925 to 204296a Compare December 4, 2024 14:23

fix: test

daf0983

al3mart approved these changes Dec 4, 2024

View reviewed changes

AlexD10S commented Dec 5, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Outdated Show resolved Hide resolved

AlexD10S commented Dec 5, 2024

View reviewed changes

crates/pop-cli/src/commands/build/spec.rs Show resolved Hide resolved

Daanvdplas added 2 commits December 5, 2024 17:23

fix: clippy

68a9065

docs: deprecation message

3cd8194

Daanvdplas self-requested a review December 5, 2024 17:39

Daanvdplas approved these changes Dec 5, 2024

View reviewed changes

al3mart approved these changes Dec 5, 2024

View reviewed changes

AlexD10S merged commit 3f86d18 into main Dec 5, 2024
20 checks passed

AlexD10S deleted the fix/build-spec branch December 5, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: build spec experience #331

fix: build spec experience #331

AlexD10S commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

brunopgalvao commented Nov 5, 2024

AlexD10S commented Nov 5, 2024 •

edited

Loading

evilrobot-01 left a comment

AlexD10S commented Nov 5, 2024

al3mart left a comment

AlexD10S commented Nov 5, 2024

brunopgalvao commented Nov 6, 2024 •

edited

Loading

brunopgalvao commented Nov 6, 2024

AlexD10S commented Nov 6, 2024

al3mart commented Nov 6, 2024

al3mart left a comment

evilrobot-01 left a comment

evilrobot-01 commented Nov 19, 2024

evilrobot-01 left a comment

evilrobot-01 commented Nov 21, 2024

al3mart left a comment

al3mart left a comment

fix: build spec experience #331

fix: build spec experience #331

Conversation

AlexD10S commented Nov 1, 2024 • edited Loading

Changes

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

brunopgalvao commented Nov 5, 2024

AlexD10S commented Nov 5, 2024 • edited Loading

evilrobot-01 left a comment

Choose a reason for hiding this comment

AlexD10S commented Nov 5, 2024

al3mart left a comment

Choose a reason for hiding this comment

AlexD10S commented Nov 5, 2024

brunopgalvao commented Nov 6, 2024 • edited Loading

brunopgalvao commented Nov 6, 2024

AlexD10S commented Nov 6, 2024

al3mart commented Nov 6, 2024

al3mart left a comment

Choose a reason for hiding this comment

evilrobot-01 left a comment

Choose a reason for hiding this comment

evilrobot-01 commented Nov 19, 2024

evilrobot-01 left a comment

Choose a reason for hiding this comment

evilrobot-01 commented Nov 21, 2024

al3mart left a comment

Choose a reason for hiding this comment

al3mart left a comment

Choose a reason for hiding this comment

AlexD10S commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

AlexD10S commented Nov 5, 2024 •

edited

Loading

brunopgalvao commented Nov 6, 2024 •

edited

Loading