Skip to content

Use sys.executable -msphinx instead of sphinx-build. #8269

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

Merged
merged 1 commit into from
Mar 11, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 11, 2017

See #8208 for rationale (using sys.executable instead of python is the same idea).

python -msphinx is implemented since sphinx 1.3 (sphinx-doc/sphinx@74c7a52).

@NelleV
Copy link
Member

NelleV commented Mar 11, 2017

I am +0 on this patch. We're making things complicated because people use conda without understanding the underlying python packaging system. We're putting duck tape instead of teaching people about paths and imports.

@matthew-brett
Copy link
Contributor

Nelle - I don't think this is a conda-specific patch. I've certainly hit this where I'm working in a virtualenv, and I accidentally call a sphinx installed into the system or user directories.

@NelleV
Copy link
Member

NelleV commented Mar 11, 2017

wait… You don't set up virtualenv to be a completely independant environment ?

@NelleV
Copy link
Member

NelleV commented Mar 11, 2017

(Anyhow, I am not against this patch, but I don't think we are solving the underlying problem).

@anntzer
Copy link
Contributor Author

anntzer commented Mar 11, 2017

Well sphinx-build goes by default to /usr/bin/sphinx-build (or equivalent) (and it refers to /usr/bin/python, not /usr/bin/env python -- exactly as it wants to be run in whatever environment it was installed, not whatever environment is active) so unless you want to remove /usr/bin from your PATH (...) when you activate your venv it's still going to be a problem.

@NelleV
Copy link
Member

NelleV commented Mar 11, 2017

It's a question of which one comes first in the path. The local one should always come before /usr/bin. Technically, it should not be a problem unless sphinx is not installed in the local one.

If it is not installed in the local one, then users are having exactly the same problem as running an ipython shell outside of the environment, or pip outside of the environment.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 11, 2017

IPython actually has code to specifically handle that case, which prints a warning and tries to manipulate the PATH correctly (init_virtualenv in interactiveshell.py).

/usr/lib/python3.6/site-packages/IPython/core/interactiveshell.py:724: UserWarning: Attempting to work in a virtualenv. If you encounter problems, please install IPython inside the virtualenv.
  warn("Attempting to work in a virtualenv. If you encounter problems, please "

venvs created by python -mvenv now default to installing pip so unless you get rid of it on purpose (... or are using an ancient version...) you won't have that problem.

@matthew-brett
Copy link
Contributor

Nelle - when I use virtualenvwrapper I have initialization scripts that pull the Python --user binary install directory off the PATH, but when I use virtualenv alone, I have to remember to do that by hand. But it seems reasonable to make this easy mistake less easy, if possible.

@tacaswell
Copy link
Member

I am 👍 on this.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 11, 2017

As a side note, if we up the dependency on sphinx to 1.3 (which is needed for this to work, and part of the PR), that also means we have sphinx.ext.napoleon available which can be used instead of numpydoc.

@NelleV NelleV merged commit 0970c9e into matplotlib:master Mar 11, 2017
@anntzer anntzer deleted the python-msphinx branch March 11, 2017 22:57
@QuLogic
Copy link
Member

QuLogic commented Mar 12, 2017

See #5743 (in need of update, though.)

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

Successfully merging this pull request may close these issues.

5 participants