Skip to content

Commit

Permalink
templates: make node compilation optional (#5954)
Browse files Browse the repository at this point in the history
# Description

Closes #5940  

## 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:

- [x] zombienet with `minimal-template-node` , `parachain-template-node`
and `polkadot-omni-node`. Things work as expected.
- [x] 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.
- [x] testing the changes for the sync templates workflow (ignore the
added comment from the Cargo.tomls, it was removed here on this branch:
[99bff3e](99bff3e)):
[minimal](https://github.com/paritytech-stg/polkadot-sdk-minimal-template/pull/22/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R9),
[parachain](https://github.com/paritytech-stg/polkadot-sdk-parachain-template/pull/19/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R9),
[solochain](https://github.com/paritytech-stg/polkadot-sdk-solochain-template/pull/17/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R9).
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 <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
  • Loading branch information
iulianbarbu and kianenigma authored Nov 4, 2024
1 parent 657b550 commit 2a84917
Show file tree
Hide file tree
Showing 37 changed files with 1,468 additions and 279 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ jobs:
- uses: actions/checkout@6d193bf28034eafb982f37bd894289fe649468fc # v4.1.7
- name: script
run: |
forklift cargo clippy --all-targets --locked --workspace --quiet
forklift cargo clippy --all-targets --all-features --locked --workspace --quiet
cargo clippy --all-targets --locked --workspace --quiet
cargo clippy --all-targets --all-features --locked --workspace --quiet
check-try-runtime:
runs-on: ${{ needs.preflight.outputs.RUNNER }}
needs: [preflight]
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/misc-sync-templates.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ jobs:
homepage = "https://paritytech.github.io/polkadot-sdk/"
[workspace]
EOF
[ ${{ matrix.template }} != "solochain" ] && echo "# Leave out the node compilation from regular template usage." \
&& echo "\"default-members\" = [\"pallets/template\", \"runtime\"]" >> Cargo.toml
[ ${{ matrix.template }} == "solochain" ] && echo "# The node isn't yet replaceable by Omni Node."
cat << EOF >> Cargo.toml
members = [
"node",
"pallets/template",
Expand Down
147 changes: 117 additions & 30 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 12 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,13 @@ default-members = [
[workspace.lints.rust]
suspicious_double_ref_op = { level = "allow", priority = 2 }
# `substrate_runtime` is a common `cfg` condition name used in the repo.
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(build_opt_level, values("3"))', 'cfg(build_profile, values("debug", "release"))', 'cfg(enable_alloc_error_handler)', 'cfg(fuzzing)', 'cfg(substrate_runtime)'] }
unexpected_cfgs = { level = "warn", check-cfg = [
'cfg(build_opt_level, values("3"))',
'cfg(build_profile, values("debug", "release"))',
'cfg(enable_alloc_error_handler)',
'cfg(fuzzing)',
'cfg(substrate_runtime)',
] }

[workspace.lints.clippy]
all = { level = "allow", priority = 0 }
Expand Down Expand Up @@ -677,6 +683,7 @@ cid = { version = "0.9.0" }
clap = { version = "4.5.13" }
clap-num = { version = "1.0.2" }
clap_complete = { version = "4.5.13" }
cmd_lib = { version = "1.9.5" }
coarsetime = { version = "0.1.22" }
codec = { version = "3.6.12", default-features = false, package = "parity-scale-codec" }
collectives-westend-emulated-chain = { path = "cumulus/parachains/integration-tests/emulated/chains/parachains/collectives/collectives-westend" }
Expand Down Expand Up @@ -735,7 +742,7 @@ derive_more = { version = "0.99.17", default-features = false }
digest = { version = "0.10.3", default-features = false }
directories = { version = "5.0.1" }
dlmalloc = { version = "0.2.4" }
docify = { version = "0.2.8" }
docify = { version = "0.2.9" }
dyn-clonable = { version = "0.9.0" }
dyn-clone = { version = "1.0.16" }
ed25519-dalek = { version = "2.1", default-features = false }
Expand Down Expand Up @@ -1087,7 +1094,9 @@ polkavm-derive = "0.9.1"
polkavm-linker = "0.9.2"
portpicker = { version = "0.1.1" }
pretty_assertions = { version = "1.3.0" }
primitive-types = { version = "0.13.1", default-features = false, features = ["num-traits"] }
primitive-types = { version = "0.13.1", default-features = false, features = [
"num-traits",
] }
proc-macro-crate = { version = "3.0.0" }
proc-macro-warning = { version = "1.0.0", default-features = false }
proc-macro2 = { version = "1.0.86" }
Expand Down
Loading

0 comments on commit 2a84917

Please sign in to comment.