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

Split requirements/pip.txt #5100

Merged
merged 6 commits into from Jan 15, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jan 13, 2019

Fixes #3757

Copy link
Member

@humitos humitos left a comment

Good approach! I think we are close to have it ready for merging.

I'd like to be more aggressive on the split and have it properly separated.

Example: If I downloaded the git repo and want to build the Read the Docs documentation, I should only do pip install requirements/docs.txt and then make html should work and produce the desired docs _without installing anything no-needed` (django or whatever).

This will probably ends up on having repeated packages on the requirements, but that's fine to me because we want to be able to fine tune it: "use Sphinx 2.0 for our docs, but not for building user's docs"

requirements/deploy.txt Outdated Show resolved Hide resolved
requirements/lint.txt Outdated Show resolved Hide resolved
requirements/onebox.txt Outdated Show resolved Hide resolved

# Docs
sphinxcontrib-httpdomain==1.7.0
sphinx-prompt==1.0.0
Copy link
Member

@humitos humitos Jan 13, 2019

I suppose that these two packages are for building our own docs and not for building user's documentation. So, these should probably go into a different reqs file maybe.

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 13, 2019

Wasn't aware of that these are only for RTD docs.

these should probably go into a different reqs file maybe.

Since these are RTD-only requirements, we can put it back in the pip.txt

Copy link
Member

@humitos humitos Jan 13, 2019

Well, they are not required to "run Read the Read software" but to build the docs of it.

So, basically, we should use this requirements/docs.txt file in our own projects under readthedocs.io (https://readthedocs.org/projects/docs/)

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 14, 2019

Okay.
I have separated them in docs.txt and also add -r local-docs-build.txt to install the other dependencies.

@@ -14,3 +14,4 @@ pylint-celery==0.3
prospector==1.1.6.2
# prospector 1.1.6.2 is not compatible with 2.0.0
pyflakes<2.0.0
Pygments==2.3.1
Copy link
Member Author

@dojutsu-user dojutsu-user Jan 13, 2019

I put this here because it was required to make the lint tests.
This gets repeated here as it was already moved to the local_docs_build.txt

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 13, 2019

@humitos

Example: If I downloaded the git repo and want to build the Read the Docs documentation, I should only do pip install requirements/docs.txt and then make html should work and produce the desired docs _without installing anything no-needed` (django or whatever).

I tried to do exactly the same thing. But in the conf.py, but we are importing django and other stuff, so make html doesn't work with the virtual environment created from the local_docs_build.txt

@humitos
Copy link
Member

@humitos humitos commented Jan 14, 2019

But in the conf.py, but we are importing django

Do you know why we are doing that? Is it to document our django settings?

On the other hand, in case that it's strictly needed, does the docs build just installing django without installing the whole RTD dependencies?

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 14, 2019

@humitos

Do you know why we are doing that? Is it to document our django settings?

These lines.

https://github.com/rtfd/readthedocs.org/blob/b07f4550345b90e07fd2504b984c3361a05a97c6/docs/conf.py#L15-L19

  • I didn't find any use for settings.
  • But timezone is being used down in the file.
  • And django.setup()

On the other hand, in case that it's strictly needed, does the docs build just installing django without installing the whole RTD dependencies?

I tried this.

  • Installed django (raised error for future)
  • Install future (raised error for celery)

I think that RTD docs needs the RTD-specific requirements (pip.txt)

@humitos
Copy link
Member

@humitos humitos commented Jan 15, 2019

We are using the extension djangodocs which allow us to use :djangosetting: in our .rst files.

I think that RTD docs needs the RTD-specific requirements (pip.txt)

OK, we can open another issue to research about this later and try to have the minimal dependencies for our docs.

Copy link
Member

@humitos humitos left a comment

Please, revert the creation of docs.txt, open a new issue to track that path and we will be ready to merge this PR from my point of view.

@humitos
Copy link
Member

@humitos humitos commented Jan 15, 2019

Install future (raised error for celery)

This is because we are importing from celery.schedules import crontab in our settings. I'm almost sure that we can split this to a docs.txt and install only the needed dependencies.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 15, 2019

@humitos
I have moved Docs requirements back to local_docs_build.txt

This is because we are importing from celery.schedules import crontab in our settings. I'm almost sure that we can split this to a docs.txt and install only the needed dependencies.

It would be great. Will open a new issue to track this.

Copy link
Member

@ericholscher ericholscher left a comment

Won't this cause our docs build to fail, unless we update our .readthedocs.yml to point at a new config file: https://github.com/rtfd/readthedocs.org/blob/master/.readthedocs.yml#L6

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 15, 2019

@ericholscher
I forget to do this.
I have updated tha yaml file.

@ericholscher ericholscher merged commit ac82058 into readthedocs:master Jan 15, 2019
2 checks passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 15, 2019

This will go out with next deploy, so we have time to debug 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants