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

Fix doc build with --all-features #8277

Merged
merged 5 commits into from
Mar 5, 2021
Merged

Fix doc build with --all-features #8277

merged 5 commits into from
Mar 5, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Mar 5, 2021

this fixes the cmd: RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --no-deps --workspace --all-features

Some outdated doc in network + link to private item

Some doc in sc_network were outdated, and some were linking to private item. I'm not sure how to solve them so I just remove the invalid link. But some are still outdated. (maybe cc @tomaka )

Issue with feature try-runtime

The feature try-runtime is breaking the build for unknown reason: (to reproduce go to the first commit 16b38eb and execute the cmd: RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --no-deps --workspace --all-features)

error[E0046]: not all trait items implemented, missing: `pre_upgrade`, `post_upgrade`
  --> test-utils/runtime/src/system.rs:44:1
   |
44 | / decl_module! {
45 | |     pub struct Module<T: Config> for enum Call where origin: T::Origin {}
46 | | }
   | |_^ missing `pre_upgrade`, `post_upgrade` in implementation
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
   = help: implement the missing item: `fn pre_upgrade() -> sc_service::Result<(), &'static str> { todo!() }`
   = help: implement the missing item: `fn post_upgrade() -> sc_service::Result<(), &'static str> { todo!() }`


Thus I actually implemented in the trait OnRuntimeUpgrade the default implementation for pre_upgrade and post_upgrade. The disadvantage is that if node-runtime forget to bound my-pallet/try-runtime in its Cargo.toml then the pallet implementation will simply be ignored.

@gui1117 gui1117 requested review from athei, kianenigma and a team as code owners March 5, 2021 12:46
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 5, 2021
@gui1117 gui1117 marked this pull request as draft March 5, 2021 12:48
@tomaka
Copy link
Contributor

tomaka commented Mar 5, 2021

I'd prefer to not go for this solution of "un-linking" the dead links.

The problem is that the documentation is outdated. Doc-links can detect when the documentation is outdated. If you disable the doc-links, it doesn't solve the problem of the documentation being outdated. You're just killing the messenger.

@@ -1404,7 +1404,6 @@ impl_runtime_apis! {
#[cfg(feature = "try-runtime")]
impl frame_try_runtime::TryRuntime<Block> for Runtime {
fn on_runtime_upgrade() -> Result<(Weight, Weight), sp_runtime::RuntimeString> {
frame_support::debug::RuntimeLogger::init();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the compilation is failing because debug doesn't exist anymore cc @kianenigma

I think logger is automatically initialized now, so we can remove no ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this should be removed, and I think there might be other places in try-runtime that we use this the deprecated logger as well.

I've made an issue here: https://github.com/paritytech/ci_cd/issues/103, I hope we have this in the CI soon.

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Unless this is urgent, please either do all the appropriate changes in sc-network or (more likely) revert so that I do it later.

frame/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
@@ -65,7 +65,7 @@
//! origin can not bail out in any way, if their solution is queued.
//!
//! Upon the end of the signed phase, the solutions are examined from best to worse (i.e. `pop()`ed
//! until drained). Each solution undergoes an expensive [`Pallet::feasibility_check`], which
//! until drained). Each solution undergoes an expensive `Pallet::feasibility_check`, which
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is private item

@gui1117
Copy link
Contributor Author

gui1117 commented Mar 5, 2021

Unless this is urgent, please either do all the appropriate changes in sc-network or (more likely) revert so that I do it later.

yes, sorry done

@kianenigma
Copy link
Contributor

If there are further doc-related issues, I'd be happy of the fixes that are related to try-runtime to get extracted out and merged, as other PRs are waiting for them. I can help with this if needed 👍

@gui1117
Copy link
Contributor Author

gui1117 commented Mar 5, 2021

This PR was more a draft for the stuff needed to change, if you need the try-runtime changes feel free to take them.

Sorry if it felt pushing, I'm also not entirely convinced if it is the good way of addressing try-runtime issue.

@gui1117 gui1117 removed the A0-please_review Pull request needs code review. label Mar 5, 2021
@gui1117 gui1117 marked this pull request as ready for review March 5, 2021 14:29
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 5, 2021
@gui1117 gui1117 added 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 Mar 5, 2021
@gui1117 gui1117 changed the title Fix doc Fix doc build with --all-features Mar 5, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Mar 5, 2021

bot merge

@ghost
Copy link

ghost commented Mar 5, 2021

Checks failed; merge aborted.

@bkchr bkchr merged commit adca498 into master Mar 5, 2021
@bkchr bkchr deleted the gui-fix-doc branch March 5, 2021 15:08
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. 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.

6 participants