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

Update docs requirements list #74956

Merged
merged 7 commits into from
Jun 22, 2021
Merged

Update docs requirements list #74956

merged 7 commits into from
Jun 22, 2021

Conversation

acozine
Copy link
Contributor

@acozine acozine commented Jun 9, 2021

SUMMARY

In the Docs Working Group meeting on 25 May 2021 we agreed to maintain two requirements files for the docs build:

  • one with as few version pins as possible
  • a second one pinned to known good versions for use with the publication pipeline

The plan is to "unleash" the known good versions, test the most recent available packages, and update the. known_good_reqs. file on a quarterly basis going forward, so the versions don't get too stale.

This PR:

  • creates the second requirements file
  • removes the pin on the Sphinx version

Related to #74318
Closes #74194
Closes #71395

Tested on a mac running macOS Catalina. Pip package versions listed below. Test if you have time and report any issues you find!

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

docs.ansible.com

ADDITIONAL INFORMATION

Versions installed from the main (unpinned) requirements.txt:

Package                       Version
----------------------------- -----------
aiofiles                      0.7.0
aiohttp                       3.7.4.post0
alabaster                     0.7.12
antsibull                     0.33.0
antsibull-changelog           0.10.0
async-timeout                 3.0.1
asyncio-pool                  0.5.2
attrs                         21.2.0
Babel                         2.9.1
certifi                       2021.5.30
cffi                          1.14.5
chardet                       4.0.0
click                         8.0.1
cryptography                  3.4.7
docutils                      0.16
idna                          2.10
imagesize                     1.2.0
Jinja2                        3.0.1
MarkupSafe                    2.0.1
multidict                     5.1.0
packaging                     20.9
perky                         0.5.5
pip                           21.0.1
pycparser                     2.20
pydantic                      1.8.2
Pygments                      2.9.0
pyparsing                     2.4.7
pytz                          2021.1
PyYAML                        5.4.1
requests                      2.25.1
resolvelib                    0.5.4
rstcheck                      3.3.1
semantic-version              2.8.5
setuptools                    54.2.0
sh                            1.14.2
six                           1.16.0
snowballstemmer               2.1.0
Sphinx                        4.0.2
sphinx-ansible-theme          0.6.0
sphinx-intl                   2.0.1
sphinx-notfound-page          0.7.1
sphinx-rtd-theme              0.5.2
sphinxcontrib-applehelp       1.0.2
sphinxcontrib-devhelp         1.0.2
sphinxcontrib-htmlhelp        2.0.0
sphinxcontrib-jsmath          1.0.1
sphinxcontrib-qthelp          1.0.3
sphinxcontrib-serializinghtml 1.1.5
straight.plugin               1.5.0
Twiggy                        0.5.1
typing-extensions             3.10.0.0
urllib3                       1.26.5
yarl                          1.6.3

@ansibot ansibot added affects_2.12 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docs_only All changes are to files within the docs/docsite/ directory needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 9, 2021
@acozine acozine changed the title Update docs requirements list WIP: Update docs requirements list Jun 9, 2021
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 9, 2021
sphinx-notfound-page >= 0.6
sphinx-intl
sphinx_ansible_theme === 0.6.0
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 like to have an exact pin here. == matches .post releases while === matches the exact spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks @webknjaz, that is not syntax I knew before. I'll restore that.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in a4fb538

@ansibot ansibot added the docsite This issue/PR relates to the documentation website. label Jun 10, 2021
@samccann
Copy link
Contributor

Tested locally (Fedora 34) in new venv - all permutations work:

  • requirements.txt - works for make webdocs and make coredocs
  • known)_good_reqs.txt - works for make webdocs and make coredocs

@@ -74,8 +74,7 @@ Setting up your environment to build documentation locally

To build documentation locally, ensure you have a working :ref:`development environment <environment_setup>`.

To work with documentation on your local machine, you need to have python-3.5 or greater and the
following packages installed:
To work with documentation on your local machine, you need to have python-3.5 or greater and the following packages installed:
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a comment in an older PR that suggests we just remove this list and say something like see xx requirements.txt for the packages and versions required (maybe even link to that file in github.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I've updated based on this comment

@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jun 10, 2021
@webknjaz
Copy link
Member

Tested locally (Fedora 34) in new venv - all permutations work:

  • requirements.txt - works for make webdocs and make coredocs
  • known)_good_reqs.txt - works for make webdocs and make coredocs

I've mentioned this in Slack but let me document it here. It's easier and more reliable to test such a setup with pip-tools.

@ansibot
Copy link
Contributor

ansibot commented Jun 14, 2021

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/community/documentation_contributions.rst:77:0: warning: Unknown target name: "documentation dependencies<https://github.com/ansible/ansible/blob/devel/docs/docsite/requirements.txt>".

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/community/documentation_contributions.rst:77:0: Unknown target name: "documentation dependencies<https://github.com/ansible/ansible/blob/devel/docs/docsite/requirements.txt>".

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jun 14, 2021
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jun 14, 2021
@acozine
Copy link
Contributor Author

acozine commented Jun 14, 2021

@webknjaz I'm sure pip-tools is an excellent option, but adding another tool seems like an over-engineered solution for the problem we're addressing

@acozine acozine changed the title WIP: Update docs requirements list Update docs requirements list Jun 18, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. has_issue and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jun 18, 2021
@webknjaz
Copy link
Member

@acozine it's not really overengineering, its a pretty standard practice of keeping the builds reproducible. Currently, this PR almost solves a part of the problem not taking into account transitive deps that may break things while my proposal is to actually address the root of the issue. But I agree that the current patch is better than nothing, of course. And automatic pinning could be implemented separately.

sphinx==4.0.2
sphinx-notfound-page==0.7.1 # must be >= 0.6
sphinx-intl==2.0.1
sphinx_ansible_theme===0.6.0
Copy link
Member

Choose a reason for hiding this comment

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

Taking into account #75059, it should now be

Suggested change
sphinx_ansible_theme===0.6.0
sphinx_ansible_theme===0.7.0

PyYAML
rstcheck
sphinx==2.1.2
sphinx
sphinx-notfound-page >= 0.6
sphinx-intl
sphinx_ansible_theme === 0.6.0
Copy link
Member

Choose a reason for hiding this comment

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

With #75059 it's now

Suggested change
sphinx_ansible_theme === 0.6.0
sphinx_ansible_theme >= 0.7.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I would really avoid >= here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay for direct unpinned deps. Anybody wanting the pins should do pip install -r docs/docsite/requirements.txt -c docs/docsite/known_good_reqs.txt

jinja2
Pygments >= 2.4.0
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary among the direct deps anymore because the theme pulls in this dep and we no longer use it directly.

Suggested change
Pygments >= 2.4.0

@acozine
Copy link
Contributor Author

acozine commented Jun 22, 2021

The goal we agreed on in the DaWGs meeting is to offer two options: one set of requirements with the least possible restrictions on versions, and a second set of requirements with a known-good set that updates infrequently. I think the current PR meets those requirements. I'm fine with the known-good list getting stale, as long as it continues to work.

@acozine
Copy link
Contributor Author

acozine commented Jun 22, 2021

We can update again in future if we all agree on a goal and an implementation strategy.

@acozine acozine merged commit 58f2638 into ansible:devel Jun 22, 2021
@acozine acozine deleted the update_docs_reqs branch June 22, 2021 18:58
felixfontein pushed a commit to felixfontein/ansible that referenced this pull request Jun 22, 2021
* removes upper bound on sphinx version
* updates versions of docs build dependencies, adds known good requirements file
* adds instructions for using known_good_reqs file

(cherry picked from commit 58f2638)
Comment on lines +91 to +92
pip install --user -r requirements.txt
pip install --user -r docs/docsite/known_good_reqs.txt
Copy link
Member

Choose a reason for hiding this comment

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

@acozine @samccann I missed this but it should be something like pip install -r docs/docsite/requirements.txt -c docs/docsite/known_good_reqs.txt. Which basically constraints what should be installed.

@webknjaz
Copy link
Member

The goal we agreed on in the DaWGs meeting is to offer two options: one set of requirements with the least possible restrictions on versions, and a second set of requirements with a known-good set that updates infrequently. I think the current PR meets those requirements. I'm fine with the known-good list getting stale, as long as it continues to work.

The point is that it won't continue to work if some indirect dependency changes in an incompatible way. This will be a silent change with no instrumentation to notice it automatically until a human complains. This is because the current pins list is incomplete.

acozine added a commit that referenced this pull request Jun 23, 2021
* Use `sphinx_ansible_theme` Sphinx theme in docs (#74318)

(cherry picked from commit 346c7a7)

* 🔥 Drop unused `core.css` file

This is a forgotten leftover from #74318 that should've been removed
earlier.

(cherry picked from commit ec408a6)

* Update docs requirements list (#74956)

* removes upper bound on sphinx version
* updates versions of docs build dependencies, adds known good requirements file
* adds instructions for using known_good_reqs file

(cherry picked from commit 58f2638)

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
@ansible ansible locked and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.12 core_review In order to be merged, this PR must follow the core review workflow. docs_only All changes are to files within the docs/docsite/ directory docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. has_issue support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Sphinx and sphinx_rtd_theme doc build fails on Python 3.8 + MacOS
6 participants