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

chore: update ReadTheDocs config file to be compliant #439

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

matthewfeickert
Copy link
Member

@alexander-held not sure if this is going to fix your issues, but this is basically the pyhf RTD config that has been working fine so far.

@matthewfeickert matthewfeickert added the documentation Improvements or additions to documentation label Sep 19, 2023
@matthewfeickert matthewfeickert self-assigned this Sep 19, 2023
* From https://docs.readthedocs.io/en/stable/config-file/v2.html

> This file is named .readthedocs.yaml and should be placed in the
> top level of your Git repository.
@matthewfeickert
Copy link
Member Author

@alexander-held There's no RTD test build happening at the moment. Do you have it configured to not run on PRs?

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2af877c) 100.00% compared to head (aa5ee0d) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #439   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2082      2082           
  Branches       340       340           
=========================================
  Hits          2082      2082           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.readthedocs.yaml Outdated Show resolved Hide resolved
@alexander-held
Copy link
Member

@alexander-held There's no RTD test build happening at the moment. Do you have it configured to not run on PRs?

Yes, so far it was just running on master as there was never a need for branches (local compilation has been working fine for testing). I now switched the setup temporarily to build from this branch, but still see the build failing.

These are the commands I see running:

git clone --depth 1 https://github.com/scikit-hep/cabinetry.git .
git fetch origin --force --prune --prune-tags --depth 50 refs/heads/chore/update-RTD-config:refs/remotes/origin/chore/update-RTD-config
git checkout --force origin/chore/update-RTD-config
git clean -d -f -f
cat .readthedocs.yaml
apt-get update --assume-yes --quiet
apt-get install --assume-yes --quiet -- curl jq
asdf global python 3.11.4
python -mvirtualenv $READTHEDOCS_VIRTUALENV_PATH
python -m pip install --upgrade --no-cache-dir pip setuptools
python -m pip install --upgrade --no-cache-dir pillow mock==1.0.1 alabaster>=0.7,<0.8,!=0.7.5 commonmark==0.9.1 recommonmark==0.5.0 sphinx<2 sphinx-rtd-theme<0.5 readthedocs-sphinx-ext<2.3 jinja2<3.1.0
python -m pip install --upgrade --upgrade-strategy only-if-needed --no-cache-dir .[docs,contrib]
cat docs/conf.py
python -m sphinx -T -E -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html

The thing I do not understand here is what triggers this line

python -m pip install --upgrade --no-cache-dir pillow mock==1.0.1 alabaster>=0.7,<0.8,!=0.7.5 commonmark==0.9.1 recommonmark==0.5.0 sphinx<2 sphinx-rtd-theme<0.5 readthedocs-sphinx-ext<2.3 jinja2<3.1.0

This installs sphinx-rtd-theme<0.5 which does not subsequently get updated, as the dependency listed in cabinetry has no lower bound on this:

cabinetry/setup.py

Lines 27 to 31 in 2af877c

"sphinx!=5.2.0.post0", # broken due to version parsing in RTD theme
"sphinx-click",
"sphinx-copybutton",
"sphinx-jsonschema",
"sphinx-rtd-theme",

I imagine this could be solved by adding a lower bound, I'll push a commit to this branch here to try it out.

It is very unclear to me why this one step happens with the upper bounds set though (which is what ultimately causes the problem). Maybe @henryiii might have an idea?

@alexander-held
Copy link
Member

I can confirm that the build passes again with the changes from aa5ee0d. It feels to me that there should be a better solution that addresses what happens in the build in that extra step. Unless anyone has other ideas, I think this is good enough to go ahead with though.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 20, 2023

I agree that's strange, but I also agree that this is probably not worth the effort to debug further.

@henryiii
Copy link
Member

RtD pre-installs old versions of things, like the old rtd theme. Setting a minimum forces the upgrade.

@alexander-held alexander-held changed the title chore: Update ReadTheDocs config file to be compliant chore: update ReadTheDocs config file to be compliant Sep 20, 2023
Copy link
Member

@alexander-held alexander-held left a comment

Choose a reason for hiding this comment

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

Thanks everyone, this looks good to me then.

* add build.os config setting for readthedocs
* set lower bound of readthedocs Sphinx theme to fix readthedocs CI

@alexander-held alexander-held merged commit 80b4af1 into master Sep 20, 2023
7 checks passed
@alexander-held alexander-held deleted the chore/update-RTD-config branch September 20, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants