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

Docs: Reset requirements.txt path #9877

Merged
merged 6 commits into from
Feb 1, 2024
Merged

Conversation

zoracon
Copy link
Contributor

@zoracon zoracon commented Jan 18, 2024

Readthedocs does not like relative paths. I believe it can just read down from the root of the repo (hopefully). There's no clear way to test this locally.

@zoracon zoracon changed the title Reset requirements.txt path Docs: Reset requirements.txt path Jan 18, 2024
@zoracon zoracon requested review from a team, wgreenberg and bmw and removed request for a team and wgreenberg January 18, 2024 21:36
@zoracon
Copy link
Contributor Author

zoracon commented Jan 23, 2024

Seeing that there is a way to test with the branches. I am assuming someone adjusted to see that. So putting merging on hold until I resolve the error.

bmw
bmw previously approved these changes Jan 23, 2024
Copy link
Member

@bmw bmw left a comment

Choose a reason for hiding this comment

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

Progress! Using the same process to test this that I described here, I got this build. Now it properly finds the file, but it fails to install everything in it because this file contains ALL possible Python dependencies of ours and readthedocs is missing non-Python dependencies like Augeas our project links to.

To suggest a possible path forward, I'd maybe use our requirements file as constraints for pip and then configure readthedocs to only install the parts of our code it needs. This is the kind of setup we had before in files like https://github.com/certbot/certbot/blob/7ca1b8f2867b252f8657b49a3fb4c374670a629d/certbot-dns-cloudflare/readthedocs.org.requirements.txt before readthedocs decided it no longer wanted to support it 🙁 I found an example of this kind of configuration with the new yaml files at readthedocs/readthedocs.org#7258 (comment) although I think it may work to just use these existing "readthedocs.org.requirements.txt" files as the requirements in each respective yaml file.

I think it's up to you if you'd like to merge this as a correct, incremental improvement or continue iterating on this here.

@zoracon
Copy link
Contributor Author

zoracon commented Feb 1, 2024

@bmw Okay I redirected to the requirements.txt and have passing builds.

Copy link
Member

@bmw bmw left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for wrangling this Alexis.

@bmw bmw merged commit 0a56581 into master Feb 1, 2024
16 checks passed
@bmw bmw deleted the zoraconpatch-readthedocs-reqtxt branch February 1, 2024 16:27
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

2 participants