Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Docs: Create venv if missing#98266

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
hugovk wants to merge4 commits intopython:mainfromhugovk:update-docs-makefile

Conversation

@hugovk
Copy link
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 venvor$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 inb863b9c

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 addensure-venv target that will install the venv if thevenv 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 changingDocs/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 atPillow and at work, it helps a lot. (I'll add it to the devguide and PEPs too.)

@zware
Copy link
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.

hugovk reacted with thumbs up emoji

@JelleZijlstra
Copy link
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.

zware reacted with thumbs up emojihugovk reacted with eyes emoji

@hugovk
Copy link
MemberAuthor

hugovk commentedOct 21, 2022•
edited
Loading

Right, so we need to be able to build using pre-downloaded toolsnot 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 inb863b9c

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
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 thisNov 11, 2022
@hugovkhugovk deleted the update-docs-makefile branchNovember 11, 2022 20:03
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hugovk@zware@JelleZijlstra@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp