Skip to content

Possible regression in linkcheck_allowed_redirects default following PR 13452 #13462

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

Open
pllim opened this issue Apr 2, 2025 · 11 comments · May be fixed by #13483
Open

Possible regression in linkcheck_allowed_redirects default following PR 13452 #13462

pllim opened this issue Apr 2, 2025 · 11 comments · May be fixed by #13483
Labels

Comments

@pllim
Copy link

pllim commented Apr 2, 2025

Describe the bug

Hi. Is it possible that #13452 introduced a regression? We see a new failure at astropy/sphinx-astropy#80 for sphinx-dev but we have no changed anything related to linkcheck lately.

The only specific linkcheck setting we touch is this at https://github.com/astropy/sphinx-astropy/blob/9ee5275a8ccc09f434dc54f66993dbd0356d7200/sphinx_astropy/conf/v2.py#L386

linkcheck_timeout = 60

So the warning below is unexpected unless I missed something?

WARNING: The config value `linkcheck_allowed_redirects' has type `NoneType'; expected `dict'.

Hope you can advise. Thanks!

How to Reproduce

Run sphinx-astropy test suite using dev version of sphinx.

Environment Information

See https://github.com/astropy/sphinx-astropy/actions/runs/14198250147/job/39778585162

Sphinx extensions

N/A

Additional context

@jayaddison
Copy link
Contributor

I've begun investigating; we do have some test coverage of projects that include a conf.py that that configures linkcheck_timeout without configuring linkcheck_allowed_redirects -- e.g this one -- and I wasn't able to replicate the problem by performing a repeat-rebuild of a similar minimal project (included below).

conf.py

linkcheck_timeout = 60

index.rst

# testing

Test command:

$ sphinx-build -b linkcheck . _build

Those would seem to me to indicate that the problem exists outside of Sphinx -- but even so, it feels to me that we should continue to track it here; it could be an interaction that is commonplace in the Sphinx extension/theme ecosystem, for example.

I'm planning to join the downstream bugthread to add any relevant information there.

@pllim
Copy link
Author

pllim commented Apr 4, 2025

Thanks for the help, @jayaddison ! I am not expert with the theme code. At a glance, they don't seem to touch a lot of linkcheck stuff that might affect downstream. Hmm.

https://github.com/search?q=repo%3Apydata%2Fpydata-sphinx-theme+redirect&type=code

https://github.com/search?q=repo%3Apydata%2Fpydata-sphinx-theme+linkcheck+redirect&type=code

https://github.com/search?q=repo%3Apydata%2Fpydata-sphinx-theme+linkcheck&type=code

Perhaps @drammock could advise? 🙏

@jayaddison
Copy link
Contributor

jayaddison commented Apr 4, 2025

Test command:

$ sphinx-build -b linkcheck . _build

I can now replicate the problem, after narrowing in on it downstream in astropy/sphinx-astropy#80.

It is not replicable when using the linkcheck builder -- but is replicable for the html builder.

Here is a minimal set of files to replicate the warning, using Sphinx commit 3c4b4e3 as the baseline:

requirements.txt

pydata-sphinx-theme==0.16.1

conf.py

html_theme = 'pydata_sphinx_theme'

index.rst (empty)

Edit: markdown fixup

@drammock
Copy link

drammock commented Apr 4, 2025

I can reproduce locally; this is really odd. The pydata sphinx theme doesn't touch the linkcheck_allow_redirects variable. We do set that value in our own conf.py for building our own docs, but it appears nowhere in our source tree (the sole result is in a comment):

$ git grep linkcheck src
src/pydata_sphinx_theme/translator.py:94:            # some builders, e.g. linkcheck, do not define 'default_translator_class'

@jayaddison
Copy link
Contributor

@AA-Turner I'm not precisely certain what the cause of this problem is yet, other than it is something that occurs in combination with the pydata HTML theme (and perhaps others).

Rolling back to a default value of None (instead of a sentinel) for linkcheck_allow_redirects solves the problem without requiring downstream modifications. I think that might be the simplest approach here. Wdyr?

@jayaddison
Copy link
Contributor

jayaddison commented Apr 4, 2025

tl;dr:

Either setting _sentinel_lar to None, or instead adding type(None) to the permitted types for linkcheck_allowed_redirects is sufficient to resolve the warning.

(doesn't entirely explain why pydata creates a None-valued config var when the sentinel is an object default is a sentinel, but that should be discoverable too)

Edit: clarify a detail

@AA-Turner
Copy link
Member

Note: the error also appears in other themes, like Furo.

I want to avoid having None as an allowed value, so I think a different fix is needed.

A

@jayaddison
Copy link
Contributor

I'm planning to re-investigate this (particularly re: finding a solution that avoids the default None value) over the upcoming weekend.

@jayaddison
Copy link
Contributor

I think the main problem is the following line, where we reset an encountered sentinel value to None:

config.linkcheck_allowed_redirects = None

Removing that prevents the warning from occurring.

The unit tests have to be updated a little to accommodate that, though. I'll plan to open a pull request later today or tomorrow, unless anyone else is planning to!

@jayaddison
Copy link
Contributor

I think the main problem is the following line, where we reset an encountered sentinel value to None:

sphinx/sphinx/builders/linkcheck.py

Line 754 in 3c4b4e3
config.linkcheck_allowed_redirects = None

Removing that prevents the warning from occurring.

The unit tests have to be updated a little to accommodate that, though. I'll plan to open a pull request later today or tomorrow, unless anyone else is planning to!

I've attempted to fix this with #13483 -- however, one of the test_environment.py test cases fails, I think because the default sentinel value for linkcheck_allowed_redirects fails to round-trip (pickle and unpickle) during config serialization.

I'm not sure how to proceed at the moment; I'll think about it a bit more.

@jayaddison
Copy link
Contributor

I've attempted to fix this with #13483 -- however, one of the test_environment.py test cases fails, I think because the default sentinel value for linkcheck_allowed_redirects fails to round-trip (pickle and unpickle) during config serialization.

I'm not sure how to proceed at the moment; I'll think about it a bit more.

The most straightforward solution I've thought of is to use True instead of _sentinel_lar as the default, sentinel value. Would that be acceptable @AA-Turner?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants