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

Improve installation guide #3631

Merged
merged 17 commits into from
Oct 31, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 18, 2018

This is my second attend to improve the installation guide (first one #3614), I have take some notes from #3614 (comment), #3614 (comment), #3488 (comment).

I try to reduce the trying to teach how to use for provide useful links and be a little more lineal on the steps. And remove lots of notes that are really necessary, but give the impression of be optional.

Please let me know any issues with the English :)

Closes #2246

@@ -3,121 +3,120 @@
Installation
============

Here is a step by step plan on how to install Read the Docs.
Here is a step by step guide on how to install Read the Docs.
Copy link
Member Author

@stsewd stsewd Feb 18, 2018

Choose a reason for hiding this comment

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

I'm not completely sure about this change, please let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Better, for me.

docs/install.rst Outdated
Read the Docs does not itself run under Python 3 (though it does support
building documentation for Python 3 projects). Please ensure the subsequent
steps are performed using Python 2.7.
Read the Docs does not full support running under Python 3
Copy link
Member Author

Choose a reason for hiding this comment

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

The rtd code can run on python3, but I think we are not confident enough (#3570)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't advertise any level of Python 3 support until we use it on production, have a running test suite, and bandwidth to response user reports. I think it would be better to keep the old wording or say something like "Python 3 support is planned, PRs welcome."

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the previous wording was better.

RTD should fully support Python3. There are some core devs that develop using Py3 and it's fine. Although, I'd say that it's not well/fully tested as it should and production runs Py2 yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I don't think there is any reason to scare users into not using py3. We'll be using it in prod as soon as we migrate. I'm not sure I even want to mention that RTD isn't py3 in prod, as I'd rather we get PRs now for py3 incompatibilities.

docs/install.rst Outdated
CFLAGS=-I/usr/local/opt/libxml2/include/libxml2 \
LDFLAGS=-L/usr/local/opt/libxml2/lib \
pip install -r requirements.txt
* On Ubuntu you can install it using ``sudo apt-get install elasticsearch``
Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure if this lines are necessary, since is only for Ubuntu.

docs/install.rst Outdated
.. note::
CFLAGS=-I/usr/local/opt/libxml2/include/libxml2 \
LDFLAGS=-L/usr/local/opt/libxml2/lib \
pip install -r requirements.txt
Copy link
Member Author

Choose a reason for hiding this comment

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

These is the only non-linear step, but no big deal I think

docs/install.rst Outdated
.. _Python 2.7: http://www.python.org/
.. _virtualenv: https://virtualenv.pypa.io/en/stable/
.. _Git: http://git-scm.com/
.. _`Mercurial`: https://www.mercurial-scm.org/
Copy link
Member Author

@stsewd stsewd Feb 18, 2018

Choose a reason for hiding this comment

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

Ups, a little of inconsistency here


pip --version
.. _Python 2.7: http://www.python.org/
.. _virtualenv: https://virtualenv.pypa.io/en/stable/
Copy link
Member Author

@stsewd stsewd Feb 18, 2018

Choose a reason for hiding this comment

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

Referring to the docs here, instead of pypi page of virtualenv

docs/install.rst Outdated

pip install --upgrade pip
Cloning the repository and start Django
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this title, please help

Copy link
Member

Choose a reason for hiding this comment

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

"Get and Run Read the Docs" ?

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 like more that!

mkdir checkouts
cd checkouts
git clone https://github.com/rtfd/readthedocs.org.git
virtualenv venv
Copy link
Member Author

Choose a reason for hiding this comment

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

Following the pythonic convention here, virtual env called venv and on the project root (now we have this on the .gitignore :) #3609).


Next, install the dependencies using ``pip`` (included inside of virtualenv_)::
Next, install the dependencies using ``pip``
(make sure you are inside of the virtual environment)::
Copy link
Member Author

Choose a reason for hiding this comment

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

little warning for common error


python manage.py migrate

Then please create a superuser account for Django::
Then create a superuser account for Django::
Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I do not like it, but I had the feeling that it was optional.

docs/install.rst Outdated

python manage.py createsuperuser

Now let's properly generate the static assets::

python manage.py collectstatic

By now, it is the right time to load in a couple users and a test project::
Now you can optionally load a couple users and a test projects::
Copy link
Member Author

Choose a reason for hiding this comment

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

Now, this is optional!

Copy link
Member Author

Choose a reason for hiding this comment

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

And believe the a on a test projects isn't necessary.

docs/install.rst Outdated
with the name of any added project::
While the webserver is running,
you can build the documentation for the latest version of any project using the ``update_repos`` command.
for example to update the ``pip`` repo::
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 forgot to include on the PR, but this closes #2246

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems that this command doesn't work #3696

Copy link
Member Author

Choose a reason for hiding this comment

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

After #3696 (comment), is really necessary show this command here? Probably is better to have a section explaining the full set of management commands for development.

docs/install.rst Outdated

python manage.py update_repos pip

Further steps
Copy link
Member Author

Choose a reason for hiding this comment

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

With this steps you can start the server and do a build (sometimes is required to install some more dependencies for generating the pdf), but I feel that these others docs are connected on some way

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that this section should go after "What's available".

Copy link
Member

Choose a reason for hiding this comment

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

Also, in the section "What's available" you could include a note mentioning that pdf generation is not covered here or to do that it's required to use the docker image.

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.

Nice! I left some comments to consider.

docs/install.rst Outdated
Read the Docs does not itself run under Python 3 (though it does support
building documentation for Python 3 projects). Please ensure the subsequent
steps are performed using Python 2.7.
Read the Docs does not full support running under Python 3
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the previous wording was better.

RTD should fully support Python3. There are some core devs that develop using Py3 and it's fine. Although, I'd say that it's not well/fully tested as it should and production runs Py2 yet.

.. _Git: http://git-scm.com/
.. _Homebrew: http://brew.sh/
.. _Elasticsearch: https://www.elastic.co/products/elasticsearch
.. _PostgreSQL: https://www.postgresql.org/
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove py2.7, and postgresql here? RTD depends on them, right?

Or, at least Py2.7 since SQLite is used instead of PostgreSQL.

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 didn't, there are just moved down :) the diff doesn't help too much p:

But, I think I forgot postgres

Copy link
Member Author

Choose a reason for hiding this comment

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

@humitos redis is mandatory for the installation?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have some code that checks redis queue length directly, but i'd like to eventually remove that check completely.

docs/install.rst Outdated

Users of other Linux distributions may need to install the equivalent
packages, depending on their system configuration.
If you are having trouble on OS X Mavericks
Copy link
Member

Choose a reason for hiding this comment

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

Mmm... I think the order of the steps are confusing.

Here we are talkinga about the problems you may be having but we didn't run any command yet. These errors could appear while running pip install -r.

So, I like the idea of having these OS heads here, but maybe this one in particular could still be a note after the pip install command. That way, if you are a Mac user you skip all the other OS section, run that command and in case that it fail you read the note :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's better, perhaps I should put that this steps are only for linux for now


Requires Git version >=1.9
* `Elasticsearch`_ (only if you want full support for searching inside the site)
Copy link
Member

Choose a reason for hiding this comment

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

Elasticsearch needs some more configuration and I wasn't able to configure. I'd say that we should remove this as "depends on" and leave it as a note or remove it completely.

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 think this guide is also used for custom installations, right?

docs/install.rst Outdated

.. note::
* On Ubuntu you can install it using ``sudo apt-get install redis-server``
Copy link
Member

Choose a reason for hiding this comment

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

Why not moving this to the "Ubuntu" section where other packages are installed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you are right, that's a better place

.. _Mercurial: https://www.mercurial-scm.org/
.. _Pip: https://pip.pypa.io/en/stable/
.. _Homebrew: http://brew.sh/
.. _Elasticsearch: https://www.elastic.co/products/elasticsearch
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Probably removing this is better to avoid confusion.

docs/install.rst Outdated

python manage.py update_repos pip

Further steps
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that this section should go after "What's available".

docs/install.rst Outdated

python manage.py update_repos pip

Further steps
Copy link
Member

Choose a reason for hiding this comment

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

Also, in the section "What's available" you could include a note mentioning that pdf generation is not covered here or to do that it's required to use the docker image.

docs/install.rst Outdated

pip install --upgrade pip
Cloning the repository and start Django
Copy link
Member

Choose a reason for hiding this comment

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

"Get and Run Read the Docs" ?

docs/install.rst Outdated

By now you can trigger builds on your local environment,
to encapsulete the build process inside a Docker container,
see :doc:`development/buildenvironments`.
Copy link
Member

Choose a reason for hiding this comment

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

Also, I think that this page that is linked here should contain a link to another section (a Guide probably) that explains how is the process of creating your own docker image for your user and avoid permission issues.

I mean, a Guide outside the official docs and under /guides/. That's for another PR, though


If you have problems building successfully a project,
probably is because some missing libraries for ``pdf`` and ``epub`` generation.
You can uncheck this on the advanced settings of your project.
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 added a note about common problems (see #2271 (comment))

docs/install.rst Outdated
What's available
----------------

After registering with the site (or creating yourself a superuser account),
you will be able to log in and view the `dashboard <http://localhost:8000/dashboard/>`_.

.. note::

You can get the verification link from the console where you ran the server.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also a common question

@stsewd
Copy link
Member Author

stsewd commented Feb 21, 2018

@humitos I still have some doubts about removing completely elasticsearch, I think with the current note (is only needed for search) is fine, I mean, people will think the searching no working is a bug.

.. note::

You can get the verification link from the console where you ran the server.

Importing your docs
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 think we can remove this subsection and only have one section

@ericholscher
Copy link
Member

@stsewd want to resolve the merge conflicts and I can review & commit it. Trying to reduce the number of open PR's we have :)

@stsewd
Copy link
Member Author

stsewd commented Oct 31, 2018

Got it, working on that

@stsewd stsewd force-pushed the improve-installation-guide branch from 140e09a to c505a5d Compare October 31, 2018 15:13
@stsewd
Copy link
Member Author

stsewd commented Oct 31, 2018

@ericholscher ok, this is ready for re-review.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks great. 💯

docs/install.rst Outdated

sudo yum install python-devel python-pip libxml2-devel libxslt-devel
On Ubuntu
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could use https://github.com/djungelorm/sphinx-tabs to make this a nicer UI for our users here, instead of having to scroll through all the data for platforms I don't use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that will look better

@codecov
Copy link

codecov bot commented Oct 31, 2018

Codecov Report

Merging #3631 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3631      +/-   ##
==========================================
+ Coverage   76.21%   76.27%   +0.05%     
==========================================
  Files         158      158              
  Lines       10019     9981      -38     
  Branches     1265     1261       -4     
==========================================
- Hits         7636     7613      -23     
+ Misses       2039     2024      -15     
  Partials      344      344
Impacted Files Coverage Δ
readthedocs/builds/models.py 75.6% <0%> (-0.38%) ⬇️
readthedocs/builds/admin.py 95.83% <0%> (-0.17%) ⬇️
readthedocs/projects/models.py 84% <0%> (-0.07%) ⬇️
readthedocs/profiles/views.py 86.44% <0%> (ø) ⬆️
readthedocs/projects/views/private.py 80.1% <0%> (+1.85%) ⬆️
readthedocs/builds/forms.py 95.83% <0%> (+7.95%) ⬆️

@stsewd
Copy link
Member Author

stsewd commented Oct 31, 2018

@ericholscher done, it looks like this

screenshot_2018-10-31 installation read the docs 2 7 documentation

@ericholscher
Copy link
Member

done, it looks like this

💯

@ericholscher ericholscher merged commit 0f5d979 into readthedocs:master Oct 31, 2018
@stsewd stsewd deleted the improve-installation-guide branch October 31, 2018 16:58
@humitos
Copy link
Member

humitos commented Oct 31, 2018

I love those tabs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants