Skip to content

Docs: Create venv if missing - #98266

Closed
hugovk wants to merge 4 commits into
python:mainfrom
hugovk:update-docs-makefile
Closed

Docs: Create venv if missing#98266
hugovk wants to merge 4 commits into
python:mainfrom
hugovk:update-docs-makefile

Conversation

@hugovk

Copy link
Copy Markdown
Member

Problem

I started getting this error:

$ make -C Doc cleanrm -rf ./venvrm -rf build/*
$ make -C Doc htmlmkdir -p buildBuilding NEWS from Misc/NEWS.d with blurbPATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees -j auto -W . build/htmlRunning Sphinx v5.1.1making output directory... doneTheme error:no theme named 'python_docs_theme' found (missing theme.conf?)make: *** [build] Error 2

That's because I have no venv so it can't find the theme.

The problem is we have this, which looks for Sphinx in the venv or$PATH:

SPHINXBUILD = PATH=$(VENVDIR)/bin:$$PATH sphinx-build

And in my case it's finding it in my $PATH, so this guard passes:

elif $(BLURB) help >/dev/null 2>&1 && $(SPHINXBUILD) --version >/dev/null 2>&1; then \

But I don't get this warning:

cpython/Doc/Makefile

Lines 65 to 66 in b863b9c

echo "Missing the required blurb or sphinx-build tools."; \
echo "Please run 'make venv' to install local copies."; \

Fix

Two things (aka let's do it like the devguide)

  1. let's just use the tools in the venv, not $PATH (to make the guard fail, and show the warning):

https://github.com/python/devguide/blob/05f6d0c8d09bd757440e0346190dbd62e06c7251/Makefile#L9-L10

  1. and add ensure-venv target that will install the venv if the venv dir is missing (to avoid the warning in the first place):

https://github.com/python/devguide/blob/05f6d0c8d09bd757440e0346190dbd62e06c7251/Makefile#L58-L64

Bonus

Whilst we're changing Docs/Makefile, PR #98189 recently fixed some missing .PHONY targets. 👍

I expect missing .PHONY targets will happen again, because it's normal to copy/paste a target, and forget (or not know) to update the long .PHONY line right at the top.

Let's do as @zware suggested and define them right next to each target: #98189 (comment)

We do this at Pillow and at work, it helps a lot. (I'll add it to the devguide and PEPs too.)

@zware

Copy link
Copy Markdown
Member

We've been down this road before and it was either not accepted or shortly reverted, though I don't remember why. There is some history to dig up and review here, though past rejection doesn't necessarily mean we must still stick to the status quo.

@JelleZijlstra

Copy link
Copy Markdown
Member

See #27635 and issues #88919 and #88986. Because of a similar change we ended up with Pablo's venv in the distributed 3.10 tarballs.

@hugovk

hugovk commented Oct 21, 2022

Copy link
Copy Markdown
MemberAuthor

Right, so we need to be able to build using pre-downloaded tools not in the venv, so fix 1 and 2 are not needed.


We could add a check for other missing tools in this guard:

cpython/Doc/Makefile

Lines 55 to 59 in b863b9c

elif $(BLURB) help >/dev/null 2>&1 && $(SPHINXBUILD) --version >/dev/null 2>&1; then \
if [ -d ../Misc/NEWS.d ]; then \
echo "Building NEWS from Misc/NEWS.d with blurb"; \
$(BLURB) merge -f build/NEWS; \
else \

In this case, python_docs_theme was missing, and isn't easy to check for. So I guess "if in doubt, make clean venv applies :)

(I think I've seen a similar problem for another tool that could be checked easily, but I can't remember what it was so I'll address it if it comes up again.)


What's left? I think the bonus .PHONY change is still useful. Shall I refactor this PR or make a new, cleaner one?

@hugovk

Copy link
Copy Markdown
MemberAuthor

What's left? I think the bonus .PHONY change is still useful. Shall I refactor this PR or make a new, cleaner one?

I went for a new, clean PR, please see #99396.

And let's close this one, thanks for the background!

@hugovkhugovk closed this Nov 11, 2022
@hugovk
hugovk deleted the update-docs-makefile branch November 11, 2022 20:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hugovk@zware@JelleZijlstra@bedevere-bot