Skip to content
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

Check for GCC/Clang compiler warnings in Ubuntu GitHub Actions CI #304

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

aloisklink
Copy link
Contributor

@aloisklink aloisklink commented Aug 23, 2023

Add a CMakePresets.json file that contains ci-ubuntu preset. This preset contains most of the recommended compiler warnings from the cmake-init project, with the following changes:

  • I had to remove the -Wdouble-promotion and -Werror=float-equal flags since there are issues within the GTest headers.
  • For compatibility with g++-7, which doesn't support this flag, -Wextra-semi was removed.
  • I also had to remove some of the following flags like: -fstack-protector-strong -fstack-clash-protection. These flags cause errors in Clang v10, and to be honest, with stuff like address sanitizer, we probably don't need them.

I've also set the "CMAKE_CXX_FLAGS_DEBUG": "-Werror" variable when running on ubuntu in GitHub Actions. This will make the ubuntu build fail whenever any warnings are seen in the code, so you'll instantly spot it whenever somebody tries to add code that has warnings to this project. This should prevent issues like #294 from happening in the future.


I've left this PR as a draft, since the CI currently fails, but after the following PRs are merged, this CI job should work All of these PRs have now been merged, so CI passes:

@skypjack
Copy link
Owner

-Dlibuv_buildtests=OFF is gone during the rework. Am I wrong?

@aloisklink
Copy link
Contributor Author

-Dlibuv_buildtests=OFF is gone during the rework. Am I wrong?

I've added it to the dev-mode CMake preset:

uvw/CMakePresets.json

Lines 26 to 29 in 8a23e58

"cacheVariables": {
"BUILD_TESTING": true,
"libuv_buildtests": false
}

It will make it easier for us developers to run cmake --preset dev-mode locally, and in the future, we can add a ci-windows/ci-darwin presets for doing the Windows/MacOS CI runs, that also inherit from the dev-mode preset, so we'll only have to modify everything in a single file.

The only problem is that is cmake-presets are only supported in CMake 3.19 or later, but since we're only using them for CI/development (they won't be used by end-users), this isn't too big of an issue for us!

Add a [`CMakePresets.json`][1] file that contains `ci-ubuntu` preset.
This preset contains most of the recommended compiler warnings from the
[`cmake-init`][2] project.

However, I had to remove the `-Wdouble-promotion` and
`-Werror=float-equal` flags since there are issues within the GTest
headers.

For compatibility with g++-7, `-Wextra-semi` was removed.

I also had to remove some of the following flags like:
`-fstack-protector-strong -fstack-clash-protection`.
These flags cause errors in Clang v10 (still run in CI), and to be
honest, with stuff like address sanitizer, we probably don't need them.

[1]: https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html
[2]: https://github.com/friendlyanon/cmake-init/blob/aa42211c79ab5117b05a2d9f795427f078a0a3d5/cmake-init/templates/common/CMakePresets.json#L88
@aloisklink aloisklink marked this pull request as ready for review September 1, 2023 17:24
@aloisklink
Copy link
Contributor Author

I've git rebase-d onto 683883b on the main branch, since all the PRs have been merged and all the GCC/Clang compiler warnings have been fixed!

@skypjack
Copy link
Owner

skypjack commented Sep 5, 2023

Before merging, is this the last PR in the row or is there something else to address?
Doing this work on warnings over the holidays made it a bit tricky to follow, sorry. 🙂

@aloisklink
Copy link
Contributor Author

Before merging, is this the last PR in the row or is there something else to address?

This is the last PR in the row. Every other PR I made was about fixing warnings. This PR is just here to make big red cross in GitHub Actions if any new code is added that has warnings in it.

I considered doing something similar for Windows/MacOS, but I don't have easy access to a Windows/MacOS machine, so I guess that can wait until a Windows/MacOS user complains about warnings.

I do also want to make a PR fixing the https://skypjack.github.io/uvw/ website (currently a lot of links don't work, since they point to https://skypjack.github.io/uvw/classuvw_1_1emitter.html, but the file is actually called https://skypjack.github.io/uvw/classuvw_1_1Emitter.html (e.g. with a capital E not e)), but that's completely unrelated to this compiler warnings series of PRs.

Doing this work on warnings over the holidays made it a bit tricky to follow, sorry. 🙂

And no worries :) Thanks for reviewing stuff on your holiday! To be honest, I wouldn't have minded waiting. After all, since these PRs were mostly just refactoring PRs, there's probably no point in making a uvw release just for this 😄

@skypjack skypjack merged commit b32fd63 into skypjack:main Sep 6, 2023
48 checks passed
@aloisklink aloisklink deleted the ci/test-for-compiler-warnings branch September 6, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants