search

canonical / sphinx-docs-starter-pack

A documentation starter-pack
https://canonical-starter-pack.readthedocs-hosted.com/
Other
13 stars 35 forks source link

feat: optional dependency management #174

Closed dviererbe closed 7 months ago

dviererbe commented 7 months ago

Changes:

Further context: Issue #140

dviererbe commented 7 months ago

@ru-fu (or someone else) can you give me the details why the CI fails? I do not have the permissions to see the logs.

dviererbe commented 7 months ago

Note: I had an outdated local main. I rebased to resolve a merge conflict. Read the Docs build still failed :/

pmatulis commented 7 months ago 
[rtd-command-info] start-time: 2024-01-17T15:05:04.797851Z, end-time: 2024-01-17T15:05:05.505930Z, duration: 0, exit-code: 1
python -m pip install --exists-action=w --no-cache-dir -r .sphinx/requirements.txt
ERROR: Could not open requirements file: [Errno 2] No such file or directory: '.sphinx/requirements.txt'

I will provide full log out of band.

dviererbe commented 7 months ago

I'm a little concerned though because this will be a breaking change that requires all teams to go through their current requirements and figure out which ones they need to add manually, and it's not easy to see which ones are included and which are missing.

Note: I thought that my solution covers this automatically. Every team just has to run make clean && make install and the .sphinx/requirements.txt file will be generated based on their needs.

Question: @ru-fu can you explain this to me with a simple example where this does not apply?

ru-fu commented 7 months ago

Question: @ru-fu can you explain this to me with a simple example where this does not apply?

Disclaimer: I haven't tested it yet, so it's totally possible that I've just misread the code. ;)

For LXD, for example, we are using an extension called sphinx_remove_toctrees, which we have in the custom_extensions list, and where we load the corresponding package (sphinx-remove-toctrees) in the requirements file. If I now update and remove the requirements file, the package will not be loaded and the build will fail. I need to first build to generate requirements.txt, compare that to what I had, and then figure out for this package (and possibly others) that I need to specify it in custom_required_modules. But I don't need to add ALL of the extensions from custom_extensions there, because some are already included.

dviererbe commented 7 months ago

@ru-fu I see :/ That is indeed not covered.

Thought: Maybe we should write a tiny migration guide that you could point to. For example an expanded version of:

Go to the .sphinx/requirements.txt and copy every line that is not contained on the following list to the custom_required_modules array contained in the custom_conf.py file:

  • canonical-sphinx-extensions
  • furo
  • linkify-it-py
  • myst-parser
  • pyspelling
  • sphinx
  • sphinx-autobuild
  • sphinx-copybutton
  • sphinx-design
  • sphinx-notfound-page
  • sphinx-reredirects
  • sphinx-tabs
  • sphinxcontrib-jquery
  • sphinxext-opengraph

Thought: I can also spend some more time and write a migration helper that makes this diff and tells the user what to do (should not be too complicated).

ru-fu commented 7 months ago

I don't think we need a script for that, but we should have some clear indication on which packages can be ignored (because they are included automatically). The best place for this would be in custom_conf.py imo if we can keep it short enough (there we can leave out all required packages, for example).

dviererbe commented 7 months ago

Note: I added documentation and a fail safe mechanism. In case that someone explicitly defines an automatically handled extension the extension will be enabled and the module added to the requirements even if it does not make a lot of sense.

Question: What do you think?

dviererbe commented 7 months ago

Note: The CI fails because of linkcheck this looks to me like a temporary unavailability of the service and not related to my commit.

ru-fu commented 7 months ago

For the build failure, the problem is that RTD doesn't use the Makefile. So you'll need to add a prebuild step to .readthedocs.yaml (see here for some examples).

You're still relying on the Makefile - however, some teams don't build locally, so their requirements file will never update. Instead of checking it in, it should be added to the .gitignore file and be generated before every build (both locally through the Makefile and on RTD through .readthedocs.yaml).

dviererbe commented 7 months ago

Note: I force-pushed to change the last commit (9e68db5c76817503dc5038d37bd05f2791255088).

I initially used pre_build instead post_checkout to run make .sphinx/requirements.txt. This resulted in a build failure, because RTD wants to read the file before pre_build.

dviererbe commented 7 months ago

For the build failure, the problem is that RTD doesn't use the Makefile. So you'll need to add a prebuild step to .readthedocs.yaml (see here for some examples).

You're still relying on the Makefile - however, some teams don't build locally, so their requirements file will never update. Instead of checking it in, it should be added to the .gitignore file and be generated before every build (both locally through the Makefile and on RTD through .readthedocs.yaml).

Note: I somehow missed this part. Resolved with https://github.com/canonical/sphinx-docs-starter-pack/pull/174/commits/9e68db5c76817503dc5038d37bd05f2791255088

dviererbe commented 7 months ago

Note: It can be merged now. I squashed all the commits into logical commits (should be easier to understand now). I didn't expect to do squashing at the end, so I could have written shorter commit messages 😅

dviererbe commented 7 months ago

TODO:

We'll need to add this change to the change log when it's merged so users know this is a breaking change.

dviererbe commented 7 months ago

Note: I do not know why the RTD build failed. Looks like an RTD Infra problem, because there were no logs in the admin interface. Additionally the same source build fine before all the squashing and the squashing introduced no new changes since then.

dviererbe commented 7 months ago

Suggestion: @ru-fu How about this change log entry? Feel free to make any changes as you see fit.

BREAKING CHANGE: An dependency management system was added. You can now opt-out of the extensions myst_parser, sphinx_reredirects, sphinxext.opengraph, sphinx_tabs.tabs, canonical.youtube-links, canonical.related-links, canonical.custom-rst-roles, canonical.terminal-output, notfound.extension.

Recommended actions:

make clean
git commit .sphinx/requirements.txt
make install

See dd0ae8400b0e9f94f34cefb06bc61b8f766ed3ae for more details about how the dependency management system works.

Note: As raw markdown:

**BREAKING CHANGE:** A dependency management system was added. You can now opt-out of the extensions `myst_parser`, `sphinx_reredirects`, `sphinxext.opengraph`, `sphinx_tabs.tabs`, `canonical.youtube-links`, `canonical.related-links`, `canonical.custom-rst-roles`, `canonical.terminal-output`, `notfound.extension`.

Recommended actions:
* Move custom required Python modules to the `custom_required_modules` array in the `custom_conf.py` file.
* Delete the `.sphinx/requirements.txt` file. It will now get generated based on the configuration in `custom_conf.py` with `make install`.
* Reinstall the local virtual Python environment with `make clean && make install`.

make clean git commit .sphinx/requirements.txt make install


See dd0ae8400b0e9f94f34cefb06bc61b8f766ed3ae for more details about how the dependency management system works.
ru-fu commented 7 months ago

Note: I do not know why the RTD build failed. Looks like an RTD Infra problem, because there were no logs in the admin interface. Additionally the same source build fine before all the squashing and the squashing introduced no new changes since then.

No sure what it was, but I rebuilt and it's now green. :)

ru-fu commented 7 months ago

Suggestion: @ru-fu How about this change log entry? Feel free to make any changes as you see fit.

Looks good to me, thanks! I made two tiny changes (only in the Markdown section). Will you put it in?