Document the make venv documentation build target by brettcannon · Pull Request #2953 · python/cpython (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation10 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

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

What's your thinking about changing the name of the venv directory from venv to env? Making the change would introduce a compatibility issue; even though "make venv" wasn't documented, it's likely being used and, besides leaving a stray "venv" directory around, it would also require current doc builders to change their scripts.

Choose a reason for hiding this comment

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

I've just seen more use of env instead than venv out in the wild is all. I can put it back.

Choose a reason for hiding this comment

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

Alas, this is currently insufficient. The sphinx-build command in the venv won't be found without other Makefile changes (it doesn't fail if you already have sphinx-build installed on your PATH). Since we're going to have tweak this a bit once the blurb support is merged in, suggest for now just adding a shell source command prior to the make html, i.e. . ./env/bin/activate or (. ./venv/bin/activate).

Choose a reason for hiding this comment

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

Price I pay for wrapping this up before rushing out the door. 😞

But I think I found a better solution than having to activate the venv blindly: setting SPHINXBUILD to $(PYTHON) -m sphinx. It's the same as sphinx-build but with the simpler setting of PYTHON being properly supported. Does that work for you, @ned-deily ?

Choose a reason for hiding this comment

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

I know I suggested something similar for blurb. But now I'm not sure that was the best choice. It's more problematic with sphinx-build since existing doc builds may have been depending on using an already installed (e.g. system) sphinx-build and have never bothered before to run the make venv step (and before, even if they did run that step, it wouldn't have made a difference without tweaking PYTHON). With this change, those builds would now fail unless sphinx had happened to be installed in whatever python3 on PATH points to. I don't know how many users this would affect but it's not hypothetical: I ran into this myself. There are several ways around it; I was thinking the most straightforward way might be to play with PATH in the "build" rule; so something like:
PATH=./venv/bin:$PATH sphinx-build

Either that or we need to make sure this is documented as an incompatible change and then maybe only for 3.7?

Choose a reason for hiding this comment

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

Another possibility would be to make the build rule depend on the venv rule but, as it stands, that causes venv to be run every time you do any doc build.

Choose a reason for hiding this comment

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

My worry with the PATH=./$(VENVDIR)/bin:$PATH $(SPHINXBUILD) solution is won't this require being set for anything that might have an entry in bin? And that seems a bit more error-prone long-term than this current approach as I can see forgetting to set PATH in some rule.

I'm fine with this being a Python 3.7-only thing if that doesn't cause you grief in 3.6 releases. But if you would rather have the PATH solution I'm fine with it as well. Just let me know your preference and I'll update the PR.

@ned-deily any update on whether my proposed solution works for you?

Choose a reason for hiding this comment

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

I know I suggested something similar for blurb. But now I'm not sure that was the best choice. It's more problematic with sphinx-build since existing doc builds may have been depending on using an already installed (e.g. system) sphinx-build and have never bothered before to run the make venv step (and before, even if they did run that step, it wouldn't have made a difference without tweaking PYTHON). With this change, those builds would now fail unless sphinx had happened to be installed in whatever python3 on PATH points to. I don't know how many users this would affect but it's not hypothetical: I ran into this myself. There are several ways around it; I was thinking the most straightforward way might be to play with PATH in the "build" rule; so something like:
PATH=./venv/bin:$PATH sphinx-build

Either that or we need to make sure this is documented as an incompatible change and then maybe only for 3.7?

A Python core developer, ned-deily, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify ned-deily along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

This pr has been incorporated into and superseded by #3440 and #3441. Thanks, Brett!