Skip to content

bpo-30487: automatically create a venv and install Sphinx when running make#4346

Merged
zware merged 3 commits intopython:masterfrom
cjrh:auto-venv-docbuilder
Nov 26, 2017
Merged

bpo-30487: automatically create a venv and install Sphinx when running make#4346
zware merged 3 commits intopython:masterfrom
cjrh:auto-venv-docbuilder

Conversation

@cjrh
Copy link
Copy Markdown
Contributor

@cjrh cjrh commented Nov 9, 2017 •
edited by bedevere-bot
Loading

#1743 went horribly wrong, and there are now too many people automatically subscribed to that PR so I closed it. Now that I've fixed the feature branch, I've reopened the PR here. @zware

https://bugs.python.org/issue30487

zware
zware requested changes Nov 10, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@zware zware left a comment

Choose a reason for hiding this comment

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

Fixing these comments should be enough to make Travis happy; I'll try to get another look at it after that :)

Doc/Makefile Outdated
Copy link
Copy Markdown
Member

@zware zware Nov 9, 2017

Choose a reason for hiding this comment

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

This should be a dependency of this target (after the : on the line above); it's not a command on its own.

Doc/Makefile Outdated
Copy link
Copy Markdown
Member

@zware zware Nov 9, 2017

Choose a reason for hiding this comment

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

This message should still go after the actual creation of the venv.

Doc/Makefile Outdated
Copy link
Copy Markdown
Member

@zware zware Nov 10, 2017

Choose a reason for hiding this comment

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

This should also check whether $(SPHINXBUILD) --version > /dev/null 2>&1 works, and only build the venv if it doesn't.

Copy link
Copy Markdown
Contributor Author

@cjrh cjrh Nov 11, 2017

Choose a reason for hiding this comment

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

I fixed your other comments. The tests pass. This, about checking whether the command runs, is a bit more interesting. Since the venv Makefile action will only run if the venv dir is absent (is that right?), it means that $(SPHINXBUILD) could never work, until the venv is created. So to get into the body, we need the situation to be that the venv has just been created. Therefore, (in this situation), $(SPHINXBUILD) could never work. So why check for it? (fyi all my experience with makefiles has been on this issue; feel free to assume that know very little about how it works :).

The other, separate complication is that blurb now uses the same venv so we have to be careful to not mess up anything to do with that other tool. I've run make html several times. It looks ok right now, and the blurb action appears to work correctly.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 10, 2017

A Python core developer 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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@cjrh
Copy link
Copy Markdown
Contributor Author

cjrh commented Nov 11, 2017

I'll try to makes the fixes as soon as I can. Hopefully next day or two.

@cjrh cjrh force-pushed the auto-venv-docbuilder branch from 8ff7e90 to bef5c57 Compare November 11, 2017 06:21
@zware
Copy link
Copy Markdown
Member

zware commented Nov 14, 2017

Let me know when you're ready for me to take another look (see @bedevere-bot's message above).

@cjrh
Copy link
Copy Markdown
Contributor Author

cjrh commented Nov 14, 2017 •
edited
Loading

@zware thx for checking in. You can look now. Note my comment that starts with "I fixed your other comments. The tests pass. ". I was perhaps unclear there, but I'm basically asking: since the venv block in the Makefile will only run if venv dir is missing, "$(VENVDIR)/bin/sphinx-build" can never succeed until the venv is built; so why check whether $(SPHINXBUILD) --version > /dev/null 2>&1 works?

Also, it was necessary to change the "$(VENVDIR)/bin/sphinx-build" mentioned in the previous paragraph to "PATH=$(VENVDIR)/bin:$$PATH sphinx-build", due to upstream changes. You will see this in the diff.

Next, management of blurb is now also tied to management of Sphinx, since they share a virtualenv. The permutations get complicated if the user can set their own blurb, but not sphinx, or vice versa, or both, or neither, etc. I think that is all overkill and we should make the simple case work well, but it's your feature, tell me how you want it to be :)

@zware zware merged commit d8d6b91 into python:master Nov 26, 2017
@zware
Copy link
Copy Markdown
Member

zware commented Nov 26, 2017

Thanks for the patch!

ned-deily added a commit to ned-deily/cpython that referenced this pull request Nov 27, 2017
@ned-deily
Revert "bpo-30487: automatically create a venv and install Sphinx whe…
9e6ea79 
…n running make (pythonGH-4346)"

Fix breakage documented in bpo-32149.
This reverts commit d8d6b91.
@ned-deily ned-deily mentioned this pull request Nov 27, 2017
Merged
ned-deily added a commit that referenced this pull request Nov 27, 2017
@ned-deily
Revert "bpo-30487: automatically create a venv and install Sphinx whe…
122fc13 
…n running make (GH-4346)" (#4592)

Fix breakage documented in bpo-32149.
This reverts commit d8d6b91.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cjrh @bedevere-bot @zware @the-knights-who-say-ni