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

Make templates node building optional #5940

Closed
2 tasks done
iulianbarbu opened this issue Oct 6, 2024 · 0 comments · Fixed by #5954
Closed
2 tasks done

Make templates node building optional #5940

iulianbarbu opened this issue Oct 6, 2024 · 0 comments · Fixed by #5954
Assignees

Comments

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Oct 6, 2024

The node shouldn't compile by default when you do cargo build. It should be feature gated behind --features with-node. By default, the template should only compile the runtime and produce a wasm. Instead, the user is invited to download polkadot-omni-node/polkadot-parachain.

To run this wasm, one is expected to use either of the options below, so test them too:

Do it for:

  • minimal
  • parachain
@iulianbarbu iulianbarbu moved this to Milestone 0 in Polkadot Omni Node Oct 6, 2024
@iulianbarbu iulianbarbu self-assigned this Oct 6, 2024
ptechrelease-forkwrite-polkadotsdk bot pushed a commit to paritytech-release/polkadot-sdk that referenced this issue Nov 4, 2024
# Description

Closes paritytech#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](paritytech@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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Milestone 0
Development

Successfully merging a pull request may close this issue.

1 participant