-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
Conversation
There was a problem hiding this 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
.
Actually Is there any downside in adding them both directly to the |
Co-authored-by: Adam Turner <[email protected]>
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. |
There was a problem hiding this 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 bespokeSPHINXERRORHANDLING
one—I thought that was whatSPHINXOPTS
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 theconf.py
contains an invalid escape sequence due to being a regex without ar
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!
Ok.
The only reason is to keep consistent with cpython/Doc/Makefile. Adding it directly to
I'll update it once we decide which options we want.
I can fix this too. |
It seems that
Perhaps for So the plan would be:
|
FWIW, in case of errors |
I think it's fine to bail: it's a prompt to fix the (new) errors. |
Anyone on Windows wants to check if the bat works? 🦇 |
There was a problem hiding this 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)
Co-authored-by: CAM Gerlach <[email protected]>
There was a problem hiding this 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 !
This PR turns warnings into errors, in a way similar to what
cpython/Doc/Makefile
does.