github linkcheck anchor change in 4.1.0 break some usage

[alex]

Describe the bug

Given a link like:

.. _`OpenSSL's test vectors`: https://github.com/openssl/openssl/blob/97cf1f6c2854a3a955fd7dd3a1f113deba00c9ef/crypto/evp/evptests.txt#L232 

in a github doc, with release 4.1.0 this will fail with linkcheck, while previously it worked.

How to Reproduce

$ git clone https://github.com/pyca/cryptography
$ cd cryptography
$ tox -e docs-linkcheck

Expected behavior

It passes.

Your project

https://github.com/pyca/cryptography

Screenshots

No response

OS

Linux

Python version

3.9.5

Sphinx version

4.1.0

Sphinx extensions

No response

Extra tools

No response

Additional context

The relevant change happened in 92335bd

Failing test logs: https://github.com/pyca/cryptography/runs/3046691393

[alex]

A simple fix here would be for the linkcheck logic to simply exclude links of the form L\d+.

But of course really the current logic is broken for any link that's generated by github itself and thus doesn't have this prefix.

[sbesson]

But of course really the current logic is broken for any link that's generated by github itself and thus doesn't have this prefix.

Agreed. Here's a subset of native GitHub anchor prefixes that are currently broken in Sphinx 4.1.0: #L, #issuecomment-, #pullrequestreview-, #commits-pushed-, #event-, #ref-commit-, #ref-pullrequest. My feeling is that it's going to be hard to maintain an exhaustive list of valid prefixes especially since there does not seem to be any reference authoritative GitHub page listing all of these.

[alex]

Yes, it's a pretty unfortunate circumstance. It's not at all surprising github doesn't make a list of these, I haven't seen any website with a list of their anchor refs!

ATM I'm interested in whatever solution will get this back to a working state as quickly as possible.

[sbesson]

Completely untested but my understanding of the original PR is that it attempts to fix the linkchecking of anchors in the scenario of a rendered rST/Markdown file. As an immediate patch, would it help to add an additional check to the rewrite_github_anchor method of the like of the parsed.path.endswith('.rst') or parsed.path.endswith('.md')? Happy to open a PR to that effect.

[tk0miya]

OMG! I must admit our approach in #9016 is wrong. So we have to reconsider the way to check the anchors in reST/MD files. Until then, we have to fix the broken linkcheck-builder. I think it's better to disable rewrite_github_anchor handler for a while.

[alex]

That'd be fine with me :-)

[astrojuanlu]

In #9260 a new linkcheck-process-uri event was added, which I think it was a good idea. The problem here is that the current rewrite_github_anchor is insufficient - perhaps it should be moved to an extension that folks can install separately?

[tk0miya]

I don't think so. The rewrite_github_anchor's approach is rewriting the anchor of the URLs to correct one. It means the handler must know all kinds of the form of anchors. But the format is not in public. So I think it's difficult to maintain the module. Another idea to resolve the anchors is 1) checking the original anchor exists, and 2) retry the modified anchor when not found.