update bot tooling #137
update bot tooling #137
Conversation
This is required in order to build with GCC 13 with warnings-as-errors
Also: Linux: gcc-13, cmake-3.31.2 Windows: python 3.12 Bug: crbug.com/383538610
CMakeLists.txt
Outdated
| @@ -301,6 +301,9 @@ if(CPPDAP_BUILD_TESTS) | |||
| list(APPEND DAP_TEST_LIST | |||
| ${CPPDAP_GOOGLETEST_DIR}/googletest/src/gtest-all.cc | |||
| ) | |||
| set_property(SOURCE | |||
| ${CPPDAP_GOOGLETEST_DIR}/googletest/src/gtest-all.cc | |||
| APPEND PROPERTY COMPILE_OPTIONS -w) | |||
There was a problem hiding this comment.
Incorrect indentation. But more importantly, I don't think this will work on MSVC builds. I think the problem here is the fact that we're adding gtest-all.cc to cppdap's SOURCE var, so it inherits the compilation flags from it. We can see that when CPPDAP_USE_EXTERNAL_GTEST_PACKAGE is defined, we use find_package, which brings in the GTest::gtest target. I think in the else() case above, we could use add_subdirectory to add ${CPPDAP_GOOGLETEST_DIR}/googletest, which would allow us to just link against the same target below. This would no longer build with the same compilation/warning flags.
There was a problem hiding this comment.
ok, looking at that.
There was a problem hiding this comment.
MSVC allows -w to disable warnings:
- Options are specified by either a forward slash (/) or a dash (-). https://learn.microsoft.com/en-us/cpp/build/reference/compiler-command-line-syntax?view=msvc-170#:~:text=Options%20are%20specified%20by%20either%20a%20forward%20slash%20(/)%20or%20a%20dash%20(%2D).
/wdisables warnings: https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=msvc-170
|
I've updated the build procedure to use add_subdirectory to include gtest code rather than reaching into its source tree. |
CMakeLists.txt
Outdated
| # Disable all warnings in googletest code. | ||
| target_compile_options(gtest PRIVATE -w) | ||
| set(CPPDAP_GTEST_TARGET gtest) | ||
| set(DAP_TEST_INCLUDE_DIR ${CPPDAP_GOOGLETEST_DIR}/googlemock/include/) |
There was a problem hiding this comment.
Turns out that this isn't necessary. You need to also link against the gmock target. See, GTest::gtest is an alias for those two targets, but sadly, it doesn't get added with add_subdirectory.
Also, you need to enable gtest_force_shared_crt on Windows, anyway, otherwise you get linker errors about mismatched CRTs. Finally, when setting sub-project variables in CMake, you want to set them in the CACHE, because these only get set once during CMake generation, but any build steps that rely on these values will not work properly if they're not cached (CMake sucks).
Take a look at my changes here: main...amaiorano:cppdap:main#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR299
There was a problem hiding this comment.
I pulled in your commit, thanks!
I still need to add the -w compile option though. Am trying that.
GCC 13 issues a maybe-uninitialized warning on googletest code. Use -w to disable it. Note: MSVC understands -w
Update to:
Turn off warnings in googletest source. There are some warnings in it that cause compilation failure when using warnings-as-errors