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

Readthedocs config template is broken #416

Closed
robrap opened this issue Dec 6, 2023 · 7 comments · Fixed by #419
Closed

Readthedocs config template is broken #416

robrap opened this issue Dec 6, 2023 · 7 comments · Fixed by #419

Comments

@robrap
Copy link
Contributor

robrap commented Dec 6, 2023

The file https://github.com/openedx/edx-cookiecutters/blob/master/python-template/%7B%7Bcookiecutter.placeholder_repo_name%7D%7D/.readthedocs.yml gets complaints from readthedocs.

I fixed using more of the template from edx-platform, but I'm not sure of all the parts we want: https://github.com/openedx/edx-platform/blob/master/.readthedocs.yaml. Note that the requirements path is different, so don't blindly copy-paste.

This is from the readthedocs documentation: https://docs.readthedocs.io/en/stable/config-file/v2.html

Lastly, the cookiecutter itself is missing this top-level file, which means its build would probably be broken. Whether as a separate ticket or this ticket, we should probably add that file and publish the cookiecutter docs.

@robrap
Copy link
Contributor Author

robrap commented Dec 6, 2023

Readthedocs template:

# Optional but recommended, declare the Python requirements required
# to build your documentation
# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html
# python:
#   install:
#     - requirements: docs/requirements.txt

edx-platform version of this:

python:
  install:
  - requirements: "requirements/doc.txt"
  - method: pip
    path: .

Do we need the extra bits from the edx-platform template, or is that an edx-platform specific need?

@robrap
Copy link
Contributor Author

robrap commented Dec 6, 2023

I see that the extra bits for edx-platform were added in https://github.com/openedx/edx-platform/pull/32752/files. Maybe we can just see if edx-drf-extensions (recently updated here) would still pass without these final two lines?

dianakhuang added a commit that referenced this issue Dec 6, 2023
RTD updated their config format. This updates
the cookiecutter to match their recommendations.

#416
@dianakhuang dianakhuang mentioned this issue Dec 6, 2023
6 tasks
dianakhuang added a commit that referenced this issue Dec 6, 2023
RTD updated their config format. This updates
the cookiecutter to match their recommendations.

#416
@dianakhuang
Copy link
Contributor

I decided to add the extra lines to our cookiecutter config just to be on the safer side.

dianakhuang added a commit that referenced this issue Dec 6, 2023
RTD updated their config format. This updates
the cookiecutter to match their recommendations.

#416
dianakhuang added a commit that referenced this issue Dec 6, 2023
RTD updated their config format. This updates
the cookiecutter to match their recommendations.

#416
@github-project-automation github-project-automation bot moved this to Done in Arch-BOM Dec 6, 2023
@robrap
Copy link
Contributor Author

robrap commented Dec 6, 2023

I'm going to reopen for now. See #419 (comment). We may need to add additional python install details.

@robrap robrap reopened this Dec 6, 2023
@robrap
Copy link
Contributor Author

robrap commented Dec 6, 2023

@feanil: The lighter version that you used in this PR worked fine in openedx/event-routing-backends#374. I didn't look into this further, but it would be nice to know what version is needed for what, and to have it documented easily (commented out lines?) for those that will need the extra lines.

UPDATE: @feanil gives some explanation here: #418 (comment)

@feanil
Copy link
Contributor

feanil commented Dec 7, 2023

Yea, thinking more through it, I think it does in fact make sense to put the pip install . into the .readthedocs.yaml file in the cookiecutter since the cookiecutter is for python projects and will have a setup.py by default. I created #420 to add it in along with some comments.

@feanil
Copy link
Contributor

feanil commented Dec 13, 2023

Given the above mentioned PRs to correct the cookiecutter, I think it's okay to close this again.

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 a pull request may close this issue.

3 participants