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

Ensure invoked Sphinx matches importable one #6965

Merged
merged 8 commits into from May 4, 2020

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Apr 26, 2020

This should fix crashes when both conda and pip have Sphinx installed. It's likely also negligibly faster since it avoids the binary wrappers.

This should fix crashes when both conda and pip have Sphinx installed. It's likely also negligibly faster since it avoids the binary wrappers.
@eric-wieser
Copy link
Contributor Author

@eric-wieser eric-wieser commented Apr 26, 2020

Note that this approach is already what you use for mkdocs.

Copy link
Member

@stsewd stsewd left a comment

We need to test this across all sphinx versions (>=1.5).

But looks like it should work https://github.com/sphinx-doc/sphinx/blob/v1.5/sphinx/__main__.py

readthedocs/doc_builder/backends/sphinx.py Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Contributor Author

@eric-wieser eric-wieser commented Apr 27, 2020

We need to test this across all sphinx versions (>=1.5).

But looks like it should work https://github.com/sphinx-doc/sphinx/blob/v1.5/sphinx/__main__.py

It was added in 1.3: sphinx-doc/sphinx@74c7a52

stsewd
stsewd approved these changes Apr 27, 2020
Copy link
Member

@stsewd stsewd left a comment

Tested this with a couple of versions, everything looks correct 👍

readthedocs/rtd_tests/tests/test_builds.py Outdated Show resolved Hide resolved
@stsewd stsewd requested a review from Apr 27, 2020
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 27, 2020

Interesting. Anything that changes how we do builds scares me, but this seems like a reasonable change. I wonder if we should put it behind a feature flag to test it, and then roll it out via historical_default=True, so it's easy to rollback if needed?

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 27, 2020

@eric-wieser this is how we add feature flags https://github.com/readthedocs/readthedocs.org/pull/6729/files let me know if you want to give it a try o I can take it from here.

The flag could be called USE_SPHINX_FROM_VENV or FORCE_SPHINX_FROM_VENV or similar

@eric-wieser
Copy link
Contributor Author

@eric-wieser eric-wieser commented Apr 28, 2020

I'd prefer if you could take it from here, thanks.

I think a more suitable name for the flag might be DONT_MIX_PYTHONS_FOR_SPHINX - currently the system python is being used to invoke a script from the venv, which seems like a poor idea. The -m sphinx change here is probably far less relevant than the switch to using the venv python. If you're concerned, you could roll out the change in two pieces.

Whatever you decide, could you enable this new behavior on the cocotb.rtfd.io PR builds (and ping me when you've done so), so we can see if it fixes our issue? Thanks!

@stsewd stsewd requested a review from ericholscher Apr 29, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Looks good. 👍

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@eric-wieser
Copy link
Contributor Author

@eric-wieser eric-wieser commented May 4, 2020

Should there be a test for this? There was one present in 1fa3dda, but it seems it was removed again.

@stsewd
Copy link
Member

@stsewd stsewd commented May 4, 2020

Should there be a test for this? There was one present in 1fa3dda, but it seems it was removed again.

I'll add it back when the feature flag is enabled by default.

@stsewd stsewd merged commit d7e86b3 into readthedocs:master May 4, 2020
2 checks passed
@humitos
Copy link
Member

@humitos humitos commented May 5, 2020

@eric-wieser I enabled this feature flag in your project. Let me know if it does work as you expected. Thanks!

@eric-wieser
Copy link
Contributor Author

@eric-wieser eric-wieser commented May 7, 2020

Definitely doesn't break anything. Not sure if I can easily reproduce the original issue, since it relied on PyPI having a newer release of sphinx than conda, something that only happens immediately after a sphinx release.

@eric-wieser eric-wieser deleted the patch-1 branch May 7, 2020
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.

None yet

4 participants