[workflow] Use MSVC problem matcher for Windows action build #18532
Conversation
|
Looks pretty nice, though I'm concerned that I've gotten more server errors trying to access this PR than I've ever seen from GitHub in the past. Is it possible that it's due to the extra annotation? Also, where is the source code for the action? I'd like to review it before enabling it. |
yeah... I think Github still has some kinks to iron out, I've had some weird problems with annotations before too.
The source code is here: https://github.com/ammaraskar/msvc-problem-matcher primarily consisting of these two files: |
|
Ping @zooba, looks like some new warnings managed to sneak their way into the repo. If you'd prefer it's also possible to put the problem matcher into the CPython repo but it might get stale faster there as opposed to an action published on the market. |
|
@pablogsal @zware updated to check-in the problem matcher into the repo, take a look at the bottom of https://github.com/python/cpython/pull/18532/files for the existing warnings. |
There was a problem hiding this comment.
It's a little unfortunate that all of the warnings are duplicated (or triplicated, in some cases). I'm also still worried about whether these warnings are going to appear on every PR henceforth; do we know for sure what will happen there? Is it every warning, or just new ones that appear?
Other than those two concerns, LGTM.
Yeah, looks like MSVC outputs the warnings on the files they occur and then a summary leading to the duplication. VSCode de-duplicates warnings if the text and line numbers match exactly but Github Actions don't. I've filed actions/runner#504 upstream, let's see what they say.
The behavior is that if you make changes to a file without the warnings, then they will appear like they do in this PR. At the bottom and marked as As such, before merging this we should try to get rid of as many as the warnings first I think. |
|
Thanks to
most of the existing warnings are gone. There are 3 left but those are minor. This should be ready to go @pablogsal @zware |
|
Closing and re-opening to check if there are no more warnings left. |
Right, the commit aefb69b should fix 3 warnings about pragma in _zoneinfo.c. |
|
Can you try to rebase your PR on the master branch? There are warnings which are already fixed in master. |
53a5d8f to
f27af0a
Compare
|
Rebased, looks like the "Unchanged files with check annotations" section is gone now. And there don't seem to be any more warnings highlighted in the log. Test failure looks unrelated. |
|
This change looks like a nice enhancement! I hate having to dig into compiler logs to spot some new warnings. The "problem matcher" thing looks convenient. |
…H-18532) This makes warnings and errors from the compiler very prominent so this should help prevent warnings from sneaking into the code base and catch them in review. See https://discuss.python.org/t/using-github-problem-matchers-to-catch-warnings-early/4254 for more details You can see a demo of this in action here: https://github.com/ammaraskar/cpython/pull/15/files#diff-9ba2eeca0f254ece0a9df4d7cb68e870 GCC and Sphinx matchers have previously been added in pythonGH-18567 and pythonGH-20325, respectively.
This makes warnings and errors from the compiler very prominent so this should help prevent warnings from sneaking into the code base and catch them in review. See https://discuss.python.org/t/using-github-problem-matchers-to-catch-warnings-early/4254 for more details
You can see a demo of this in action here: https://github.com/ammaraskar/cpython/pull/15/files#diff-9ba2eeca0f254ece0a9df4d7cb68e870
or in screenshot form: