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

feat: optional dependency management #174

Merged

Conversation

dviererbe
Copy link
Contributor

Changes:

  • automatically generates .sphinx/requirements.txt based on used extensions (when make install is invoked)
    • only enables redirects extension if any redirects are specified
    • only enables open graph extension if any configuration is present
    • only enables myst extensions if any configuration is present
    • moved sphinx_tabs.tabs, canonical.* and notfound.extension to custom_extensions
  • minor spacing adjustments

Further context: Issue #140

@dviererbe dviererbe closed this Jan 16, 2024
@dviererbe dviererbe deleted the add-optional-dependency-management-mechanism branch January 16, 2024 11:36
@dviererbe dviererbe restored the add-optional-dependency-management-mechanism branch January 16, 2024 11:38
@dviererbe dviererbe reopened this Jan 16, 2024
@dviererbe
Copy link
Contributor Author

@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 dviererbe force-pushed the add-optional-dependency-management-mechanism branch from 1e71f1b to 1e9378f Compare January 17, 2024 15:04
@dviererbe
Copy link
Contributor Author

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

@pmatulis
Copy link
Contributor

[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.

Copy link
Collaborator

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

No thorough review yet, but overall I think this will work. 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.

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).

@dviererbe
Copy link
Contributor Author

dviererbe commented Jan 17, 2024

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
Copy link
Collaborator

ru-fu commented Jan 17, 2024

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
Copy link
Contributor Author

@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
Copy link
Collaborator

ru-fu commented Jan 17, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

ru-fu commented Jan 18, 2024

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).

build_requirements.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
@dviererbe dviererbe force-pushed the add-optional-dependency-management-mechanism branch from 5df9a7f to 9e68db5 Compare January 18, 2024 22:44
@dviererbe
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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 9e68db5

.readthedocs.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

I did some testing now, so a few more comments.
But overall it works nicely! :)

init.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
build_requirements.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
custom_conf.py Outdated Show resolved Hide resolved
readme.rst Outdated Show resolved Hide resolved
readme.rst Show resolved Hide resolved
Copy link
Collaborator

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good!

Do you want to squash any commits before merging?

dviererbe and others added 3 commits January 23, 2024 17:25
* `.sphinx/requirements.txt` was removed and added to `.gitignore`
* `make clean` deletes `.sphinx/requirements.txt` (if present)
* `make install` automatically generates `.sphinx/requirements.txt`
  (if not present) based on the configuration specified index
  `custom_conf.py`:
  - only enables redirects extension if any redirects are specified
  - only enables open graph extension if any configuration is present
  - only enables myst extensions if any configuration is present
  - moved `sphinx_tabs.tabs`, `canonical.`* and `notfound.extension`
    to `custom_extensions`
  - canonical sphinx module will only be used as dependency if any
    canonical sphinx extension is specified in `custom_extensions`
    (legacy names (without "canonical." prefix) are handled correctly)
  - if any automatically handled extension is explicitly specified in
    `custom_extensions` it will be enabled even if the configuration
    context suggest otherwise
  - the resulting list of extension will be deduplicated (legacy names
    of canonical sphinx extension are handled correctly)

Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
* `.readthedocs.yaml` instructs Read The Docs (RTD) builder to generate
  `.sphinx/requirements.txt` post checkout
* `init.sh` script correctly updates the path where the
  `build_requirements.py` is located/needs to be executed

Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
@dviererbe dviererbe force-pushed the add-optional-dependency-management-mechanism branch from 9352e1e to 7ec0fcf Compare January 23, 2024 17:16
Documents the added optional dependency mechanism in the `readme.rst`.

Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
@dviererbe dviererbe force-pushed the add-optional-dependency-management-mechanism branch from 7ec0fcf to bf41e17 Compare January 23, 2024 17:17
@dviererbe
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

dviererbe commented Jan 24, 2024

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:

  • 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 dd0ae84 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
Copy link
Collaborator

ru-fu commented Jan 25, 2024

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
Copy link
Collaborator

ru-fu commented Jan 25, 2024

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?

@ru-fu ru-fu merged commit 0d74850 into canonical:main Jan 25, 2024
2 checks passed
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

3 participants