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 requirements.txt to Ubuntu 22.04 #465

Conversation

AlexeyMerzlyakov
Copy link
Collaborator

@AlexeyMerzlyakov AlexeyMerzlyakov commented Sep 13, 2023

Proposed solution

This is attempt to fix the #364 , which requirements.txt is out of date for Ubuntu 22.04.
The following pip3 requirements were updated (empty fields in columns 2 and 3 mean that package version is unspecified):

Package Vanilla New Changed Rationale
breathe 4.28.0 4.35.0 Y breathe 4.28.0 is incompatible with Sphinx above v3.6, so updating the version and fix it to the currently working 4.35.0
docutils 0.14 0.15 Y myst-parser 1.0.0 requires 0.15<=docutils<0.20. The docutils version can not be relaxed, as sphinx 5.3.0, myst-parser 1.0.0 and sphinx-rtd-theme have restrictions from above on the upper version of it, so switching it to the next sub-version.
jinja2 3.0.3 3.0.3 N
myst-parser 1.0.0 1.0.0 N Available versions of myst-parser are 1.0.0 and 2.0.0, but I'd suppose for the compatibility purposes is better to not update this verision to 2.0.0
sphinx_copybutton 0.5.2 0.5.2 N
sphinx_rtd_theme N
sphinx-autobuild N
sphinx 3.5.0 5.3.0 Y Fix for sphinx-doc/sphinx#9562 requires Sphinx at least v4.2.0. However, myst-parser 1.0.0 requires 5<=sphinx<7 and latest breathe (4.35.0) is not compatible with Sphinx==5.0.0, so choosing latest Sphinx of version 5 line, which is 5.3.0
sphinxcontrib-plantuml N

Verification results

Tested on local Ubuntu 22.04 machine and ubuntu:focal Docker container locally repeating CircleCI steps and configuration.

Pip3 install and make html logs on Docker container with vanilla requirements.txt are attached below:
pip3_install_vanilla.log
make_html_vanilla.log

There is the following issue appear on Ubuntu 20.04:

ERROR: myst-parser 1.0.0 has requirement docutils<0.20,>=0.15, but you'll have docutils 0.14 which is incompatible.
ERROR: myst-parser 1.0.0 has requirement sphinx<7,>=5, but you'll have sphinx 3.5.0 which is incompatible.

The same error messages are also observed on CircleCI ("Install Doc Dependencies" phase):
https://app.circleci.com/pipelines/github/ros-planning/navigation.ros.org/1899/workflows/f70556c1-22e1-4320-8912-6e6fc427e52b/jobs/2208

Logs after applying the changes from above requirements table, are listed here:
pip3_install_new.log
make_html_new.log

All error messages on pip3 install phase were disappeared.
On make html stage one new warning message appear:

WARNING: extlinks: Sphinx-6.0 will require a caption string to contain exactly one '%s' and all other '%' need to be escaped as '%%'.

This message was introduced in Sphinx 4.0 (https://www.sphinx-doc.org/en/master/changes.html#release-4-0-0-released-may-09-2021) and thus will be appeared in any Sphinx update configuration. This is not critical issue for now and requires an attention on next Sphinx 6++ version update, I suppose.
The documentation build passes and generates HTML well for the new configuration.

@AlexeyMerzlyakov
Copy link
Collaborator Author

AlexeyMerzlyakov commented Sep 13, 2023

Also, it is highly wanted if someone to verify these changes won't break CircleCI builds

@SteveMacenski
Copy link
Member

#466 to get CI to run. The last time we made updates to this, alot of the website broke. I'd just go through the website for a few minutes and make sure you don't run into any weird issues. The only one I see from CI right now is below on the build and install page (there used to be little paragraph signs). I don't care about the symbol; remove it if you like. I just don't want a missing unicode icon.

Screenshot from 2023-09-13 08-43-27

There is the following issue appear on Ubuntu 20.04:

CI runs focal, so should that be updated in the circleci config?

/navigation.ros.org/configuration/index.rst:9: WARNING: toctree contains reference to nonexisting document 'configuration/packages/configuring-collision-detector'

I just noticed this issue as well when building another tutorial. This looks related to the Collision Detector node PR where we split up the pages

@SteveMacenski
Copy link
Member

I think you also need to update the install instructions in circleci and the instructions in the readme

@AlexeyMerzlyakov
Copy link
Collaborator Author

AlexeyMerzlyakov commented Sep 15, 2023

I'd just go through the website for a few minutes and make sure you don't run into any weird issues.

I've made side-by-side comparison of 2 Docker containers: vanilla requirements.txt vs updated requirements.txt with sphinx==5.3.0 breathe==4.35.0 docutils==0.15 set.

There are the following changes I've noted:

Change 1
Missing unicode "chain" symbol (U+F0C1). Initially it should be a Pilcrow symbol, but after sphinx_rtd_theme package is applied, it was replaced to the "chain" permalink symbol according to the their theme. This symbol is being displayed for usual links, but somehow for is not being displayed for collapsible texts.

Actually, in sphinx-rtd-theme == 1.3.0 there is an API switch between Sphinx 3.5.0++ and less, where the permalink symbol should be U+F0C1 for all versions of Sphinx:

    if sphinx_version >= (3, 5, 0):
        app.config.html_permalinks_icon = "\uf0c1"
    else:
        app.config.html_add_permalinks = "\uf0c1"

However, for some reasons inside vanilla Sphinx 3.5.0, we did not see the support of newer claimed html_permalinks_icon API and therefore, in vanilla html-s there are Pilcrows instead of U+F0C1 chains.

Therefore, it seems that bug in render/fonts over the bug Sphinx 3.5.0 gives us a correct way to build html.
The try to workaround it could be just to change setup() routine in __init__.py of sphinx-rtd-theme package. But this option is not appropriate for us at all. The html_add_permalinks variable is set to Pilcrow in Sphinx, then changed to "chain" in sphinx-rtd-theme and we need to change it later somewhere to Pilckrow back in our user-code (say in inherited from sphinx-rtd-theme -> otc_tcs_sphinx_theme).

Change 2
I've found it in lined code renders as follows at pictures below, but it is not notable I believe:

Vanilla:
Screenshot 2023-09-14 at 15-45-38 Setting Up The URDF — Nav2 1 0 0 documentation

Updated:
Screenshot 2023-09-14 at 15-45-42 Setting Up The URDF — Nav2 1 0 0 documentation

Vanilla:
Screenshot 2023-09-14 at 15-47-26 Setting Up The URDF — Nav2 1 0 0 documentation

Updated:
Screenshot 2023-09-14 at 15-47-30 Setting Up The URDF — Nav2 1 0 0 documentation

Change 3
Strange search on updated pip3 packages.
In vanilla seach by naviagation keyword gives 76 pages matching, while in updated version: 80 pages. 4 pages are repeating twice in search results, so I'd like to consider it as an issue for now.

Moreover, if I click to the found with some keyword page, the results are not being highlighted on it, as it made on vanilla configuration:
Screenshot 2023-09-14 at 16-54-52 (SLAM) Navigating While Mapping — Nav2 1 0 0 documentation

I could add "...?highligh=navigation" post-fix in the address-string and the results will be highlighted for updated version too, but this is not made by-default.
So, the search in Sphinx 5.3.0 updated configuration should be tuned to work as we are expecting.


CI runs focal, so should that be updated in the circleci config?

I've tried to align it with currently officially supported by ROS2-Rolling Jammy, but this might be somehow not straightforward, as I thought.
So, I think the answer is No: we'd better to leave it working on previous Ubuntu LTS.

I just noticed this issue as well when building another tutorial. This looks related to the Collision Detector node PR where we split up the pages

Fixed by #464


From the above, it seems like we are opening a can of worms, when trying to update the Sphinx, which will take a lot of analysis and resources spent. So, for now, I'd like to think about to postpone the task for better times, or when we will have a need to update this: now despite of installation errors, the configuration works on CI, so it is really questionable in the reasonableness to do this.

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.

None yet

2 participants