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

[ci] use cargo deadlinks to check for dead links #1021

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Mar 5, 2024

Since cargo deadlinks is just a link-checking wrapper around cargo doc, we can simply replace our doc invocation with deadlinks to get the benefits of both.

# is a link-checking wrapper around `cargo doc`.
export RUSTDOCFLAGS="${{ matrix.toolchain == 'nightly' && '-Z unstable-options --document-hidden-items' || '' }} $RUSTDOCFLAGS $METADATA_DOCS_RS_RUSTDOC_ARGS"
./cargo.sh +${{ matrix.toolchain }} deadlinks --check-intra-doc-links -- --document-private-items --package ${{ matrix.crate }} ${{ matrix.features }}
if: matrix.toolchain != 'msrv'
Copy link
Member

Choose a reason for hiding this comment

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

Explain why?

Also, I'd recommend just nightly. Runs fewer jobs and the behavior of this is very unlikely to differ by toolchain. Notably, our docs.rs docs are only built from nightly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The MSRV of this version of deadlinks is higher than our MSRV.

Restricting to nightly seems reasonable, though.

Comment on lines 277 to 278
# We invoke `cargo deadlinks` instead of `cargo doc`; `cargo deadlinks`
# is a link-checking wrapper around `cargo doc`.
Copy link
Member

Choose a reason for hiding this comment

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

No we don't anymore 🙂

@jswrenn jswrenn force-pushed the deadlinks branch 3 times, most recently from 61ac2c7 to 2022b0c Compare March 5, 2024 16:06
Since `cargo deadlinks` is just a link-checking wrapper around
`cargo doc`, we can simply replace our `doc` invocation with
`deadlinks` to get the benefits of both.
@jswrenn
Copy link
Collaborator Author

jswrenn commented Mar 5, 2024

Hm, I can't replicate the issue here locally: https://github.com/google/zerocopy/actions/runs/8159490578/job/22304041953?pr=1021

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

@@ -277,6 +277,17 @@ jobs:
export RUSTDOCFLAGS="${{ matrix.toolchain == 'nightly' && '-Z unstable-options --document-hidden-items' || '' }} $RUSTDOCFLAGS $METADATA_DOCS_RS_RUSTDOC_ARGS"
./cargo.sh +${{ matrix.toolchain }} doc --document-private-items --package ${{ matrix.crate }} ${{ matrix.features }}

- name: Check dead links
run: |
cargo install -q cargo-deadlinks --version 0.8.1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cargo install -q cargo-deadlinks --version 0.8.1
set -eo pipefail
cargo install -q cargo-deadlinks --version 0.8.1

And honestly, in other jobs (notably, the previous one) while you're at it. We should have it in every job.

@joshlf joshlf mentioned this pull request Sep 20, 2024
87 tasks
@joshlf joshlf added the do-after-next-release Not blocking release, but we should do soon after release label Oct 2, 2024
joshlf added a commit that referenced this pull request Oct 5, 2024
This is a point in favor of landing #1021
@joshlf joshlf mentioned this pull request Oct 5, 2024
joshlf added a commit that referenced this pull request Oct 5, 2024
Release 0.8.2.

This is a point in favor of landing #1021
github-merge-queue bot pushed a commit that referenced this pull request Oct 5, 2024
Release 0.8.2.

This is a point in favor of landing #1021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-after-next-release Not blocking release, but we should do soon after release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants