Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Mar 7, 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!

@CleanCut
Copy link
Member

CleanCut commented Mar 7, 2021

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
Copy link
Member Author

mnmaita commented Mar 7, 2021

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
Copy link
Member

CleanCut commented Mar 7, 2021

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
Copy link
Member

CleanCut commented Mar 7, 2021

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

@mockersf
Copy link
Member

mockersf commented Mar 7, 2021

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
Copy link
Member

CleanCut commented Mar 7, 2021

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
Copy link
Member

mockersf commented Mar 7, 2021

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
Copy link
Member Author

mnmaita commented Mar 8, 2021

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
Copy link
Member

CleanCut commented Mar 8, 2021

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
Copy link
Member

mockersf commented Mar 8, 2021

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
Copy link
Member

cart commented Mar 9, 2021

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
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
Copy link
Contributor

MinerSebas commented Mar 12, 2021

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
Member

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

@mnmaita
Copy link
Member Author

mnmaita commented Mar 14, 2021

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
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
Member

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

@mnmaita
Copy link
Member Author

mnmaita commented Apr 20, 2021

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

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

Copy link
Member

@CleanCut CleanCut left a comment

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
Copy link
Member

cart commented Apr 22, 2021

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
Copy link
Member

cart commented Apr 22, 2021

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
Member

#1983 for tracking known dead link sources

@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