Skip to content
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

Fixing doc building with newer pip versions #189

Merged
merged 2 commits into from
Jul 26, 2015
Merged

Fixing doc building with newer pip versions #189

merged 2 commits into from
Jul 26, 2015

Conversation

xentec
Copy link
Contributor

@xentec xentec commented Jul 25, 2015

CalledProcessError should only be raised, if 'No command ...' is in stderr, not otherwise.

@xentec
Copy link
Contributor Author

xentec commented Jul 25, 2015

Well, this is interesting: Travis somehow fails on the opposite, indicating that his pip version does not support show.

Is the check for show support even necessary?

@vitaut
Copy link
Contributor

vitaut commented Jul 26, 2015

Thanks for the PR, but the logic looks correct (although the comment could be better). pip show is used to check if the package is already installed. If the show command is not supported pip gives error "No command by the name pip show" which is ignored and the installation continues, otherwise a CalledProcessError exception is thrown.

Is the check for show support even necessary?

Yes, the check is necessary because the old version of pip installed on travis doesn't support the show command.

Is there any problem when you run unchanged build.py?

@xentec
Copy link
Contributor Author

xentec commented Jul 26, 2015

Yes, I get the CalledProcessError when building it on Arch Linux, because p.returnCode is 1 but stderr (and stdout) is empty.
Using virtualenv 13.1.0 and pip 7.1.0.

@vitaut
Copy link
Contributor

vitaut commented Jul 26, 2015

Could be a change in pip behavior. What happens when you run pip show somenoninstalledpackage from a command line, does it return nonzero error code and doesn't print anything?

@xentec
Copy link
Contributor Author

xentec commented Jul 26, 2015

Pretty much.

» doc/virtualenv/bin/pip show breathe; echo $?
1
» doc/virtualenv/bin/pip show somenoninstalledpackage; echo $?
1
» doc/virtualenv/bin/pip show sphinx; echo $?
---
Metadata-Version: 2.0
Name: Sphinx
Version: 1.3.1
Summary: Python documentation generator
Home-page: http://sphinx-doc.org/
Author: Georg Brandl
Author-email: georg@python.org
License: BSD
Location: /home/xentec/build/cppformat-git/src/cppformat-git/build/doc/virtualenv/lib/python3.4/site-packages
Requires: Jinja2, alabaster, babel, Pygments, six, sphinx-rtd-theme, snowballstemmer, docutils
0

@vitaut
Copy link
Contributor

vitaut commented Jul 26, 2015

I've tried to modify the logic in build.py to make it work with the new version of pip: 3b224e1

Does it work for you?

@xentec xentec changed the title Fixing doc building Fixing doc building with never pip versions Jul 26, 2015
@xentec xentec changed the title Fixing doc building with never pip versions Fixing doc building with newer pip versions Jul 26, 2015
Some distros changed their default to python3 causing it to fail.
@xentec
Copy link
Contributor Author

xentec commented Jul 26, 2015

I updated my fix, which is not pretty nor very future proof.
Let's just hope this won't need as much tinkering as you needed with MinGW.

vitaut added a commit that referenced this pull request Jul 26, 2015
Fixing doc building with newer pip versions
@vitaut vitaut merged commit 69a0317 into fmtlib:master Jul 26, 2015
@xentec xentec deleted the patch-1 branch July 26, 2015 21:39
@vitaut
Copy link
Contributor

vitaut commented Jul 26, 2015

Merged, thanks!

One way to improve this is to ignore the error from pip show altogether but the current approach is OK too even if it is not particularly pretty.

Let's just hope this won't need as much tinkering as you needed with MinGW.

Haha yeah, MinGW32 was a bit of pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants