Re-integrate Sphinx's linkcheck into CI configuration #12932

ladislav987 · 2024-11-01T19:08:30Z

Bring Back Link Checks for Documentation (#12474)

Description:
This pull request addresses the need to re-enable link checks as detailed in issue #12474. The following changes were implemented:

Updated Sphinx Version

The latest Sphinx version was added in both requirements.txt and tox.ini to ensure compatibility and functionality with the new configurations.

Enhanced Sphinx Configuration in conf.py

Adjusted linkcheck_workers from 5 to 20 to improve check efficiency.
Added parameters to enhance link check stability:
- linkcheck_timeout = 30
- linkcheck_retries = 2
- linkcheck_anchors = False
- linkcheck_rate_limit_timeout = 2.0
- linkcheck_delay = 2.0
Added commented-out code (optional) to exclude specific directories to avoid check interruptions:

# Exclude documents that are known to cause issues
# linkcheck_exclude_documents = [
#     'old_versions/*',
#     'deprecated_features/*',
# ]

Expanded linkcheck_ignore to avoid unnecessary checks on frequently referenced and stable links:

r"https://github\.com/sponsors/.*",
r"https://pypi\.org/project/pytest.*",
r"https://github\.com/pytest-dev/pytest/issues/.*",

Without r"https://github\.com/sponsors/.*", I get lot "Too Many Requests for url". It seems to be related to missing credentials or tokens for GitHub.

Updated Tox Commands

Modified the Sphinx build command in tox.ini:
- Changed sphinx-build -W -q --keep-going -b linkcheck . _build to sphinx-build -W -q --keep-going -b linkcheck -j auto . _build for using sphinx parallel build option to speed up the process.
Added commented-out (optional) code to facilitate output logging if needed:

; /bin/sh -c "sphinx-build -W -q --keep-going -b linkcheck -j auto . _build > linkcheck_output.txt 2>&1"
;allowlist_externals =
;    /bin/sh

Pierre-Sassoulas

Thank you for the PR ! I have some suggestions, I think the dead code comments should be removed before merging too. Also what makes the sphinx upgrade mandatory ?

Pierre-Sassoulas · 2024-11-01T20:57:21Z

doc/en/conf.py

+    r"https://github\.com/sponsors/.*",
+    r"https://pypi\.org/project/pytest.*",
+    r"https://github\.com/pytest-dev/pytest/issues/.*",


Yes in theory the dot should be escaped in a regex, as it's not a dot, it's "any character", but it works and is simpler imo. (like in the two links above). Or we can make the two links above consistent :)

Suggested change

r"https://github\.com/sponsors/.*",

r"https://pypi\.org/project/pytest.*",

r"https://github\.com/pytest-dev/pytest/issues/.*",

r"https://github.com/sponsors/.*",

r"https://pypi.org/project/pytest.*",

I think we should modify the existing regex line 144 /145 so they accept link to a comment, or remove them entirely in favor of the simpler one you added.

I simplified it to:

linkcheck_ignore = [ "https://blogs.msdn.microsoft.com/bharry/2017/06/28/testing-in-a-cloud-delivery-cadence/", "http://pythontesting.net/framework/pytest-introduction/", r"https://pypi\.org/project/pytest.*", r"https://github\.com/sponsors/.*", r"https://github\.com/pytest-dev/pytest/issues/.*", r"https://github\.com/pytest-dev/pytest/pull/.*", ]

tox.ini

… into chvastas/bring-back-link-checks

ladislav987 · 2024-11-02T12:35:06Z

Thank you for the PR ! I have some suggestions, I think the dead code comments should be removed before merging too. Also what makes the sphinx upgrade mandatory ?

I deleted commented line in code.

The update of Sphinx from version 7 to version 8.1.3 was due to better configuration and performance Improvements in the new version.

Pierre-Sassoulas

Sorry for the delay. Thank you for the PR, the changes look good.

I would expect some links to have to be fixed, isn't there any to fix ? (There was a list of broken link in the original issue.) Also reading the issue I understood that it should be something periodical and automated like pre-commit or dependabot but I'm not sure who this is addressed here.

AUTHORS

webknjaz · 2024-11-11T19:34:09Z

doc/en/conf.py

-    r"https://github.com/pytest-dev/pytest/issues/\d+",
-    r"https://github.com/pytest-dev/pytest/pull/\d+",
+    r"https://pypi\.org/project/pytest.*",
+    r"https://github\.com/sponsors/.*",


Why is this necessary? I don't remember such URLs not working in other projects.

r"https://github.com/pytest-dev/pytest/issues/\d+", r"https://github.com/pytest-dev/pytest/pull/\d+",

this is from the original version. But you're right, this doesn't trigger any issues with link.

Without r"https://github\.com/sponsors/.*", checking will trigger issues:
WARNING: broken link: https://github.com/sponsors/xxx (429 Client Error: Too Many Requests for url: https://github.com/xxx)

It seems to be related to missing credentials or tokens for GitHub but I'm not 100% sure.

With xxx specifically, I suppose? How about with real existing usernames?

webknjaz · 2024-11-11T19:35:04Z

doc/en/conf.py

@@ -141,10 +141,18 @@
 linkcheck_ignore = [
    "https://blogs.msdn.microsoft.com/bharry/2017/06/28/testing-in-a-cloud-delivery-cadence/",
    "http://pythontesting.net/framework/pytest-introduction/",
-    r"https://github.com/pytest-dev/pytest/issues/\d+",
-    r"https://github.com/pytest-dev/pytest/pull/\d+",
+    r"https://pypi\.org/project/pytest.*",


This matches any project page starting with pytest. Like pytesty. Why? I don't think I've seen any problems with this host in linkcheck in the past.

webknjaz · 2024-11-11T19:37:32Z

doc/en/conf.py

-    r"https://github.com/pytest-dev/pytest/pull/\d+",
+    r"https://pypi\.org/project/pytest.*",
+    r"https://github\.com/sponsors/.*",
+    r"https://github\.com/pytest-dev/pytest/issues/.*",


This should probably be more accurate. Such a wildcard would also silence obviously broken URLs like https://github.com/pytest-dev/pytest/pull/zombie that should produce errors otherwise.

webknjaz · 2024-11-11T19:38:30Z

doc/en/requirements.txt

@@ -2,7 +2,8 @@
 pluggy>=1.5.0
 pygments-pytest>=2.3.0
 sphinx-removed-in>=0.2.0
-sphinx>=7
+sphinx>=8.1.3
+sphinx_issues


sphinx-issues is already listed below

Suggested change

sphinx_issues

webknjaz · 2024-11-11T19:39:03Z

tox.ini

+deps =
+    -r{toxinidir}/doc/en/requirements.txt


could you keep such formatting changes out?

webknjaz · 2024-11-11T19:40:10Z

tox.ini

@@ -143,7 +144,7 @@ passenv =
 deps =
    PyYAML
    regendoc>=0.8.1
-    sphinx
+    sphinx>=8.1.3


I wonder if this should instead just reference the requirements file or the base section value...

webknjaz

Thanks for taking time to look into the issue. It looks like this PR mass-ignores a lot of URLs that should instead be validated.

Instead, we should make every ignore regex very precise so we don't suppress too many things and hide problems in doing so.

Additionally, I'd like to ask you to add code comments justifying every single ignore entry you're adding or changing. Here's an example: https://github.com/cherrypy/cheroot/blob/a9c1fe4/docs/conf.py#L124-L125. So far, it looks like some of those are not justified and shouldn't be there, but it's hard to be sure because the explanation is mysteriously missing.

webknjaz · 2024-11-11T19:49:26Z

Also reading the issue I understood that it should be something periodical and automated like pre-commit or dependabot but I'm not sure who this is addressed here.

This can't be run in pre-commit or dependabot. Instead, it should be integrated into GHA.

@ladislav987 it's necessary to add a separate GHA workflow with a job calling tox -e docs-checklinks, following the structure of other GHA workflows. But it'd need to have an additional trigger for scheduled runs, not just push and pull_request.

… into chvastas/bring-back-link-checks

webknjaz · 2024-11-11T20:20:44Z

doc/en/conf.py

+linkcheck_workers = 20
+linkcheck_timeout = 30
+linkcheck_retries = 2
+linkcheck_anchors = False
+linkcheck_rate_limit_timeout = 2.0
+linkcheck_delay = 2.0


What are the defaults of these?

Ladislav Chvastaš and others added 7 commits November 1, 2024 12:51

update python version to 3.12

d60d253

added name

024917a

latest version of sphinx

1cb2f70

latest version of sphinx

9e5a6a1

new configuration

e7b64e9

updated command + alternative

029fb15

Merge branch 'main' into chvastas/bring-back-link-checks

b95e56f

Pierre-Sassoulas added the skip news used on prs to opt out of the changelog requirement label Nov 1, 2024

Pierre-Sassoulas requested changes Nov 1, 2024

View reviewed changes

Ladislav Chvastaš added 4 commits November 2, 2024 13:14

removed dead code

9777e1b

Merge remote-tracking branch 'origin/chvastas/bring-back-link-checks'…

3f2e0c9

… into chvastas/bring-back-link-checks

edited linkcheck_ignore

0bb4166

deleted duplicated deps

3ea3e2f

ladislav987 requested a review from Pierre-Sassoulas November 4, 2024 12:58

ladislav987 and others added 2 commits November 7, 2024 17:46

Merge branch 'main' into chvastas/bring-back-link-checks

22f5296

Merge branch 'main' into chvastas/bring-back-link-checks

683121f

Pierre-Sassoulas reviewed Nov 10, 2024

View reviewed changes

Merge branch 'main' into chvastas/bring-back-link-checks

7128c3d

webknjaz reviewed Nov 11, 2024

View reviewed changes

AUTHORS Outdated Show resolved Hide resolved

webknjaz reviewed Nov 11, 2024

View reviewed changes

fix misplaced name

cabba81

webknjaz requested changes Nov 11, 2024

View reviewed changes

webknjaz changed the title ~~Chvastas/bring back link checks~~ Re-integrate Sphinx's linkcheck into CI configuration Nov 11, 2024

Ladislav Chvastaš added 3 commits November 11, 2024 21:01

fix duplicity

b4655f9

fix formatting

3e48d69

Merge remote-tracking branch 'origin/chvastas/bring-back-link-checks'…

dfaada7

… into chvastas/bring-back-link-checks

webknjaz reviewed Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-integrate Sphinx's linkcheck into CI configuration #12932

Re-integrate Sphinx's linkcheck into CI configuration #12932

ladislav987 commented Nov 1, 2024

Pierre-Sassoulas left a comment

Pierre-Sassoulas Nov 1, 2024

ladislav987 Nov 2, 2024

ladislav987 commented Nov 2, 2024

Pierre-Sassoulas left a comment

webknjaz Nov 11, 2024

ladislav987 Nov 11, 2024

ladislav987 Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz Nov 11, 2024

ladislav987 Nov 11, 2024

webknjaz Nov 11, 2024

ladislav987 Nov 11, 2024

webknjaz Nov 11, 2024

webknjaz left a comment

webknjaz commented Nov 11, 2024

webknjaz Nov 11, 2024

Re-integrate Sphinx's linkcheck into CI configuration #12932

Are you sure you want to change the base?

Re-integrate Sphinx's linkcheck into CI configuration #12932

Conversation

ladislav987 commented Nov 1, 2024

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladislav987 commented Nov 2, 2024

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz commented Nov 11, 2024

Choose a reason for hiding this comment