Skip to content

[Merged by Bors] - Adds docs deadlinks check on CI#1590

Closed
mnmaita wants to merge 8 commits into
bevyengine:mainfrom
mnmaita:feat/add-docs-deadlinks-ci-check
Closed

[Merged by Bors] - Adds docs deadlinks check on CI#1590
mnmaita wants to merge 8 commits into
bevyengine:mainfrom
mnmaita:feat/add-docs-deadlinks-ci-check

Conversation

@mnmaita

@mnmaita mnmaita commented Mar 7, 2021

Copy link
Copy Markdown
Member

Closes #1579

This is my first contribution to this repository, feel free to correct anything that I'm missing and I'll address feedback as soon as possible!

@CleanCut

CleanCut commented Mar 7, 2021

Copy link
Copy Markdown
Member

Since you're running the CI job on Linux, you'll need to add a step to install the Linux build dependencies. 😄

- name: Install alsa and udev
run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev

@mnmaita

mnmaita commented Mar 7, 2021

Copy link
Copy Markdown
Member Author

Since you're running the CI job on Linux, you'll need to add a step to install the Linux build dependencies. 😄

- name: Install alsa and udev
run: sudo apt-get update; sudo apt-get install --no-install-recommends libasound2-dev libudev-dev

Thanks! I just figured that out. 😅

Will be fixing it in a few. ;)

BTW, should I use ubuntu-latest for this job or is it easier to run it on other platform? Any preferences on that?

@CleanCut

CleanCut commented Mar 7, 2021

Copy link
Copy Markdown
Member

BTW, should I use ubuntu-latest for this job or is it easier to run it on other platform? Any preferences on that?

ubuntu-latest is probably fine. We've generally leaned that way since it has been fastest (or near fastest) while also having very predictable behavior.

@mnmaita mnmaita marked this pull request as draft March 7, 2021 18:01
@mnmaita mnmaita force-pushed the feat/add-docs-deadlinks-ci-check branch from f88b77a to 74255e2 Compare March 7, 2021 18:04
@Ratysz Ratysz added A-Build-System Related to build systems or continuous integration C-Docs An addition or correction to our documentation github_actions labels Mar 7, 2021
@mnmaita mnmaita force-pushed the feat/add-docs-deadlinks-ci-check branch from 74255e2 to 084674f Compare March 7, 2021 18:11
@mnmaita mnmaita marked this pull request as ready for review March 7, 2021 18:19
@CleanCut

CleanCut commented Mar 7, 2021

Copy link
Copy Markdown
Member

The dead links have to be fixed as well! Otherwise this PR won't pass CI. 😆

@mockersf

mockersf commented Mar 7, 2021

Copy link
Copy Markdown
Member

I... think there are too many dead links to ask it as part of this PR. Around 7000?

We could:

  • make it ignore failures for now, as a mean to track
  • or open a tracking issue with the dead links split in logical groups to share the effort

@CleanCut

CleanCut commented Mar 7, 2021

Copy link
Copy Markdown
Member

I... think there are too many dead links to ask it as part of this PR. Around 7500?

That number is high enough to make me suspicious that it's not functioning as intended.

@mockersf

mockersf commented Mar 7, 2021

Copy link
Copy Markdown
Member

I checked a few, it seems they all come from methods from the crate downcast-rs

This will make the job a source of information on which links are broken, since a considerable amount is missing from the docs.
@mnmaita

mnmaita commented Mar 8, 2021

Copy link
Copy Markdown
Member Author

I changed this so the deadlinks check doesn't fail, is this acceptable?

I'm not 100% sure on how these are fixed to be honest, any tips?

@CleanCut

CleanCut commented Mar 8, 2021

Copy link
Copy Markdown
Member

I checked a few, it seems they all come from methods from the crate downcast-rs

Hmm...links from documentation of dependencies of Bevy is probably not helpful.

@mockersf

mockersf commented Mar 8, 2021

Copy link
Copy Markdown
Member

ok, after digging more :

  • errors from downcast-rs can be "fixed" by running cargo doc --all-features && cargo deadlinks --dir target/doc/bevy instead of cargo deadlinks
  • errors from tracing should be fixed on their next release (0.2 with some breaking changes)
  • an issue in rustdoc that added an anchor to a section that didn't exist (when implementing Deref for a target that didn't have methods, probably fixed by rust-lang/rust@61c8aae, should be ok in next stable)
  • some issues with rust doc itself

so... I think this is not worth it right now, maybe take a look after tracing release 0.2

@cart

cart commented Mar 9, 2021

Copy link
Copy Markdown
Member

Thanks for looking into it! (I agree)

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Mar 10, 2021
@mockersf

Copy link
Copy Markdown
Member

fixed a few dead links from rust std: rust-lang/rust#82950, that will be available in two versions so... 2021/05/06

@MinerSebas

MinerSebas commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

What happens if you --no-deps to cargo doc command?
I suspect it will supress the dependencies errors, but the question is whether it bevy documention will now show errors, due to missing them.

@CleanCut

Copy link
Copy Markdown
Member

@MinerSebas makes a good point that there's no need to check dependencies' documentation links. 👍🏼

@mnmaita

mnmaita commented Mar 14, 2021

Copy link
Copy Markdown
Member Author

What happens if you --no-deps to cargo doc command?
I suspect it will supress the dependencies errors, but the question is whether it bevy documention will now show errors, due to missing them.

So should I add --no-deps or replace --all-features?. I did some changes here based on a suggestion, please check that too since I'm kinda playing by ear here 😅.

BTW, I've read that cargo-deadlinks "runs cargo doc for you" so I'm wondering if there's a way to pass down args to that...

@mockersf

Copy link
Copy Markdown
Member

I'm wondering if there's a way to pass down args to that...

Yup, you run cargo doc yourself, with the args you want, then cargo deadlinks --dir path/to/the/doc/you/built

@CleanCut

Copy link
Copy Markdown
Member

To answer your first question: Add --no-deps to your cargo doc you run separately.

@mnmaita

mnmaita commented Apr 20, 2021

Copy link
Copy Markdown
Member Author

To answer your first question: Add --no-deps to your cargo doc you run separately.

It's done. I've also updated the branch.

@CleanCut CleanCut left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, thanks!

@CleanCut CleanCut added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Blocked This cannot move forward until something else changes labels Apr 20, 2021
@cart

cart commented Apr 22, 2021

Copy link
Copy Markdown
Member

Looks good to me. Lets merge this! Anyone want to volunteer to create a tracking issue / lead the effort for resolving deadlinks so we can make this job fail on deadlink detection?

@cart

cart commented Apr 22, 2021

Copy link
Copy Markdown
Member

bors r+

bors Bot pushed a commit that referenced this pull request Apr 22, 2021
Closes #1579

This is my first contribution to this repository, feel free to correct anything that I'm missing and I'll address feedback as soon as possible!
@mockersf mockersf mentioned this pull request Apr 22, 2021
3 tasks
@mockersf

Copy link
Copy Markdown
Member

#1983 for tracking known dead link sources

@bors

bors Bot commented Apr 22, 2021

Copy link
Copy Markdown

@bors bors Bot changed the title Adds docs deadlinks check on CI [Merged by Bors] - Adds docs deadlinks check on CI Apr 22, 2021
@bors bors Bot closed this Apr 22, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
Closes bevyengine#1579

This is my first contribution to this repository, feel free to correct anything that I'm missing and I'll address feedback as soon as possible!
@mnmaita mnmaita deleted the feat/add-docs-deadlinks-ci-check branch March 30, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Build-System Related to build systems or continuous integration C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CI to check for dead links in docs

7 participants