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

Make linkcheck2 detection automatic #2181

Closed

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Dec 30, 2024

No description provided.

@jieyouxu jieyouxu added S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content A-CI Area: CI A-linkcheck Area: linkcheck WG-rustc-dev-guide Working group: rustc-dev-guide labels Dec 30, 2024
@marxin
Copy link
Contributor Author

marxin commented Dec 31, 2024

CC: @camelid

@Kobzol
Copy link
Contributor

Kobzol commented Jan 5, 2025

Sorry, due to me messing up a git operation, we sadly had to force-push the whole commit history of rustc-dev-guide :( If you'd like to update this pull request, you will have to rebase it in a special way onto the new commit history (the new master):

git fetch origin --all
git checkout <pr-branch>
git rebase --onto origin/master origin/master-old
git push --force-with-lease

More context can be found here.

@marxin marxin force-pushed the make-mdbook-linkcheck2-optional branch from b0904f8 to 2aa0212 Compare January 6, 2025 07:57
@Kobzol
Copy link
Contributor

Kobzol commented Jan 6, 2025

When I don't have the mdbook-linkcheck2 and try to run mdbook build, it fails with this error:

2025-01-06 09:55:26 [INFO] (mdbook::book): Book building has started
2025-01-06 09:55:26 [INFO] (mdbook::book): Running the html backend
2025-01-06 09:55:28 [INFO] (mdbook::book): Running the linkcheck backend
2025-01-06 09:55:28 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
WARNING: Skipping link check. Consider running 'cargo install mdbook-linkcheck2'.
2025-01-06 09:55:28 [WARN] (mdbook::renderer): Error writing the RenderContext to the backend, Broken pipe (os error 32)

I assume that is not expected?

@camelid
Copy link
Member

camelid commented Jan 6, 2025

The RenderContext message is unrelated to mdbook-linkcheck2 and linkchecking in general AFAICT. I'm not sure why it's happening, but note that it's a warning not an error. The command still finishes successfully.

@Kobzol
Copy link
Contributor

Kobzol commented Jan 6, 2025

Sorry, I didn't notice that, the "Error" in the warning message threw me off :)

Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

I lean against doing this change for a couple of reasons:

  • Binary detection can lead to weird inconsistencies where different environments lead to different behaviors. E.g. one person may have linkcheck2 installed for use in other projects but another doesn't, and it will be confusing if their build behaves differently.
  • Running linkchecking wastes a bit of your time and network resources locally (especially for people who have bad internet). Since it's run in CI anyway, it seems unnecessary to also run it by default locally (for people who already have the binary).
  • Nearly all of the time new links you're adding are going to be correct because you just copy and paste them. Links usually only break over time.
  • It's not much of a hassle to add ENABLE_LINKCHECK=1 locally if you want it.

@marxin
Copy link
Contributor Author

marxin commented Jan 8, 2025

I lean against doing this change for a couple of reasons:

Works for me!

@marxin marxin closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI A-linkcheck Area: linkcheck S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content WG-rustc-dev-guide Working group: rustc-dev-guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants