Removing the sphinx constraint on devel

Hi all,

We recently bumped the sphinx version on the devel branch of ansible/ansible-documentation. This is pinned in the constraints.in file here: ansible-documentation/tests/constraints.in at bb7e06f49d81b02e42f32efba612566f5887df83 · ansible/ansible-documentation · GitHub

While doing this, it was pretty clear that sphinx hadn’t been updated in quite some time. It was set to 7.2.5 almost three years ago in Update sphinx to 7.2.5 and remove constraint (#365) · ansible/ansible-documentation@b7158c5 · GitHub

The reasoning to add sphinx to the constraints.in was probably based on the fact that we didn’t have nightly builds of devel or the workflows to build the package docs via GHA when we initially set up the docs repo. Now that those mechanisms are in place, I think we should revisit things.

If we remove sphinx from constraints.in on the devel branch, when a new version is published the pip-compile workflow will bump the package version in tests/requirements.txt (for example, on this line: ansible-documentation/tests/requirements.txt at bb7e06f49d81b02e42f32efba612566f5887df83 · ansible/ansible-documentation · GitHub).

This dep bump will come through a [devel] ci: refresh docs build dependencies pull request such as this one: [devel] ci: refresh docs build dependencies by ansible-documentation-bot[bot] · Pull Request #3758 · ansible/ansible-documentation · GitHub

That pull request will catch any broken builds via the nox -s checkers(docs-build) session. We’ll also catch issues with package docs via the scheduled builds for devel docs workflow: ansible-documentation/.github/workflows/build-devel-docs.yaml at devel · ansible/ansible-documentation · GitHub

If issues do arise with the new sphinx version, we can temporarily add the pin back to the constraints.in file while we figure things out. In any case, the issues will be limited to the devel branch where it should be acceptable for things to break from time to time.

The benefit with all this is that someone won’t need to remember to periodically check for new sphinx versions and do manual bumps. Not to mention being able to take advantage of the latest and greatest bells and whistles in sphinx. We might even uncover issues in sphinx itself and contribute back.

When it comes time to cut a new stable branch off devel, we could take the most recent good version of sphinx from tests/requirements.txt and add that to the constraints.in file on the fresh stable branch to ensure that latest is always pinned. Although I’m not sure if that’s necessary because the pip-compile workflow for doc build dependencies runs against devel only. But we could still do it for consistency with other stable branches.

Looking forward to hearing if others have any thoughts on this. Thank you.

:+1: for removing the pin in constraints.in from my side.

1 Like

Removing the constraint makes sense for the reasons you mentioned, but we should make sure to run a full package docs test build before merging any dep update PRs that bump the Sphinx version, instead of relying on the scheduled CI job to eventually catch any issues.

I slightly lean toward explicitly pinning a Sphinx version in constraints.in for stable branches but don’t feel strongly. If we do make this change, we should document the necessary steps in the branching guide in MAINTAINERS.md.

1 Like

It turns out that removing the Sphinx == 9.1.0 pin actually reverts Sphinx back to 7.3.7, since we also pin rstcheck < 6, which in turn pins docutils < 0.19, which in turn apparently forces Sphinx to 7.3.7.

Since the docs requirements were never compiled after the un-pin PR was merged (and the un-pin PR didn’t refresh them), we never noticed until now.

Ref: [devel] ci: refresh docs build dependencies by ansible-documentation-bot[bot] · Pull Request #3949 · ansible/ansible-documentation · GitHub
Ref: [devel] ci: refresh docs build dependencies by ansible-documentation-bot[bot] · Pull Request #3949 · ansible/ansible-documentation · GitHub

Nuts. :chipmunk:

I commented in the pull request @felixfontein but suspect we’ll want to add that Sphinx version pin back now. Thanks for catching that (and for the new version of antsibull-docs).

I tried adding a lower bound on Sphinx instead of re-adding the pin, that also seems to work: [devel] ci: refresh docs build dependencies by ansible-documentation-bot[bot] · Pull Request #3951 · ansible/ansible-documentation · GitHub

1 Like