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

Conversation

@TriplEight
Copy link
Contributor

@TriplEight TriplEight commented Dec 4, 2020

Fixes the previous flag that apparently didn't do the job.

@danforbes I think it would make sense to fix the docs to make CI pass on this job right here. To make this faster, I've removed everything else from CI while this is in WIP state.

It's handy to track the pipelines for this PR with https://gitlab.parity.io/parity/substrate/-/pipelines?page=1&scope=all&ref=7670

To debug: remove -Dwarnings in https://github.com/paritytech/substrate/pull/7670/files#diff-037ea159eb0a7cb0ac23b851e66bee30fb838ee8d0d99fa331a1ba65283d37f7R133 to see all the warnings at once

@TriplEight TriplEight added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A3-in_progress Pull request is in progress. No review needed at this stage. I3-bug The node fails to follow expected behavior. 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 Dec 4, 2020
@TriplEight TriplEight requested a review from danforbes December 4, 2020 10:29
@TriplEight TriplEight self-assigned this Dec 4, 2020
@danforbes
Copy link
Contributor

@TriplEight can you give me a sense of the priority on this as well as estimated effort you think will be required to address it?

@TriplEight
Copy link
Contributor Author

@danforbes apparently, failing on warnings didn't ever work for this job. So just a random job succeeds and Ctrl+F warning: shows 143 matches.
Not to say drop everything, but our docs have quite a lot of warnings.

@gui1117
Copy link
Contributor

gui1117 commented Dec 10, 2020

I think we can fix all doc warning, this fix almost all of them except nalgebra and authority-discovery #7710

* fix docs

* Update frame/merkle-mountain-range/src/lib.rs

Co-authored-by: Alexander Theißen <[email protected]>

Co-authored-by: Alexander Theißen <[email protected]>
@danforbes
Copy link
Contributor

danforbes commented Dec 10, 2020

@mxinden can you help with the broken link in the docs for the authority discovery worker? I think it was committed here https://github.com/paritytech/substrate/pull/7368/files#diff-da93fe21a550b47fbf80544c1d1144276be1b5c39354597883ddce0a935cf39eR96

90 | /// When constructed with either [`Role::PublishAndDiscover`] or [`Role::Publish`] a [`Worker`] will
   |                                                                   ^^^^^^^^^^^^^^^ the enum `Role` has no variant or associated item named `Publish`

@mxinden
Copy link
Contributor

mxinden commented Dec 11, 2020

@danforbes I pushed f618f8d fixing the link. Thanks for the pointer.

@TriplEight
Copy link
Contributor Author

Thanks all!
Does it mean I can undo the debug CI setup and merge?

@gui1117
Copy link
Contributor

gui1117 commented Dec 11, 2020

cargo doc will still complains about nalgebra
To fix it I asked them to backport their doc fix.
But we can also ask linregress to make a new version with latest nalgebra.
Or ignore nalgebra doc grumbles but I don't know how

@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 4, 2021
@gnunicorn
Copy link
Contributor

any progress on this?

@gui1117
Copy link
Contributor

gui1117 commented Mar 4, 2021

nalgebra will probably never fix for its old version: dimforge/nalgebra#807
I don't expect linregress ppl to make a new major release just for fixing doc.

So maybe we can do a script on our side to ignore the failing doc of nalgebra or push nalgebra to release the minor version somehow. nalgebra dev is dimforge, https://www.dimforge.com/, he should be reachable and accept donation. that may be a way to solve.

@TriplEight
Copy link
Contributor Author

TriplEight commented Mar 4, 2021

maybe it makes sense to skip these particular ones, merge and have a tracking issue to unskip them?

@gui1117
Copy link
Contributor

gui1117 commented Mar 4, 2021

I don't know how to avoid the ci to test the doc of nalgebra. it is not one of our crate

@TriplEight
Copy link
Contributor Author

do we actually care documenting deps? --no-deps Don't build documentation for dependencies

@TriplEight
Copy link
Contributor Author

or --exclude <SPEC>... Exclude packages from the build to exclude something specific

@gui1117
Copy link
Contributor

gui1117 commented Mar 4, 2021

do we actually care documenting deps? --no-deps Don't build documentation for dependencies

I think it could work but actually I personally prefer to have deps documented.
Something we can do is to check doc without deps in the CI and but still generate doc with deps (without checking about warnings), it seems a good solution to me

@TriplEight
Copy link
Contributor Author

so I think it makes sense to add to this job two runs

  1. run it with --no-deps and -Dwarnings to fail on what's ours, in the case of it passes, we produce the docs
  2. run it with -Dwarnings to keep track that's something's still broken

@TriplEight
Copy link
Contributor Author

TriplEight commented Mar 5, 2021

wait a sec, it has been time cargo +nightly doc --no-deps --workspace --all-features --verbose with --no-deps all along.

without it, doc job fails way earlier, with

Checking fallible-iterator v0.2.0
     Running `sccache rustc --crate-name fallible_iterator --edition=2018 /ci-cache/cargo/registry/src/github.amrom.workers.dev-1ecc6299db9ec823/fallible-iterator-0.2.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="std"' -C metadata=c35a8b04def78abb -C extra-filename=-c35a8b04def78abb --out-dir /ci-cache/target/debug/deps -L dependency=/ci-cache/target/debug/deps --cap-lints allow`
error: Unrecognized option: 'i'

error: could not document `cfg-if`

Caused by:
  process didn't exit successfully: `rustdoc --edition=2018 --crate-type lib --crate-name cfg_if /ci-cache/cargo/registry/src/github.amrom.workers.dev-1ecc6299db9ec823/cfg-if-1.0.0/src/lib.rs --cap-lints allow -o /ci-cache/target/doc --error-format=json --json=diagnostic-rendered-ansi -L dependency=/ci-cache/target/debug/deps -in-header /builds/.maintain/rustdoc-header.html -Dwarnings --crate-version 1.0.0` (exit code: 1)
warning: build failed, waiting for other jobs to finish...
error: build failed

@TriplEight
Copy link
Contributor Author

FYI the current command cargo +nightly doc --no-deps --workspace --all-features --verbose will fail

either with

   Compiling proc-macro-error-attr v1.0.4
     Running `sccache rustc --crate-name build_script_build --edition=2018 /ci-cache/cargo/registry/src/github.amrom.workers.dev-1ecc6299db9ec823/proc-macro-error-attr-1.0.4/build.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 -C metadata=0e1ad2f9b429b0a6 -C extra-filename=-0e1ad2f9b429b0a6 --out-dir /ci-cache/target/debug/build/proc-macro-error-attr-0e1ad2f9b429b0a6 -L dependency=/ci-cache/target/debug/deps --extern version_check=/ci-cache/target/debug/deps/libversion_check-b51661b575c73396.rlib --cap-lints allow`
error: Unrecognized option: 'i'

error: could not document `sp-std`

Caused by:
  process didn't exit successfully: `rustdoc --edition=2018 --crate-type lib --crate-name sp_std primitives/std/src/lib.rs -o /ci-cache/target/doc --cfg 'feature="default"' --cfg 'feature="std"' --error-format=json --json=diagnostic-rendered-ansi -L dependency=/ci-cache/target/debug/deps -in-header /builds/.maintain/rustdoc-header.html -Dwarnings --crate-version 3.0.0` (exit code: 1)
warning: build failed, waiting for other jobs to finish...
error: Unrecognized option: 'i'

error: build failed

or

   Compiling num-traits v0.2.14
     Running `sccache rustc --crate-name build_script_build /ci-cache/cargo/registry/src/github.amrom.workers.dev-1ecc6299db9ec823/num-traits-0.2.14/build.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="default"' --cfg 'feature="libm"' --cfg 'feature="std"' -C metadata=a0aa44b6f788a097 -C extra-filename=-a0aa44b6f788a097 --out-dir /ci-cache/target/debug/build/num-traits-a0aa44b6f788a097 -L dependency=/ci-cache/target/debug/deps --extern autocfg=/ci-cache/target/debug/deps/libautocfg-126bc84acbac0b8c.rlib --cap-lints allow`
error: Unrecognized option: 'i'
error: Unrecognized option: 'i'


error: could not document `substrate-test-utils-test-crate`

Caused by:
  process didn't exit successfully: `rustdoc --edition=2018 --crate-type bin --crate-name substrate_test_utils_test_crate test-utils/test-crate/src/main.rs -o /ci-cache/target/doc --error-format=json --json=diagnostic-rendered-ansi --document-private-items -L dependency=/ci-cache/target/debug/deps -in-header /builds/.maintain/rustdoc-header.html -Dwarnings --crate-version 0.1.0` (exit code: 1)
warning: build failed, waiting for other jobs to finish...
error: build failed

@gui1117
Copy link
Contributor

gui1117 commented Mar 5, 2021

my bad I got confused about nalgebra because when testing locally warning were raised, I fixing master now and will open a PR

@TriplEight TriplEight requested a review from a team as a code owner March 10, 2021 08:47
@TriplEight TriplEight mentioned this pull request Mar 10, 2021
Copy link
Contributor

@gabreal gabreal left a comment

Choose a reason for hiding this comment

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

build-rust-doc job looks like before but everything else is missing.

@TriplEight
Copy link
Contributor Author

it's still in the debug state

@gnunicorn
Copy link
Contributor

closing for a lack of progress.

@gnunicorn gnunicorn closed this May 19, 2021
@TriplEight TriplEight reopened this Aug 9, 2021
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 9, 2021
@TriplEight TriplEight closed this Aug 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A3-in_progress Pull request is in progress. No review needed at this stage. 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. I3-bug The node fails to follow expected behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants