CI: Mark CGO warnings as errors #3192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
reviewed
Aug 31, 2021
kolyshkin
reviewed
Aug 31, 2021
Treat warning as errors only in the CI. We can enforce it in the source code (like setting CFLAGS in libcontainer/nsenter/nsenter.go), but that can force other downstream to patch the code if thei C compiler produces warnings. For that reason, we do it only on the CI. Todays CGO warnings are quite hidden in the CI (only shown for the compilation step, that is collapsed) and CI is green anyways. With this patch, CI fails if a warning is introduced. Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
9733ef6 to
347c371
Compare
|
@kolyshkin yes, verified that in a fork before opening the PR already. I even tried with that same commit you mentioned, it works as expected :) |
AkihiroSuda
approved these changes
Sep 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up of: #3165. In particular, #3165 (comment).
As pointed out there by @kolyshkin and @tianon there, this adds the -Werror flag to the CI only.
Note that I kept the "-g -O2" that is the default from "go env CGO_CFLAGS". The reason is: the warning that we fixed recently (db8330c) is not seen if we don't use those flags. So, what is expected happens: if no env var CGO_CFLAGS is defined, it uses "-g -O2", but if it is defined, it uses what we specify. Therefore, if we add the env var CGO_CFLAGS to "-Werror" only, then it doesn't use "-g -O2", the warning is not produced and therefore not catched. So, I kept those things, but it is hardcoded (instead of append -Werror to whatever the output of "go env CGO_CFLAGS" is) because github actions need to know it statically if we apply them to all jobs
We can probably append "-Werror" to the output of "go env CGO_CFLAGS", but we need to do that in the "run:" section and, therefore, can't change it for all jobs (we can manually change all jobs if we want that, but we need to also remember in the future).
I prefer hardcoding "-g -O2" rather than changing all jobs or creating a new target that only compiles with -Werror, as we might have more buildtags in the future and IMHO it seems simpler to have all tests in the test workflow compile the same way. But let me know if you prefer otherwise :)