Skip to content

Turn warnings into errors in the Makefile. #906

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

Merged
merged 8 commits into from
Jul 13, 2022
Merged

Turn warnings into errors in the Makefile. #906

merged 8 commits into from
Jul 13, 2022

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.

5 participants