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

Sphinx documentation is broken #932

Closed
ogenstad opened this issue Jun 27, 2024 · 13 comments · Fixed by #933
Closed

Sphinx documentation is broken #932

ogenstad opened this issue Jun 27, 2024 · 13 comments · Fixed by #933

Comments

@ogenstad
Copy link
Collaborator

The build process for Sphinx is broken since some time back. The problem originates from the fact that we don't even check it in the CI. We have a job to validate make sphinx however it doesn't do anything:
https://github.com/nornir-automation/nornir/blob/v3.4.1/Makefile#L25

In #855 Sphinx was upgraded from version 4.5.0 to version 6.2.1 since then the builds at https://readthedocs.org/projects/nornir/builds/ have started to fail but because we didn't validate anything within the CI nobody noticed.

I'm not sure if there are any vulnerabilities in the earlier version or why it was upgraded (if there was a reason). If it doesn't have any issues with security I'd suggest to downgrade and activate the CI job just to have it tracked and then look at what's needed to upgrade. Alternatively check what is required for it to work using the current version.

Also on Read the Docs it looks like it would build for for the stable or develop branches that doesn't exist, but not for main. Could be that I'm just missing something as I don't have any permissions on RTD. @dbarrosop, can you perhaps have a look and see if you can activate the builds for the main branch as well?

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 27, 2024

I would be against downgrading sphinx (upgrading Sphinx is a PIA). I think we should just figure out what is wrong in the process and fix with current versions or newer.

Totally agree we need better CI/CD checking here and need a way to verify that readthedocs is actually getting updated (since even with better Nornir testing it is pretty easy for readthedocs to be failing and for no one to realize it).

Probably should expand the allowed maintainers in readthedocs so it isn't entirely David (my RTD username is "ktbyers"). It looks like I am not currently a maintainer.

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 27, 2024

Note, RTD changed some things in October of last year which might also cause other breakage. See this NAPALM issue for reference:

napalm-automation/napalm#2065

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 27, 2024

Nevermind on that last item...it looks like I already fixed that (prior to it breaking last August).

@ogenstad
Copy link
Collaborator Author

Regarding upgrading Sphinx being a PITA, an issue here could be that the version was bumped without doing anything else which caused these failures. In that sense it's not property upgraded now.

@ubaumann
Copy link
Contributor

It looks like the following API has changed:

app.add_stylesheet("css/custom.css")

I should be home in 2 hours and can look for a fix and create a PR.

@ubaumann
Copy link
Contributor

As I am to blame for the PITA situation, I created a PR to fix the documentation build.
Testing it in codespaces, the created doc looks good. However, there are a lot of warnings in the build.
https://github.com/nornir-automation/nornir/pull/933/checks#step:9:149

@ogenstad
Copy link
Collaborator Author

As I am to blame for the PITA situation, I created a PR to fix the documentation build. Testing it in codespaces, the created doc looks good. However, there are a lot of warnings in the build. https://github.com/nornir-automation/nornir/pull/933/checks#step:9:149

I don't think we need to assign blame, the real problem was that the CI job was disabled for so long. I think the real problem now is that it's completely broken and as such it would be better to have it in the current state even if there are errors. Having said that this is what the -W argument to sphinx-build does i.e. it converts warnings into errors and fails early. So once it's working again we can reactivate this switch. I don't know if we can use the builtin Sphinx makefile directly for that, I think there was a reason why the old job was running the command directly (that could just have been laziness though).

@ubaumann
Copy link
Contributor

I started with fixing the warnings but not finished yet: #934

@dbarrosop
Copy link
Contributor

dbarrosop commented Jun 28, 2024

I added ktbyers to the list of maintainers and change the "latest" branch to point to main. I am not sure this is failing due to the upgrade actually. See latest build error:

nornir | Read the Docs.pdf

I am happy to add more people as RTD maintainers if you can't see the build logs

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 28, 2024

Build issue is a Python mismatch (i.e. Sphinx using one Python / Poetry using a different one).

Attempt to fix here:

39d07ee

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 28, 2024

Okay this PR was able to fix the readthedocs build issue:

#942

GH Webhook to RTD is still not working so looking into that.

@ktbyers
Copy link
Collaborator

ktbyers commented Jun 28, 2024

Okay, webhook to RTD looks to be working again...I had to add a secret from RTD into GitHub on the webhook configuration page.

@dbarrosop
Copy link
Contributor

nice work!

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.

4 participants