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

MAINT: Test old and new Sphinx versions #1413

Merged
merged 15 commits into from
Aug 17, 2023
Merged

Conversation

larsoner
Copy link
Contributor

This is a proposal-by-PR:

  1. Test the oldest supported sphinx in tests (I don't think it's being done currently?)
  2. Test the latest sphinx dev version (should help catch stuff like Incompatibility with Sphinx dev #1404)
  3. Refactor test install step into a script (DRY)

No idea if this has been discussed so far but it seemed quick enough to just do it and see what people thought :)

FYI we use a dev test against NumPy/SciPy/matplotlib etc. in MNE-Python and find bugs in their dev branches fairly frequently. As a maintainer it's easy enough to know when a dev failure can be safely ignored for the time being...

@larsoner
Copy link
Contributor Author

Looks like it's already showing a problem with the pyproject.toml / minimum reqs, assuming I'm interpreting correctly that sphinx==4.2 is meant to be the minimum required version:

ERROR: Cannot install pydata-sphinx-theme, pydata-sphinx-theme[doc]==0.13.4.dev0 and sphinx==4.2 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested sphinx==4.2
    pydata-sphinx-theme[doc] 0.13.4.dev0 depends on sphinx>=4.2
    ablog 0.11.0rc2 depends on sphinx>=5.0.0

I'll bump the minimum to 5.0 in this PR but someone should make sure that's the right thing to do!

@larsoner
Copy link
Contributor Author

... I guess it's just a min required for the doc run, so I'll revert that part. But it means the doc run will never test the oldest sphinx version (maybe okay?)

@larsoner
Copy link
Contributor Author

... also added concurrency to the GitHub actions build so that pushing repeated commits (like I did here) cancels previous ones in the same PR

@drammock
Copy link
Collaborator

thank you @larsoner this was on my mental TODO list (I hadn't managed to even open an issue about it yet)

Comment on lines 134 to 137
# TODO: Can't build on dev because of dependency resolution :(
# - os: ubuntu-latest
# python-version: "3.11"
# sphinx-version: "dev"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also can't test the doc build job on sphinx dev because of some dependency problem. Something to look at in the future since catching Sphinx warnings early (as it says are checked in this job) is pretty important I think

Copy link
Collaborator

Choose a reason for hiding this comment

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

argh. yes, definitely want to test against sphinx dev; the events of this week make it obvious that we should have been doing that all along.

@drammock
Copy link
Collaborator


I'll bump the minimum to 5.0 in this PR but someone should make sure that's the right thing to do!

our policy is to support 2 versions back so dropping 4.x support is correct here.

@drammock
Copy link
Collaborator

note to self: the sphinx dev dependency clash is coming from myst-nb which requires sphinx < 6

@larsoner
Copy link
Contributor Author

note to self: the sphinx dev dependency clash is coming from myst-nb which requires sphinx < 6

@choldgraf looks like the last release was in April, might be time for another that doesn't pin to an ancient Sphinx version :)

@larsoner
Copy link
Contributor Author

Legit failure! https://github.com/pydata/pydata-sphinx-theme/actions/runs/5893186049/job/15984213602?pr=1413

Looks like it's due to jdillard/sphinx-sitemap#69 . That repo hasn't been updated in 6 months, can we drop sphinx-sitemap?

@larsoner
Copy link
Contributor Author

All green other than macos taking ages to start. Old (now 5.0) and dev versions now tested in both pytest and doc-build jobs (when using the branch from jdillard/sphinx-sitemap#70 which seems okay until they cut a new version), ready for review/merge from my end

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

not exactly how I would have done it, but still a big step forward and we can always tweak it later. Thanks @larsoner!!

@drammock drammock merged commit dcd82ad into pydata:main Aug 17, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants