-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Design doc: new builder with explicit options #8103
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,178 @@ | ||||||||||
Explicit Builders | ||||||||||
================= | ||||||||||
|
||||||||||
.. contents:: | ||||||||||
:local: | ||||||||||
:depth: 2 | ||||||||||
|
||||||||||
Background | ||||||||||
---------- | ||||||||||
|
||||||||||
In the past we have installed some dependencies on each build, | ||||||||||
and tried to guess some options for users in order to make it *easy* for them to start using Read the Docs. | ||||||||||
This has bring some unexpected problems and confusion to users, like: | ||||||||||
|
||||||||||
- The Sphinx version (or from any other tool) isn't the one they expect. | ||||||||||
- Unexpected dependencies are installed. | ||||||||||
- The wrong docs directory is used. | ||||||||||
- Their configuration file is changed on build time overriding the defaults or adding new things. | ||||||||||
- Some files are auto-generated (like ``index.{rst,md}``). | ||||||||||
|
||||||||||
Currently we are aiming to remove this *magic* behaviour from our build process, | ||||||||||
and educating users to be more explicit about their dependencies and options | ||||||||||
in order to make their builds reproducible. | ||||||||||
|
||||||||||
We are using several feature flags to stop doing this for new projects, | ||||||||||
but with so many flags to check our code starts to be hard to follow. | ||||||||||
Instead we would manage a single feature flag and several classes of builds and environments. | ||||||||||
|
||||||||||
Python Environments | ||||||||||
------------------- | ||||||||||
|
||||||||||
Currently we have two Python environments: Virtualenv and Conda, | ||||||||||
they are in charge of installing requirements into an isolated environment. | ||||||||||
We would need to refactor those classes into two types: the new build, and the old build. | ||||||||||
|
||||||||||
The new Python environments would install only the latest versions of the following dependencies: | ||||||||||
|
||||||||||
Virtualenv | ||||||||||
- Sphinx or MkDocs | ||||||||||
- readthedocs-sphinx-ext | ||||||||||
|
||||||||||
Conda | ||||||||||
- readthedocs-sphinx-ext | ||||||||||
|
||||||||||
Note that for Conda we won't install Sphinx or MkDocs, | ||||||||||
this is to avoid problems like `#3829`_ and `#8096`_. | ||||||||||
|
||||||||||
.. _#3829: https://github.com/readthedocs/readthedocs.org/issues/3829 | ||||||||||
.. _#8096: https://github.com/readthedocs/readthedocs.org/issues/8096 | ||||||||||
|
||||||||||
Doc builders | ||||||||||
------------ | ||||||||||
|
||||||||||
Currently we have two types of doc builders: Sphinx and MkDocs. | ||||||||||
They are in charge of generating the HTML files (or PDF/EPUB) from the source files. | ||||||||||
We would need to refactor those classes into two types: the new build, and the old build. | ||||||||||
|
||||||||||
Both builders create an ``index.{rst,md}`` file with some suggestions if one doesn't exist, | ||||||||||
this is to avoid surprises to users, but sometimes this is done on purpose. | ||||||||||
We could change our default 404 page to include some of these suggestions instead. | ||||||||||
Related to `#1800`_. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The build should fail, IMO. We should show the problem in the build detail's page instead in a 404 page. We already known that the build is not in a good state and we should communicate that to the user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, fail the build. |
||||||||||
|
||||||||||
.. _#1800: https://github.com/readthedocs/readthedocs.org/issues/1800 | ||||||||||
|
||||||||||
The new builders would do the minimal (or none) changes to the user's configuration in order to build their docs. | ||||||||||
|
||||||||||
Sphinx | ||||||||||
~~~~~~ | ||||||||||
|
||||||||||
conf.py | ||||||||||
''''''' | ||||||||||
|
||||||||||
We should read the configuration file from the user or default to *one* path, | ||||||||||
and error if it doesn't exist. | ||||||||||
stsewd marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
We shouldn't change or override the values from the user's configuration (``conf.py``), | ||||||||||
some are: | ||||||||||
|
||||||||||
- ``_static`` is added to ``html_static_path`` (probably an old setting) | ||||||||||
- ``html_theme`` (we are only setting this if certain condition is met) | ||||||||||
- ``websupport2_*`` (probably old settings) | ||||||||||
- ``html_context`` (more information below) | ||||||||||
- Latex settings | ||||||||||
(they are used to improve the output for Chinese and Japanese languages, we should write a guide instead) | ||||||||||
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to the sane defaults we talked about. I think this provides value to non-experienced users and we haven't had troubles with it. Also, this is one of the cases where an experienced user can override these configs easily. I'd like to have more like this one instead of less ones 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have mixed feelings here. If we're doing almost nothing in our conf.py override, I do think there's value in trying to get that to nothing at all. If the goal here is giving users options here and letting them configure it with guides, I think that is a reasonable solution for most cases, but we need to figure out where the line is. I think getting to the point where we don't override the users conf.py at all, if possible, would be a huge win for simplifying our builds. I don't know if it's realistic though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After talking to @agjohnson I changed my mind here. I prefer to do nothing in the config file and write a good guide with a copy&paste-ready code explaining this.
Agree. This is where I'd like to end up as well after the new builder. |
||||||||||
|
||||||||||
Sphinx's ``html_context`` | ||||||||||
''''''''''''''''''''''''' | ||||||||||
|
||||||||||
We should pass the minimal needed information into the context. | ||||||||||
This is related to :doc:`/development/design/theme-context`. | ||||||||||
Comment on lines
+89
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like a good moment to implement the new theme context together with the new builder 👍 --whatever its structure would be, but having it under a namespace ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- though we should also heavily minimize the data to start, treating it basically as an API 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that this is not decided yet. The document proposes to remove (if possible) passing Considering that we are trying to avoid this magic, I'd propose to do a little research to understand if it's possible to not touch this variable from our side and remove it completely since it's a Sphinx-only feature. Also, because we are trying to remove ourselves from the build process. (Note: I'm considering using an intermediate tool-agnostic |
||||||||||
|
||||||||||
With :doc:`/api/v3` and environment variables (to store the secret token) | ||||||||||
should be possible to query several things from the project without having to pass them at build time into the context. | ||||||||||
Most the of basic information can be obtained from our environment variables (``READTHEDOCS_*``). | ||||||||||
|
||||||||||
Some values from the context are used in our Sphinx extension, | ||||||||||
we should define them as configuration options instead. | ||||||||||
Others are used in our theme, should we stop setting them? | ||||||||||
|
||||||||||
Extension | ||||||||||
''''''''' | ||||||||||
|
||||||||||
There are some things our extension does that are already supported by Sphinx or themes | ||||||||||
(analytics, canonical URL, etc), we should write a guide instead of applying our magic. | ||||||||||
Comment on lines
+103
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are the kind of the features that "bump your documentation to the next quality level" just because you are using Read the Docs. You don't have to worry about dealing with Google Analytics code, canonical URLs, sitemap, robots, etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Analytics is already an opt-in option, so I'd be ok with moving the canonical url to an option too (we hit problems with injecting the canonical url twice). I'm +1 on giving users more control over this kind of settings instead of removing them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine moving them to options and I'd like to see them built outside the extension as a post-processing HTML feature instead. |
||||||||||
|
||||||||||
The extension also injects some custom js and css files. | ||||||||||
We could try another more general approach and inject these on each HTML file after the build is complete. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This approach makes sense if we are going to support other building tools in the future. This will probably have some impact on big projects with many HTML files, since we will need to rewrite all of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I think adjusting the built HTML is the right way to handle this, so we can support non-Sphinx builders. In mkdocs we're just adding this to their setting: readthedocs.org/readthedocs/doc_builder/backends/mkdocs.py Lines 175 to 178 in 924699c
But it seems like explicitly doing it after build against all HTML is more extensible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breaking it out from the extension into the build process is probably correct here. 👍 |
||||||||||
|
||||||||||
The extension injects the warning banner if you are reading the docs from a pull request. | ||||||||||
We could implement this like the version warning banner instead, so it works for MkDocs too. | ||||||||||
(this is injected with the version selector). | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense to me 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if it makes sense to migrate the version warning banner to the new builder. We have had a lot of different opinions about how it should work and we ended up creating a Sphinx extension that allow users to customize different aspects of it. I'd say that it would be better to remove it completely and either:
I think 1) is better if we want to go in the "provide integration points where the community can hook into". Besides, it's less work for us 😄 |
||||||||||
|
||||||||||
MkDocs | ||||||||||
~~~~~~ | ||||||||||
|
||||||||||
We should read the configuration file from the user or default to *one* path, | ||||||||||
and error if it doesn't exist. | ||||||||||
|
||||||||||
We shouldn't change the values from the user's configuration (``mkdocs.yml``), | ||||||||||
currently we change: | ||||||||||
|
||||||||||
- ``docs_dir`` | ||||||||||
- ``extra_javascript`` | ||||||||||
- ``extra_css`` | ||||||||||
- ``google_analytics`` (we change this to ``None`` and use our own method) | ||||||||||
- ``theme`` (we set it to our theme for old projects) | ||||||||||
|
||||||||||
Only the additional js/css files should be added. | ||||||||||
Additionally, we could try another more general approach and inject these after the build is complete. | ||||||||||
|
||||||||||
Related to `#7844`_, `#4924`_, `#4827`_, `#4820`_ | ||||||||||
|
||||||||||
.. _#7844: https://github.com/readthedocs/readthedocs.org/issues/7844 | ||||||||||
.. _#4924: https://github.com/readthedocs/readthedocs.org/issues/4924 | ||||||||||
.. _#4827: https://github.com/readthedocs/readthedocs.org/issues/4827 | ||||||||||
.. _#4820: https://github.com/readthedocs/readthedocs.org/issues/4820 | ||||||||||
|
||||||||||
Web settings | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we get rid of web settings entirely for things that are in the config file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think they are still useful when you want to build from an existing release, since you can't change that code to add a config file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should remove them and provide a nice way to support version aliases. So, in case you have to re-build an old tag (e.g. |
||||||||||
------------ | ||||||||||
|
||||||||||
Simple defaults, without fallbacks. | ||||||||||
|
||||||||||
Currently if some of our settings aren't set or are incorrect | ||||||||||
we try to guess some values for the user. | ||||||||||
We should have some sane defaults and error otherwise. | ||||||||||
Some are: | ||||||||||
|
||||||||||
- Requirements file (we shouldn't install any if isn't set) | ||||||||||
- Sphinx/MkDocs configuration file (we could default to ``docs/conf.py`` and ``mkdocs.yml``) | ||||||||||
|
||||||||||
.. note:: | ||||||||||
|
||||||||||
When using the v2 of the config file we remove all this magic. | ||||||||||
|
||||||||||
Other settings are used for things that can be done from the user side: | ||||||||||
|
||||||||||
- Analytics code | ||||||||||
- Canonical domain (Sphinx only) | ||||||||||
|
||||||||||
Adoption | ||||||||||
-------- | ||||||||||
|
||||||||||
If we remove some magical behaviour that was doing things for the user, | ||||||||||
we should document how to do them using Sphinx/MkDocs. | ||||||||||
|
||||||||||
These new builders/environments would be under a feature flag, | ||||||||||
where projects created after ``x`` date would use the new builders and environments, | ||||||||||
and past projects would use the old ones. | ||||||||||
|
||||||||||
Deprecation | ||||||||||
----------- | ||||||||||
|
||||||||||
Using a feature flag can bring some confusion to users that have a project created before the given date, | ||||||||||
and other after that date. We can opt-in users into the new builders by adding them into the feature flag. | ||||||||||
|
||||||||||
In order to simplify our code and have all projects using the same options and dependencies | ||||||||||
we want to fully migrate all projects to use the new builders. | ||||||||||
We could put a date to do this, and contact all users of old projects about this change | ||||||||||
(an entry in our blog would also be nice). | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section could probably use a bit more detail. Migrating all our users is going to be a lot of work. There's a number of things we could do to help them migrate though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't sure how to expand this, I have updated it to put a list of things that could be done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of installing Sphinx/MkDocs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So users don't need to have a requirements file for this, and installing them with pip doesn't have the same problems as Conda, they can be easily overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts in some way with the confusions listed in this document:
The new builder requires a
conf.py
. So, they will need to have Sphinx installed locally and they have ransphinx-quickstart
. Then, when importing on Read the Docs, we are going to install a random version (potentially different than the one they have installed locally). In the end, we are not fixing this problem / removing this confusion / making this confusion clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another things that rings me a bell here are:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they don't have a requirements file, they aren't expecting any version at all. If they have
sphinx==1.2.3
orsphinx
(latest) and we install something different or isn't downgraded than that's a problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we should install only dependencies that are required at build time to make Read the Docs' features to work:
readthedocs-sphinx-ext
. All the other extra dependencies should be defined by the user. We can start with 0 extra dependencies, and add them later if we find they are required, but at this point, I wouldn't include them here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I assume
readthedocs-sphinx-ext
depends on Sphinx, but we can probably install it without.I think in this builder we should not install sphinx or mkdocs, and require the user to understand what is happening. Hopefully they can eventually specify it easily in the rtd.yaml (
install: sphinx
) or similar, but I agree that having less magic to manage is good.