Add Github Problem Matchers actions for GCC/MSVC

[jschmidt-icinga]

Closes #10545

This PR adds Github Problem Matchers for GCC and MSVC. Warnings are highlighted during compilation and annotations are added at the top of the runner page after they finish. If these annotations appear in files that are part of a PR, they area also highlighted on the "Files changed" page, directly in the code and with a warning symbol in the file list.

I've also enabled -Wall -Wextra on all Linux runners now. We can still discuss this and disable individual warnings we disagree with via -Wno-foo.

This PR also removes a few various non-code-related warnings.

• GCC/clang pragmas being used unconditionally
• boost being built without defining BOOST_COROUTINES_NO_DEPRECATION_WARNING on amazonlinux:2
• Git errors about a non-existing submodule (this isn't even a warning generated by these actions, but too small and unimportant to fix in a dedicated PR)
• Disabled warnings for unused parameters/variables on generated files (similar to how it had already been on MSVC, or at least I assume that's what the disabled warnings on MSVC are)

[julianbrost]

• The annotations only appear when opening the Checks tab and clicking on specific runner, which makes this considerably less useful in my eyes.
• There is no distinction between warnings in code touched in the PR and from the overall codebase

Shouldn't the warnings show up in the "Files changed" tab? If I look at this right now, I even see "Unchanged files with check annotations" at the bottom, so if there was a warning within the actually changed lines of the PR, I'd expect it to show there?

• It also catches a git fatal checkout error as a "warning". Is this a known issue or something new?

I've seen this before, but only because I made the mistake to try the Copilot "feature" to explain pipeline errors and is said this message from a cleanup step at the end was the cause for a C++ compile error way before that was actually the error failing the pipeline. So not new, but if it wasn't for that, I'd have never seen it before and also didn't look into it further.

• Do we already want to enable additional warnings in this PR? -Wall?

Something like that would have been my future plan, but if the warnings don't even show up in a useful place, that wouldn't be of much help.

• Does anyone know a good way to get these warnings onto the main PR page, ideally the PR-status at the bottom?

I mean it's possible to submit completely custom status checks (see cla-bot for example), but then you're down to writing a custom GitHub API/webhook integration for a linter (I don't know if you can show more than ok/failed from a GH Actions workflow). But my hope would have been that these problem matchers give all we need basically for free.

[jschmidt-icinga]

Shouldn't the warnings show up in the "Files changed" tab?

I've just pushed my commit that adds a test warning again (I had previously removed it) to see if that works. It didn't work before because the problem matcher didn't know what to do with the file paths we get in our warnings (i.e. "/icinga2/lib/foo/bar" or "../lib/foo/bar"). I've adapted the regex to strip away the prefixes and now the annotations seem to point to actual files. However, it still seems like only links to changed files actually work. I would have expected that links from Checks to files not changed in the PR would lead to the master branch version of the file, but it then seems to link to the first file in "Files Changed".

So lets see if links from Checks and annotations in Files Changed work now.

Edit: Seems to work.
Next step is to get the surrounding lines into the annotation, so you don't have to get back to look at the runner output anyway. I think that should be possible with multi-line matching.

[julianbrost]

It didn't work before because the problem matcher didn't know what to do with the file paths we get in our warnings (i.e. "/icinga2/lib/foo/bar" or "../lib/foo/bar").

We're mounting the source into the container at a location that's different from the path used on the runner directly:

icinga2/.github/workflows/linux.yml

Lines 87 to 88 in 87df80d

	docker run --rm -v "$(pwd):/icinga2" -e DISTRO=${{ matrix.distro }}
	--platform ${{ matrix.platform }} ${{ matrix.distro }} /icinga2/.github/workflows/linux.bash

So yeah, there's a good chance that it can only handle (absolute) paths if they point into the directory the source is checkout out to on the runner.

[julianbrost]

Looking good now! That's exactly what I hoped for, just making the warning "in your face" for anyone looking at the PR diff:

[jschmidt-icinga]

It seems I misunderstood multi-line matching. From the docs it seemed

message: a group number containing the error message. required at least one pattern must set the message

message could be specified multiple times and I assumed (naively) that the runner will then just concatenate the message. Alas, no detail for the error messages. This feature would be so much more useful if you could transform the captured groups somehow.

[jschmidt-icinga]

I think the regexes should be ok now. I've also added a commit that turns on -Wall -Wextra, but that'll likely not be in the finished version of this PR, at least not in that form.

[jschmidt-icinga]

• Had to do some more rounds of trial and error to match MSVC's multi-line template errors correctly when the error is in an STL header (thus not located in our project directory). In this case the source file causing the error is only mentioned several lines down and all lines in between the first line (containing the error message, code, fromPath) and the line with the source file paths. This is probably still a bit brittle, but it should work fine for our own checks.

The only way I could do this was add a second matcher, because the variables file, line and column can only be matched in a single expression.

I really wish these actions were more flexible so you could match multi-line errors in a single expression or could conditionally overwrite variables in successive matches (or concatenate in case of message). But what do I expect from something that was made for VS Code by Microsoft and then ported to Github. 🤷

[jschmidt-icinga]

• Silenced/Fixed a few non-code-related warnings. Details in the PR description.

[jschmidt-icinga]

• Turns out, not only weren't the GCC/Clang pragmas excluded for MSVC, the MSVC pargma wasn't even included because the define had a typo, i.e. _MSVC_VER instead of _MSC_VER as it's supposed to be. This puts into question why the test case still worked even without that pragma, but I'm only here to fix the warning.

[jschmidt-icinga]

• Fixed the CMAKE_CXX_FLAGS vs. CXXFLAGS conflict
• Disabled -Wtemplate-id-cdtor, because it was too verbose and -Wstringop-overflow because it has too many false positives originating in headers we don't control.

If any other warnings feel superfluous we can disable those as well (many warnings in -Wall -Wextra are debatable), or enable others not in those two that could be useful (-fanalyzer anyone?).

[jschmidt-icinga]

• Did a small experiment on a separate branch and it turns out two matchers in one file does work. I probably screwed up the regex when I tried that last time.

[julianbrost]

That's a change mostly to our CI jobs, so let's just see if this turns out useful. If not, we can still improve it or drop it again (which I don't hope we'll need to do).

[Al2Klimov]

Disabled warnings for unused parameters/variables on generated files (similar to how it had already been on MSVC, or at least I assume that's what the disabled warnings on MSVC are)

MSVC warnings 4244 and 4800 have nothing to do with unused symbols.

• https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4244?view=msvc-170
• https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4800?view=msvc-170

[jschmidt-icinga]

I know that, I was just reusing the #ifdef block. To me, this was a non-exhaustive list, per platform, of warnings we don't want in our generated files. We may also add more warnings later in case they come up.

PS: I now realize this probably needs a version for __clang__ too. I'll add it to #10609.

[Al2Klimov]

So you didn't just mix them up while attempting to restore OS symmetry, but you explicitly think the changes highlighted above were a good idea on their own?