Skip to content

Conversation

@ezio-melotti
Copy link
Member

This PR turns warnings into errors, in a way similar to what cpython/Doc/Makefile does.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Assuming we add --keep-going.

@ezio-melotti
Copy link
Member Author

Actually cpython adds both -W and --keep-goingin .github/workflows/doc.yml, and -W in cpython/Doc/Makefile.

Is there any downside in adding them both directly to the Makefile?

Co-authored-by: Adam Turner <[email protected]>
@CAM-Gerlach
Copy link
Member

Is there any downside in adding them both directly to the Makefile?

Users' local builds will fail slower, but they can Ctrl-C them anyway if they want and I'd still rather have the full error output, especially for something that only takes a few seconds to build like the devguide.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Strong 👍 on -W --keep-going.

  • Can we add -n too to automatically catch any broken internal links/ref targets? Seems like it builds cleanly with it with your PR.

  • Is there a reason we can't add it to the existing SPHINXOPTS variable instead of making a new bespoke SPHINXERRORHANDLING one—I thought that was what SPHINXOPTS is for, which is why I usually add it there, but maybe I was mistaken.

  • Wouldn't it make sense to add it to the make.bat too, so they are consistent?

  • Testing this locally with my standard python -b -X dev -m sphinx to catch Python-level warnings, I noticed that line 227 in the conf.py contains an invalid escape sequence due to being a regex without a r raw string prefix. Since its a one line change and fixes a warning, might be worth just fixing here instead of a whole separate PR.

Thanks!

@ezio-melotti
Copy link
Member Author

Can we add -n too

Ok.

Is there a reason we can't add it to the existing SPHINXOPTS variable instead of making a new bespoke SPHINXERRORHANDLING one

The only reason is to keep consistent with cpython/Doc/Makefile. Adding it directly to SPHINXOPTS (both here and in the cpython repo) is fine with me.

Wouldn't it make sense to add it to the make.bat too, so they are consistent?

I'll update it once we decide which options we want.

I noticed that line 227 in the conf.py contains an invalid escape sequence

I can fix this too.

@ezio-melotti
Copy link
Member Author

It seems that -W was

Perhaps for cpython, having -W without --keep-going was desirable for local build but not for CI, hence the addition of a separate SPHINXERRORHANDLING. Note that cpython doesn't have SPHINXOPTS, and that it could be used instead of SPHINXERRORHANDLING, but if we change it on cpython for consistency we should make sure that CI is updated accordingly.

So the plan would be:

  • set SPHINXOPTS = -W --keep-going -n and fix conf.py for this repo
  • set SPHINXOPTS = -W and update CI for cpython (in a separate issue)

@ezio-melotti
Copy link
Member Author

FWIW, in case of errors make htmlview will now bail after running the html target and won't open the browser. I don't know if this is something we can/want to fix.

@hugovk
Copy link
Member

hugovk commented Jul 2, 2022

I think it's fine to bail: it's a prompt to fix the (new) errors.

@ezio-melotti
Copy link
Member Author

Anyone on Windows wants to check if the bat works? 🦇

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

I had to set the PYTHON environment variable to python to get it to work at all (due to my Python install not shipping with py), and it created its own whole venv instead of using my existing conda env (which it fortunately did not muck with), but it then exited with an error due to the args being quoted. However, I tested it and it appears to be an easy fix.

(To be honest, I usually don't use makefiles or helper scripts for Sphinx build orchestration, as opposed to using python -m sphinx myself, as they tend to do too much magic, not work in many scenarios and get in the away of me passing the args I want. And I have make on my system so I usually use that anyway instead of batch files)

Copy link
Member

@CAM-Gerlach CAM-Gerlach 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 @ezio-melotti !

@ezio-melotti ezio-melotti merged commit 02f2b98 into main Jul 13, 2022
@ezio-melotti ezio-melotti deleted the warns-2-errs branch July 13, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants