-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
docs: update readthedocs config to new options - v2 #9566
Conversation
Our documentation was failing to build, seems connected to the new way of indicating build options (cf https://readthedocs.org/projects/suricata/builds/22112658/, https://docs.readthedocs.io/en/stable/config-file/v2.html#build, and https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os). Added the build.os required new field, and adjusted the way python version is passed.
Apparently, for the new configuration style for read the docs, one of the ways to pass extra configuration for python is having a requirements file.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9566 +/- ##
==========================================
- Coverage 82.20% 82.19% -0.02%
==========================================
Files 968 968
Lines 274275 274275
==========================================
- Hits 225461 225430 -31
- Misses 48814 48845 +31
Flags with carried forward coverage won't be shown. Click here to find out more. |
build: | ||
os: ubuntu-22.04 | ||
tools: | ||
python: "3.8" |
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.
Lets go to Python 3.11 as well, 3.8 is close to EOL, at 3.11 appears to work OK.
install: | ||
- requirements: doc/userguide/requirements.txt |
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.
Was, this (the second commit) required to make things work? Or a stylistic change?
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.
Answered my own question, it does not build without this change. I'd squash these 2 commits since they both appear required to fix RTD.
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.
Required. RtD was failing to find the correct sphinx theme without this. There seem to be some ways of making this work, this was the one I was able to understand and follow.
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.
Replaced by: #9572 |
Previous PR: #9564
Link to redmine ticket: none
Describe changes from last PR:
readthedocs.yaml
Checked with my own instance of rtd and seems to work, this time: https://suri-rtd-test.readthedocs.io/en/rtd-yaml-update-v2/
Leaving this as draft, as I don't know if this is the style we want to follow...