-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add -DPYBIND11_WERROR=ON
to mingw cmake commands
#4073
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
…OSE_MAKEFILE:BOOL=ON`).
…inding all warnings." This reverts commit f36b0af.
@henryiii this is ready for review now (I don't know if you get auto notifications, in addition to the auto-add as reviewer). |
@Skylion007 @henryiii did you see this PR? I believe this is a very quick review. Just a little bit of tweaking to silence warnings about Eigen code, and about I believe that extra little noise is a very small price to pay for having |
Hoping you can take a look. This is a very simple PR, plugging an obvious hole in our |
.github/workflows/ci.yml
Outdated
@@ -929,7 +929,7 @@ jobs: | |||
run: git clean -fdx | |||
|
|||
- name: Configure C++14 | |||
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=14 -DDOWNLOAD_CATCH=ON -S . -B build2 | |||
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=14 -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build2 |
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.
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=14 -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build2 | |
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=14 -DCMAKE_VERBOSE_MAKEFILE=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build2 |
.github/workflows/ci.yml
Outdated
@@ -947,7 +947,7 @@ jobs: | |||
run: git clean -fdx | |||
|
|||
- name: Configure C++17 | |||
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=17 -DDOWNLOAD_CATCH=ON -S . -B build3 | |||
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=17 -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build3 |
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.
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=17 -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build3 | |
run: cmake -G "MinGW Makefiles" -DCMAKE_CXX_STANDARD=17 -DCMAKE_VERBOSE_MAKEFILE=ON -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -S . -B build3 |
Let's be consistent, and this type is really only to help cmake-gui anyway.
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.
Minor nit, but otherwise seems fine. Verbose makefile make logs a bit long, not sure I prefer them, but okay if you want to.
Thanks for the review!
This setting is only for the CI. I often analyze the logs with scripts. Having the information in the logs makes the difference between day and night (seeing and being blind). Another way to look at this: reducing is easy and conclusive, guessing is hard and ambiguous. |
Description
Add
-DPYBIND11_WERROR=ON
to mingw cmake commands, plus a couple minor tweaks to resolve warnings.Brings the two MINGW jobs on par with all other CI jobs.
Suggested changelog entry: