FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-44756: in ./Doc, `make build` depends on `make html` by jdevries3133 · Pull Request #27403 · python/cpython · GitHub

/ cpython Public

bpo-44756: in ./Doc, make build depends on make html - #27403

Merged
ambv merged 1 commit into
python:mainfrom
jdevries3133:bpo-44756-doc-make
Jul 28, 2021
ConversationCommits (1)ChecksFiles changed
Merged

bpo-44756: in ./Doc, make build depends on make html#27403
ambv merged 1 commit into
python:mainfrom
jdevries3133:bpo-44756-doc-make

Conversation

jdevries3133 commented Jul 28, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor
  • 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"

https://bugs.python.org/issue44756

bpo-44756: in ./Doc, make build depends on make html
1033456 
* 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"
jdevries3133 commented Jul 28, 2021
View reviewed changes
Comment thread Doc/Makefile
-rm -rf build/*

clean-venv:
rm -rf $(VENVDIR)

jdevries3133 Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 nice

However, rm -rf'ing an additional level higher is always worth a close review.

ambv Jul 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jdevries3133 commented Jul 28, 2021
View reviewed changes
Comment thread Doc/README.rst
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,

jdevries3133 Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not a Windows user, and I do not know if make.bat needs changes as well.

ambv Jul 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

make.bat doesn't use a virtual environment, so nothing to do here.

corona10 requested a review from JulienPalard July 28, 2021 05:18
ambv approved these changes Jul 28, 2021
View reviewed changes
Comment thread Doc/Makefile
-rm -rf build/*

clean-venv:
rm -rf $(VENVDIR)

ambv Jul 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Doc/README.rst
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,

ambv Jul 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

make.bat doesn't use a virtual environment, so nothing to do here.

ambv merged commit d22c876 into python:main Jul 28, 2021

bedevere-bot commented Jul 28, 2021

Copy link
Copy Markdown

@ambv: Please replace # with GH- in the commit message next time. Thanks!

miss-islington commented Jul 28, 2021

Copy link
Copy Markdown
Contributor

Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 28, 2021
bpo-44756: in ./Doc, make build depends on make html (pythonGH-27403
3f52cdf 
)

- 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 <58614260+jdevries3133@users.noreply.github.com>
bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 28, 2021

bedevere-bot commented Jul 28, 2021

Copy link
Copy Markdown

GH-27410 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 28, 2021
bpo-44756: in ./Doc, make build depends on make html (pythonGH-27403
933a1c3 
)

- 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 <58614260+jdevries3133@users.noreply.github.com>

bedevere-bot commented Jul 28, 2021

Copy link
Copy Markdown

GH-27411 is a backport of this pull request to the 3.9 branch.

ambv pushed a commit that referenced this pull request Jul 28, 2021
bpo-44756: in ./Doc, make build depends on make html (GH-27403) (G…
f113195 
…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 <58614260+jdevries3133@users.noreply.github.com>
ambv pushed a commit that referenced this pull request Jul 28, 2021
bpo-44756: in ./Doc, make build depends on make html (GH-27403) (G…
fd2c246 
…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 <58614260+jdevries3133@users.noreply.github.com>
jdevries3133 deleted the bpo-44756-doc-make branch August 3, 2021 17:35
jdevries3133 added a commit to jdevries3133/cpython that referenced this pull request Aug 3, 2021
Revert "bpo-44756: in ./Doc, make build depends on make html (pyt…
bb9d515 
…hon#27403)"

This reverts commit d22c876.

gvanrossum commented Aug 4, 2021

Copy link
Copy Markdown
Member

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.)

ambv commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

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.

gvanrossum commented Aug 4, 2021

Copy link
Copy Markdown
Member

Great. But shouldn’t the title have said “venv” instead of “html”? AFAICT “html” actually depends on “build” and that hasn’t changed.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ambv ambv approved these changes
JulienPalard Awaiting requested review from JulienPalard

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL