Deny warning when building doc

[gui1117]

This make the CI deny for warning when building the doc. So we can catch dead link and especially dead link in rust doc when linking to types.

But I think this current PR is wrong because it will also deny warning on dependencies which can be annoying in case some of our dependence have warnings.

Maybe we should have both:

RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --workspace --all-features --verbose --no-deps
time cargo +nightly doc --workspace --all-features --verbose

Or maybe have 2 different job ?
Is there some good solution ?

cc @TriplEight

[alvicsam]

We can have 2 different jobs. During PR pipeline we can use the one with RUSTDOCFLAGS="-Dwarnings" so developer can see that the check failed. And during master pipeline use the one without flag so not to break pipeline.

[gui1117]

I don't know when the master pipelin is run instead of PR pipeline, but for me any solution which ensures that the docs for the crate in the workspace are without warning would be great.
All in all what I want to ensure is:

RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --workspace --all-features --verbose --no-deps

Because I don't want to check the doc of the dependencies, only the doc of our crates here in substrate.

[kianenigma]

We should make sure all doc links are valid at all times 💯

[gui1117]

We should make sure all doc links are valid at all times 100

but we don't control the doc of our dependency, there might be some time where their doc is broken and we need to update to their new version, that can be annoying if our CI doesn't allow our dependency to have doc with warnings.

[kianenigma]

We should make sure all doc links are valid at all times 100

but we don't control the doc of our dependency, there might be some time where their doc is broken and we need to update to their new version, that can be annoying if our CI doesn't allow our dependency to have doc with warnings.

Okay, I mean everything in our repo if possible.

[gui1117]

@alvicsam I have no idea how to implement something for the master pipeline/ PR pipeline.

[alvicsam]

@thiolliere I can help you with that but I don't understand how it should work.

Currently this job (build-rustdoc ) runs on both PR and master pipelines. With this PR the master pipeline will fail.
So my assumption was to keep this the build-rustdoc task with RUSTDOCFLAGS="-Dwarnings" so developer can see that this step fails and docs have some issues. And create another task build-rustdoc-master (the name is just an example) for master without RUSTDOCFLAGS="-Dwarnings" in order not to break the whole pipeline because of the docs.

Otherwise we can mark continuous-integration/gitlab-build-rustdoc in github as Required and a PR cannot be merge until docs are fixed. But I don't know what should a developer do if there is an issue that you described:

but we don't control the doc of our dependency, there might be some time where their doc is broken and we need to update to their new version, that can be annoying if our CI doesn't allow our dependency to have doc with warnings.

[gui1117]

or maybe we can also have 2 jobs:

• one required, which does RUSTDOCFLAGS="-Dwarnings" but doesn't build doc for deps with --no-deps
• another job, not required, which builds the whole doc without denying warnings and without excluding deps.

The only issue I see is that it makes the CI even longer

[gui1117]

So I added the new job, which is meant to be mandatory, and kept the doc generation unchanged.

It seems ok to me

EDIT: I also fixed the doc or removed the link that can't be fixed

[alvicsam]

LGTM

The only issue I see is that it makes the CI even longer

Actually it doesn't. The check-rustdoc job is quick as I can see and at the same stage there is test-linux-stable job which is way longer (jobs in one stage run simultaneously)

@TriplEight can you please mark check-rustdoc as "Required"?

[TriplEight]

So it turns out that we'll have two jobs:

check-rustdoc:
  ...
  - RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --workspace --all-features --verbose

and

build-rustdoc:
  ...
  - time cargo +nightly doc --workspace --all-features --verbose --no-deps

Both running on PRs and master.
Two questions: why do we care checking while failing on warnings, but with deps. And building rustdocs for publishing without deps? Is there some cargo doc workaround or we can just have one job as one in chech-rustdoc now?

[gui1117]

So it turns out that we'll have two jobs:

check-rustdoc:
  ...
  - RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --workspace --all-features --verbose

and

build-rustdoc:
  ...
  - time cargo +nightly doc --workspace --all-features --verbose --no-deps

Both running on PRs and master. Two questions: why do we care checking while failing on warnings, but with deps. And building rustdocs for publishing without deps? Is there some cargo doc workaround or we can just have one job as one in chech-rustdoc now?

no the jobs should be:

check-rustdoc:
  ...
  - RUSTDOCFLAGS="-Dwarnings" time cargo +nightly doc --workspace --all-features --verbose --no-deps

and

build-rustdoc:
  ...
  - time cargo +nightly doc --workspace --all-features --verbose
  ... publish docs

So we check docs while failing on warnings but without deps (because we don't want to check our deps, we only want to check our own doc). this job should be required, our doc should always build without warning.

We build the docs while with deps without failing on warnings (this is because we want to publish all the docs in one place, it makes it easier to walk through all docs)

[TriplEight]

Now it makes sense