Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

ordian
Copy link

@ordian ordian commented Sep 9, 2021

@ordian ordian added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 9, 2021
@ordian ordian requested review from tomusdrw and bkchr September 9, 2021 09:04
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

If we want to avoid accidentally disabling all of the runtimes, perhaps introducing another feature no-runtimes and throwing a compilation error in case it's not explicitly enabled is reasonable?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code isn't adapted for these feature changes.

@ordian
Copy link
Author

ordian commented Sep 9, 2021

The code isn't adapted for these feature changes.

could you elaborate? do you mean

introducing another feature no-runtimes and throwing a compilation error in case it's not explicitly enabled

@bkchr
Copy link
Member

bkchr commented Sep 9, 2021

I mean, if you disable the polkadot feature for the client or service, then both would not compile. But @tomusdrw requires the generic functions of polkadot-service for example to run it with its own runtime etc.

@bkchr
Copy link
Member

bkchr commented Sep 9, 2021

Ahh I see that all usage is hidden behind the FullNode feature. However I would still favor that https://github.com/paritytech/polkadot/blob/master/node/service/src/lib.rs#L1356-L1366 this is feature gated and all the other usages of polkadot-runtime. Like it is done for the other runtimes.

Same applied for the client crate.

We can just throw a runtime error if someone calls the functions and no runtime is enabled.

@ordian ordian added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 9, 2021
@ordian ordian marked this pull request as draft September 9, 2021 10:21
Andronik Ordian added 4 commits September 9, 2021 12:56
* master:
  Bump tokio from 1.10.1 to 1.11.0 (#3821)
  Add words to the dictionnary (#3819)
  Add vault secrets to puplish-rustdoc job (#3816)
  Change pipeline to use Vault (#3722)
  Don't drop UMP queue items if weight exhausted (#3784)
  Fix flaky availability-recovery test (#3812)
  participate in disputes only if haven't voted already (#3796)
@ordian ordian requested a review from bkchr September 9, 2021 11:09
@ordian ordian marked this pull request as ready for review September 9, 2021 11:09
@ordian ordian added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 9, 2021
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a CI check to make sure the no-default-features compilation works?

@bkchr
Copy link
Member

bkchr commented Sep 9, 2021

Fine by me.

@ordian
Copy link
Author

ordian commented Sep 9, 2021

Shall we add a CI check to make sure the no-default-features compilation works?

It doesn't work for polkadot-client and you will need at least one runtime when compiling cli and service with full-node feature.

pub type PolkadotChainSpec = service::GenericChainSpec<polkadot::GenesisConfig, Extensions>;

// Dummy chain spec, in case when we don't have the native runtime.
pub type DummyChainSpec = service::GenericChainSpec<(), Extensions>;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah should be fine.

@librelois
Copy link
Contributor

It would be nice to use static_assertions to make sure at compile time that the features combination is valid: https://docs.rs/static_assertions/1.1.0/static_assertions/macro.assert_cfg.html

@ordian
Copy link
Author

ordian commented Sep 9, 2021

We could also use std::compile_error as was suggested here: #3189 (comment).

@ordian
Copy link
Author

ordian commented Sep 9, 2021

@bkchr @tomusdrw please take a look again

@ordian
Copy link
Author

ordian commented Sep 13, 2021

Alternatively, could we patch the rococo-runtime crate and use a local version (rialto/millau)?
That way we could have both native runtime, without copying the whole node stuff over.

This PR allows only this approach, but you'd have only one native runtime if you disable polkadot and enable rococo feature.

pub type PolkadotChainSpec = service::GenericChainSpec<polkadot::GenesisConfig, Extensions>;

// Dummy chain spec, in case when we don't have the native runtime.
pub type DummyChainSpec = service::GenericChainSpec<(), Extensions>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah should be fine.

set -e

pushd node/service && cargo check --no-default-features && popd
pushd cli && cargo check --no-default-features --features "service" && popd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling a feature defeats a little bit the name of this file, but fine :P :D

@ordian
Copy link
Author

ordian commented Sep 13, 2021

bot merge

@ghost
Copy link

ghost commented Sep 13, 2021

Trying merge.

@ghost ghost merged commit 0f61cb8 into master Sep 13, 2021
@ghost ghost deleted the ao-optional-polkadot-runtime branch September 13, 2021 10:28
ordian pushed a commit that referenced this pull request Sep 14, 2021
* master:
  Bump tracing from 0.1.26 to 0.1.27 (#3841)
  Companion for substrate#9711 (#3801)
  fix complaints in CI (#3838)
  dockerfiles: upgrade to ubuntu:20.04; some chore (#3828)
  make polkadot-runtime optional feature (#3820)
  Companion for #9648 (#3757)
  Substrate Companion #9737 (#3830)
  Add logging for worker spawn failures (#3827)
  Add Canvas (#3823)
  Allow staking miner to use different election algorithms (#3752)
ordian pushed a commit that referenced this pull request Sep 15, 2021
* master: (21 commits)
  Add build with docker info to README (#3843)
  improve approval tracing (#3846)
  UMP: Support Overweight messages (#3575)
  Companion for substrate#9115 (#3265)
  Better error messages. (#3835)
  Put all authorities of a session into `SessionInfo`. (#3813)
  Bump tracing from 0.1.26 to 0.1.27 (#3841)
  Companion for substrate#9711 (#3801)
  fix complaints in CI (#3838)
  dockerfiles: upgrade to ubuntu:20.04; some chore (#3828)
  make polkadot-runtime optional feature (#3820)
  Companion for #9648 (#3757)
  Substrate Companion #9737 (#3830)
  Add logging for worker spawn failures (#3827)
  Add Canvas (#3823)
  Allow staking miner to use different election algorithms (#3752)
  Do not expire HRMP open channel requests (#3543)
  Bump tokio from 1.10.1 to 1.11.0 (#3821)
  Add words to the dictionnary (#3819)
  Add vault secrets to puplish-rustdoc job (#3816)
  ...
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants