Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

templates: make node compilation optional #5954

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented Oct 7, 2024

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. Automation that simply run `cargo run

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 but I assume it is not representative because of some knowledge gaps in how to operate correctly a local testnet and what to look for/expect. Need to revise my testing strategy with someone from the team.

  • started both minimal/parachain runtimes with omni-node with manual seal: polkadot-parachain --chain chain_spec.json --dev_block_time 1000. What I notice is that the best/finalized blocks number are not progressing, but maybe that's because I am not starting the node as a collator? I think this isn't necessarily true though because of the other test (done with zombienet) where I started the omni-node with parachain_template_runtime as a collator, and the logs look the same. TODO clarify how block authoring/finalization works when starting with manual seal.
  • started both runtimes with omni-node: polkadot-parachain --chain chain_spec.json. Similar thing as before was noticed in terms of best/finalized blocks, but it should be because of the network topology, when the number of relaychain nodes might not be sufficient and the same for collators. I think starting a local testnet manually isn't required and I can cover this by using just zombienet.
  • started zombienet based on the network topology present in cumulus/zombienet/tests/0006-rpc_collator_builds_blocks.toml, where I added a genesis_wasm_path pointing to the parachain mininal_template_runtime.wasms files, and set the relaychain.chain to polkadot. All the nodes started without errors but finding the best blocks/finalized was stuck at block with number #0, the same as for the other tests. Unfortunately I did not use zombienet in the right way and I am still looking for how to start omni-node with template runtimes. Would be great to use zombienet to start nodes with manual seal too. I'll leave this checkbox for both, covered through zombienet.
  • chopsticks testing

TODOs

  • the changes for the github workflow might need some testing to and I have to see how to raise the confidence that the changes will produce the expected results.
  • prdoc is probably needed considering there might be people relying on the templates, and how they build.
  • double check on testing strategy

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu requested review from a team as code owners October 7, 2024 15:44
@iulianbarbu iulianbarbu self-assigned this Oct 7, 2024
@iulianbarbu iulianbarbu marked this pull request as draft October 8, 2024 09:49
@iulianbarbu iulianbarbu removed request for a team October 8, 2024 09:49
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make templates node building optional
1 participant