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

Setup reproducible docs builds using pip-tools #1969

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Aug 18, 2023

Since the last Sphinx update, the docs build has started failing. It turns out pip-tools doesn't have reproducible builds for docs. Hence, the PR.

Refs: pradyunsg/furo#693.

@webknjaz
Copy link
Member

Somebody should start dogfooding pip-tools...

@atugushev
Copy link
Member Author

Somebody should start dogfooding pip-tools...

@webknjaz I'm kinda lost, what do you mean?

@atugushev
Copy link
Member Author

atugushev commented Aug 18, 2023

Ah, I wasn't aware of the word "dogfooding". You're right. It would be nice to have docs/requirements.in and perhaps dependabot integration. You're reading my mind.

Found nice docs over here.

@atugushev atugushev changed the title Upgrade Sphinx Use pip-tools for compiling docs dependencies Aug 18, 2023
@atugushev atugushev added the skip-changelog Avoid listing in changelog label Aug 18, 2023
@atugushev atugushev changed the title Use pip-tools for compiling docs dependencies Setup reproducible docs builds using pip-tools Aug 18, 2023
@atugushev atugushev marked this pull request as ready for review August 18, 2023 19:07
@atugushev atugushev requested a review from webknjaz August 18, 2023 19:07
@atugushev atugushev mentioned this pull request Aug 18, 2023
4 tasks
Comment on lines 27 to 28
- method: pip
path: .
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there is a way to use pip-sync here?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, I don't like installing from a constraints file.
Here https://github.com/aio-libs/aiohttp/pull/7471/files#diff-cde814ef2f549dc093f5b8fc533b7e8f47e7b32a8081e0760e57d5c25a1139d9R18-R44 I've been playing with a new mechanism for RTD build customization. Try reproducing the same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if there is a way to use pip-sync here?

I guess we can figure it out. I'll try to come up with a solution in a follow-up PR. Thanks for the example, @webknjaz. That's useful.

@@ -23,7 +23,7 @@ build:

python:
install:
- requirements: docs/requirements.txt
- requirements: requirements/docs.txt
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't already have generic requirements, I wouldn't move this to a separate folder. The location under docs/ is closer to where it's actually being used + this idiom is widely copied over the ecosystem. Let's not reinvent the wheel.

Copy link
Member Author

@atugushev atugushev Aug 19, 2023

Choose a reason for hiding this comment

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

Indeed, currently we don't have any generic requirements. I propose, if we're looking to fully embrace pip-tools and create reproducible builds, that we should consider compiling dependencies for the tests as well, which are currently declared in pyproject.toml under extra=testing. I'm under the impression that current approach is less favoured based on this and this comments.

So, the next step – to move the test requirements to requirements/test.{in|txt} from extra=testing in pyproject.toml – seems logical. With this in mind, wouldn't it also be logical to relocate preemptively the docs requirements to a common requirements folder to keep runtime requirements close together, thus eliminating the need to open another PR later on? Would that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Yes to no extra. But it's a bit more complicated since different envs have different trees. So it's not just one file pair but a matrix of them.
I think that the idea that works is to have tox-env based collections of lockfiles, like the one I implemented in #826 (comment).

As for the docs, I'm torn. If we were to have a constraints file following the same principle, it'd be tox-specific and would probably need several env-specific incarnations. Whereas, tox is not the only place where we call sphinx — in RTD we do so as well. The common denominator is the docs/ dir, so it kinda makes sense to bundle everything under it. OTOH, even in my cheroot PoC, I experimented with having the lockfiles under requirements/ and I'm not sure how I feel about it. That was my attempt to come up with something generic/reusable, and it seems to need more polishing still. I think I have requirements under docs/ in all (or most) the other projects.

I guess it's hard for me to decide on one way or the other. Having the docs/ dir self-contained ain't bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the detailed reply. Your insights certainly provided a lot to consider. Following the principle of "not putting the cart before the horse" let's fix the broken docs build first and then make a decision on how to handle the generic requirements. The commit has been reverted in 9beca35 so that we can progress further.

@webknjaz
Copy link
Member

Found nice docs over here.

Yeah, that's a relatively new doc.

@webknjaz webknjaz enabled auto-merge August 21, 2023 15:05
This step prepares for the removal of the testing extra from pyproject.toml
and transitioning towards using requirements/tests.{in,txt}.
@webknjaz webknjaz merged commit 3efb8f9 into jazzband:main Aug 21, 2023
@atugushev atugushev deleted the upgrade-sphinx branch August 21, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Avoid listing in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants