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

Document python.use_system_site_packages option #4422

Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 24, 2018

Closes #4418

@stsewd stsewd requested a review from a team July 24, 2018 18:08
@agjohnson agjohnson added this to the YAML File Completion milestone Aug 2, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

LGTM

* Type: Boolean

When true, it gives the virtual environment access to the global site-packages directory.
Depending of the :ref:`yaml-config:build.image`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! Missed typo here. Depending of -> Depending on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to take a look at some english grammar rules p:

Copy link
Member

Choose a reason for hiding this comment

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

Please, fix this and we can merge, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups, forgot to resolve this

@stsewd stsewd requested a review from agjohnson August 10, 2018 16:14
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Changes look good to me.


When true, it gives the virtual environment access to the global site-packages directory.
Depending on the :ref:`yaml-config:build.image`,
Read the Docs includes some libraries like scipy, numpy, etc.
Copy link
Member

Choose a reason for hiding this comment

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

Same as #4419

If we don't want / can't document exactly what are those libraries, I think we should at least link to the repository that contains the Dockerfile and tell this to the user. I mean, how they can do to know exactly what are the pre-installed libs of the docker image they are using.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe is worth having a build-images docs? (or creating docs in the docker images repo)

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of having a page (inside the current docs) for the docker images that we use.

This page could explain what's the default image, explain or release process, what's installed on each of them, what's its base OS and how to check what specific version is installed on it.

Among other things 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I just discovered that we already have some docs about the packages installed, we should just improve/update them https://docs.readthedocs.io/en/latest/builds.html#packages-installed-in-the-build-environment

Copy link
Member

Choose a reason for hiding this comment

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

Good! So, in that case, I'd say that we should link that documentation page from here. Seems to be the most accurate to do and easy to follow.

Copy link
Member

Choose a reason for hiding this comment

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

We may need a different section for Python packages in those docs, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Already did that haha, I thought you already saw the PR #4515

@humitos
Copy link
Member

humitos commented Aug 15, 2018

I'd say that this is ready to merge once we add the link to the build images.

@stsewd stsewd force-pushed the doc-python-use-system-site-packages branch from 5dd8716 to fc6afd5 Compare August 22, 2018 16:15
@stsewd
Copy link
Member Author

stsewd commented Aug 22, 2018

Rebased and updated

@ericholscher
Copy link
Member

👍

@ericholscher ericholscher merged commit 156e76e into readthedocs:master Aug 23, 2018
@stsewd stsewd deleted the doc-python-use-system-site-packages branch August 23, 2018 14:44
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.

4 participants