bpo-44756: in ./Doc, make build depends on make html
#27403
Conversation
* venv rule is now conditional, and only does anything if $VENVDIR does not exist * add rule "clean-venv" * as a result, rules dependent on build are now dependent on venv: * html * latex * etc. * update Doc/README.rst appropriately. Now users usually only need to type "make html"
| -rm -rf build/* | ||
|
|
||
| clean-venv: | ||
| rm -rf $(VENVDIR) |
There was a problem hiding this comment.
Note this change:
- rm -rf $(VENVDIR)/*
+ rm -rf $(VENVDIR)
The reason for the change is that it simplifies the conditional action in the venv rule:
# this works but smells bad because it's arbitrarily checking only venv/bin
[ -d $(VENVDIR)/bin ]
# checking venv/bin, venv/lib, venv/include seems superfluous
[ -d $(VENVDIR) ] # this is niceHowever, rm -rf'ing an additional level higher is always worth a close review.
There was a problem hiding this comment.
I note that you're no longer ignoring the exit code here. Since -f is used anyway, the likelihood of a non-zero exit code from rm is indeed low but it can still fail due to wrong permissions. Arguably, your current version is therefore better then.
| environment directory will be with the ``VENVDIR`` variable. | ||
|
|
||
| On Windows, we try to emulate the Makefile as closely as possible with a | ||
| ``make.bat`` file. If you need to specify the Python interpreter to use, |
There was a problem hiding this comment.
I am not a Windows user, and I do not know if make.bat needs changes as well.
There was a problem hiding this comment.
make.bat doesn't use a virtual environment, so nothing to do here.
| -rm -rf build/* | ||
|
|
||
| clean-venv: | ||
| rm -rf $(VENVDIR) |
There was a problem hiding this comment.
I note that you're no longer ignoring the exit code here. Since -f is used anyway, the likelihood of a non-zero exit code from rm is indeed low but it can still fail due to wrong permissions. Arguably, your current version is therefore better then.
| environment directory will be with the ``VENVDIR`` variable. | ||
|
|
||
| On Windows, we try to emulate the Makefile as closely as possible with a | ||
| ``make.bat`` file. If you need to specify the Python interpreter to use, |
There was a problem hiding this comment.
make.bat doesn't use a virtual environment, so nothing to do here.
|
@ambv: Please replace |
|
Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9. |
) - venv rule is now conditional, and only does anything if $VENVDIR does not exist - add rule "clean-venv" (cherry picked from commit d22c876) Co-authored-by: Jack DeVries <[email protected]>
|
GH-27410 is a backport of this pull request to the 3.10 branch. |
) - venv rule is now conditional, and only does anything if $VENVDIR does not exist - add rule "clean-venv" (cherry picked from commit d22c876) Co-authored-by: Jack DeVries <[email protected]>
|
GH-27411 is a backport of this pull request to the 3.9 branch. |
…H-27410) - venv rule is now conditional, and only does anything if $VENVDIR does not exist - add rule "clean-venv" (cherry picked from commit d22c876) Co-authored-by: Jack DeVries <[email protected]>
…H-27411) - venv rule is now conditional, and only does anything if $VENVDIR does not exist - add rule "clean-venv" (cherry picked from commit d22c876) Co-authored-by: Jack DeVries <[email protected]>
…hon#27403)" This reverts commit d22c876.
|
There is something confusing about the commit title. It says “X depends on Y.” It doesn’t say (and I don’t know the situation well enough to deduce) whether that is good or bad, and whether the commit makes it so or removes the dependency. (Reading the code confuses me even more because Y is not actually mentioned in the diff.) Please do not write such ambiguous commit titles. Alway phrase it in terms of what the commit changes. (And reviewers, please fix the commit title if a contributor writes such a commit title — it seems that there are projects where this style is actually preferred, and we can’t educate all contributors, so we have to do this in the review.) |
|
Alright, @gvanrossum, will do. In this case the typical "Make X do Y" unambiguous commit title would have been somewhat confusing on its own when X starts with "make" so I left it alone. Will do better next time. |
|
Great. But shouldn’t the title have said “venv” instead of “html”? AFAICT “html” actually depends on “build” and that hasn’t changed. |
6 participants
exist
"make html"
https://bugs.python.org/issue44756