templates: make node compilation optional #5954

iulianbarbu · 2024-10-07T15:44:47Z

Description

Integration

Node devs that rely on templates' nodes binaries for minimal or parachain would need to follow the updated templates' README.mds again to find how to build the nodes' binaries.

Review Notes

Conditional compilation of virtual workspaces would compile the members list as if we passed --workspace flag to cargo build , except when adding a default-members list which will be used for any cargo command executed in the virtual workspace root. To build the full members list needs passing --workspace flag.

Other options investigated:

feature guard the node crate by defining a feature in the node crate, but it feels too complex since all code needs to be feature guarded. I haven't tried it but technically speaking it might work. I think though it looks awkward and my opinion is that the alternative is better.
defining features in the virtual workspace's Cargo.toml doesn't work (thought that I might create a feature that will have a dependency on the node crate and then not passing the feature to cargo build results in ignoring the node crate)
skipping compilation by using an environment variable, read in the build script, that will exit compilation abruptly if not set, but I couldn't make it work.
exclude the crate from the members list and build it specifically by passing --package minimal-template-node flag to the cargo build command. This has the disadvantage of not allowing IDEs based on rust analyzer to index/compile the node crate.

My conclusion is that any option would require two commands to build the template, one with the node and one without, and both must be included in the README or templates usage documentation. If it comes which ones to pick I am in favor of the default-members option, which requires minimal intervention and expresses how cargo commands are executed on top of the workspace members, and what's left out from regular usage.

Testing

Testing was conducted as described bellow:

zombienet with minimal-template-node , parachain-template-node and polkadot-omni-node. Things work as expected.
no chopsticks testing was conducted - feels a bit out of scope for OmniNode related docs and overall testing when promoting it over the templates' nodes.
testing the changes for the sync templates workflow (ignore the added comment from the Cargo.tomls, it was removed here on this branch: 99bff3e): minimal, parachain, solochain. The links correspond to PRs opened by a bot after manually starting the sync-templates workflow on paritytech-stg org to test the end result of the Cargo.toml changes.

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

This increases CI time but for a good reason. We want to ensure parachains pick up the finalized block info from the relay chain. Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

…ake-node-compilation-optional

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

This reverts commit dae9b73.

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

kianenigma · 2024-11-01T10:19:48Z

substrate/bin/utils/chain-spec-builder/Cargo.toml

@@ -21,15 +21,28 @@ path = "bin/main.rs"
 name = "chain-spec-builder"

 [lib]
-crate-type = ["rlib"]
+# Docs tests are not needed since the code samples that would be executed


I am not sure why this is needed; anything in docify::embed already has a ignore (for the opposite, you have docify::embed_run!).

It is about compiling test snippet not running it.

docify::embed still compiles the snippet, and since we use the trick with bash! macro (which is inaccessible in doc context), compilation would fail.

Adding on top of what @michalkucharczyk said, docify::embed in context of markdown will use rust lang instead of ignore, which will make the sample a doc test. It might insert ignore accordingly for general public/crate-level docs from .rs files (which I haven't tested), so this can be considered a bug. Also, embedding in markdown files can not use embed_run. I tried that previously and generating the readme fails with an error at compile time.

Also, adding something like below in the README (to disable the doc test), will disable the embedding:

\`\`\`ignore  \`\`\`

substrate/bin/utils/chain-spec-builder/Cargo.toml

kianenigma · 2024-11-01T10:26:18Z

substrate/bin/utils/chain-spec-builder/tests/test.rs

@@ -28,6 +31,53 @@ const DUMMY_PATH: &str = "fake-runtime-path";

 const OUTPUT_FILE: &str = "/tmp/chain_spec_builder.test_output_file.json";

+const RUNTIME_PATH: &str = unwrap_option_str(substrate_test_runtime::WASM_BINARY_PATH);


Follow-up:

there is some code in multiple places in polkadot-sdk that reads the cargo metadata, finds the target dir, and from there, tries to find the wasm path.

If we have this new const exported by substrate-wasm-builder, it can likely simplify that code.

Example from my code:

https://github.com/iulianbarbu/polkadot-sdk/blob/8b4884dc8a9d60d09d61b08ada0e645121c0d6b8/docs/sdk/src/guides/your_first_node.rs#L146-L159

cc @pepoviola and @pgherveou I think I have seen this in your recent PRs.

templates/minimal/node/src/cli.rs

templates/minimal/zombienet.toml

kianenigma · 2024-11-01T10:43:20Z

substrate/bin/utils/chain-spec-builder/tests/test.rs

+#[docify::export]
+fn cmd_display_preset() -> String {
+	bash!(
+		chain-spec-builder display-preset -r $RUNTIME_PATH -p "staging"


How does $RUNTIME_PATH get resolved here?

https://docs.rs/cmd_lib/latest/cmd_lib/index.html#intuitive-parameters-passing

kianenigma · 2024-11-01T10:46:40Z

templates/parachain/README.md


 ```sh
-cargo build --release
+cargo build -p parachain-template-runtime --release


Suggested change

cargo build -p parachain-template-runtime --release

cargo build --release

Right? it should be the same.

just nit picking in the overall direction that cargo build should just build the runtime.

Done 51c1814.

kianenigma · 2024-11-01T10:54:57Z

templates/parachain/README.md


 ```sh
-cargo install --path node
+polkadot-omni-node --chain <path/to/chain_spec.json> 


The OmniNode setup here should not demonstrate a broken command, instead it should:

explain that a parachain cannot progress without relay chain, which we can only setup with ZN

use --dev-block-time --tmp here.

Done: 51c1814

kianenigma · 2024-11-01T10:59:45Z

templates/zombienet/tests/smoke.rs

+		Ok(())
+	}
+
+	#[tokio::test(flavor = "multi_thread")]


@pepoviola PTAL 🙏

kianenigma · 2024-11-01T11:25:19Z

templates/parachain/README.md


 ```sh
-docker build . -t polkadot-sdk-parachain-template
+chain-spec-builder create --relay-chain "rococo-local" --para-id 1000 --runtime \
+    <target/release/wbuild/path/to/minimal-template-runtime.wasm> named-preset development


Suggested change

<target/release/wbuild/path/to/minimal-template-runtime.wasm> named-preset development

./target/release/wbuild/parachain-template-runtime/parachain_template_runtime.wasm named-preset development

This path can be corrected to what will be its true value, as it is predictable

Done: 51c1814

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

kianenigma

LGTM post addressing open comments and getting the CI to be green.

This is a great work, thanks @iulianbarbu

I think there has been a lot of learning here: how to properly do good READMEs that are correct, let's learn the recipe, and then apply them to all the rest of OmniTools!

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

iulianbarbu · 2024-11-01T20:10:48Z

All in all the PR is pending on two outstanding CI issues (observed at least):

The cache is using an outdated version of substrate_test_runtime which will make clippy checks fail, seemingly because of an issue with the forklift usage, but more precisely TBD
Docify will not embed the rust,ignore langs for the code samples from the READMEs, and given we use them for staging-chain-spec-builder's lib.rs docs too, they will be interpreted as docs tests and the CI will run them (even if we use the doctest = false directive in Cargo.toml). The CI runs docs tests with cargo test --doc which will not consider the Cargo.toml directive. We have three options: (1) fix docify, release a new version and consume it by next week release (PR opened here: Fix docify embed in md sam0x17/docify#28), (2) manually insert the rust,ignore langs for the code samples in the README.md, even if regenerating it based on README.docify.md will remove them (this would be a temporary fix until docify is fixed), and (3) revert to exporting the entire tests instead of just their contents. I would choose the second option in the meantime if the first will not be resolved in time.

cc @kianenigma @michalkucharczyk

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

templates: make node compilation optional

29c9d09

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

iulianbarbu requested review from a team as code owners October 7, 2024 15:44

iulianbarbu self-assigned this Oct 7, 2024

iulianbarbu marked this pull request as draft October 8, 2024 09:49

iulianbarbu removed request for a team October 8, 2024 09:49

iulianbarbu added 12 commits October 9, 2024 13:44

don't add default-members for solochain

0488023

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

remove extra spaces

a9fb4fe

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

added zombienet tests for omni node

b20001c

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

assert on the finality instead

6a7946a

This increases CI time but for a good reason. We want to ensure parachains pick up the finalized block info from the relay chain. Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

omni-node: remove in/out peers limitation

dae9b73

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

Merge branch 'master' of github.com:paritytech/polkadot-sdk into ib-m…

5af79ea

…ake-node-compilation-optional

explained how to run minimal in multi-node setup

8e2450d

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

Revert "omni-node: remove in/out peers limitation"

b75bff2

This reverts commit dae9b73.

polish minimal template docs

e9ba647

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

more polish to the docs

67bc652

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

add parachain id const

f0232c4

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

more docs polishing

73695bd

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

iulianbarbu added the T17-Templates This PR/Issue is related to templates label Oct 22, 2024

iulianbarbu and others added 8 commits October 22, 2024 15:30

add prdoc

62c4a49

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

Merge branch 'master' into ib-make-node-compilation-optional

868302d

fix md checks

e1cbdfe

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

fix md to pass checker

3418a41

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

remove line

d02f136

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

fix parachain id type

d0e7ee0

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

more polish to the READMEs

94d6d0d

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

even more polish

16738e5

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

iulianbarbu marked this pull request as ready for review October 22, 2024 20:39

paritytech-review-bot bot requested a review from a team October 22, 2024 20:40

kianenigma reviewed Nov 1, 2024

View reviewed changes

substrate/bin/utils/chain-spec-builder/Cargo.toml Show resolved Hide resolved

kianenigma reviewed Nov 1, 2024

View reviewed changes

templates/minimal/node/src/cli.rs Show resolved Hide resolved

kianenigma reviewed Nov 1, 2024

View reviewed changes

templates/minimal/zombienet.toml Outdated Show resolved Hide resolved

kianenigma reviewed Nov 1, 2024

View reviewed changes

iulianbarbu added 2 commits November 1, 2024 13:38

rever forklift but remove SKIP_WASM_BUILD

d224db5

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

remove forklift again

0a79b74

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

This was referenced Nov 1, 2024

Add more examples to polkadot-omni-node-lib README/docs #6330

Open

Create CI workflow that checks README.docify.md's are in sync with README.mds #6331

Closed

iulianbarbu and others added 6 commits November 1, 2024 16:20

some more templates' READMEs fixes

51c1814

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

update minimal template README

92dbdd8

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

Merge branch 'master' into ib-make-node-compilation-optional

fa71ef5

fix clippy

46d0be7

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

try one more time unsetting SKIP_WASM_BUILD

e6327ae

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

just disable forklift

28583e2

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

kianenigma approved these changes Nov 1, 2024

View reviewed changes

iulianbarbu added 2 commits November 1, 2024 21:52

temporary fix for doc tests disabling

8ca2e65

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

rever forklift

256d932

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

kianenigma and others added 3 commits November 4, 2024 09:29

Merge branch 'master' into ib-make-node-compilation-optional

8eed5a3

upgrade docify to 0.2.9

4126edb

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

remove forklift based caching for clippy checks

9f4c95f

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>

kianenigma added this pull request to the merge queue Nov 4, 2024

Merged via the queue into paritytech:master with commit 2a84917 Nov 4, 2024
197 of 198 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

templates: make node compilation optional #5954

templates: make node compilation optional #5954

iulianbarbu commented Oct 7, 2024 •

edited

Loading

kianenigma Nov 1, 2024

michalkucharczyk Nov 1, 2024

iulianbarbu Nov 1, 2024 •

edited

Loading

iulianbarbu Nov 1, 2024

kianenigma Nov 1, 2024

kianenigma Nov 1, 2024

michalkucharczyk Nov 1, 2024

kianenigma Nov 1, 2024

kianenigma Nov 1, 2024

iulianbarbu Nov 1, 2024

kianenigma Nov 1, 2024 •

edited

Loading

iulianbarbu Nov 1, 2024

kianenigma Nov 1, 2024

kianenigma Nov 1, 2024 •

edited

Loading

iulianbarbu Nov 1, 2024

kianenigma left a comment

iulianbarbu commented Nov 1, 2024 •

edited

Loading

		@@ -28,6 +31,53 @@ const DUMMY_PATH: &str = "fake-runtime-path";

		const OUTPUT_FILE: &str = "/tmp/chain_spec_builder.test_output_file.json";

		const RUNTIME_PATH: &str = unwrap_option_str(substrate_test_runtime::WASM_BINARY_PATH);

	cargo build -p parachain-template-runtime --release
	cargo build --release

	<target/release/wbuild/path/to/minimal-template-runtime.wasm> named-preset development
	./target/release/wbuild/parachain-template-runtime/parachain_template_runtime.wasm named-preset development

templates: make node compilation optional #5954

templates: make node compilation optional #5954

Conversation

iulianbarbu commented Oct 7, 2024 • edited Loading

Description

Integration

Review Notes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianbarbu Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

iulianbarbu commented Nov 1, 2024 • edited Loading

iulianbarbu commented Oct 7, 2024 •

edited

Loading

iulianbarbu Nov 1, 2024 •

edited

Loading

kianenigma Nov 1, 2024 •

edited

Loading

kianenigma Nov 1, 2024 •

edited

Loading

iulianbarbu commented Nov 1, 2024 •

edited

Loading