-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
Document the make venv documentation build target
#2953
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
Conversation
Doc/Makefile
Outdated
|
|
||
| # You can set these variables from the command line. | ||
| PYTHON = python3 | ||
| VENVDIR = env |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| build the HTML output files:: | ||
|
|
||
| On Windows, set the PYTHON environment variable instead. | ||
| make html PYTHON=./env/bin/python |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? |
| build the HTML output files:: | ||
|
|
||
| On Windows, set the PYTHON environment variable instead. | ||
| make html PYTHON=./env/bin/python |
There was a problem hiding this comment.
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 Once you have made the requested changes, please leave a comment |
To help with GH-2719.