PengZheng commented on code in PR #848: URL: https://github.com/apache/celix/pull/848#discussion_r4001820599
########## documents/building/README.md: ########## @@ -123,6 +123,19 @@ To see a complete overview of the available build options in the recipe you can conan inspect . | grep build_ ``` +#### CMake 4 and Jansson + +When building on a system with CMake 4 or higher, the following entry is needed in your Conan host +profile (for example, `debug`): + +```ini +[buildenv] +jansson/2.14:CMAKE_POLICY_VERSION_MINIMUM=3.5 +``` + +This is needed because Jansson 2.14 is based on an older version of CMake, and this entry sets the +minimum CMake policy version only in Jansson's build environment. Review Comment: This should be and has already been fixed by jansson's reciple: https://github.com/conan-io/conan-center-index/blob/7fc80e7d9beb8570ef7d3b247a26d2f5905648b0/recipes/jansson/all/conanfile.py#L60-L61 Thus, it is not needed any more. ########## libs/framework/gtest/src/ScheduledEventTestSuite.cc: ########## @@ -720,8 +720,10 @@ TEST_F(ScheduledEventTestSuite, ScheduledEventTimeoutLogTest) { output = stderr; } fprintf(output, "%s: ", celix_logLevel_toString(level)); - vfprintf(output, format, args); - fprintf(output, "\n"); + if (format) { + vfprintf(output, format, args); + fprintf(output, "\n"); + } }; celix_framework_setLogCallback(fw->getCFramework(), &logCount, logCallback); Review Comment: I don't know whether gcc attribute annotation will work in this case, but guess it will. ########## bundles/logging/log_admin/gtest/src/LogAdminTestSuite.cc: ########## @@ -290,9 +290,10 @@ static void logSinkFunction(void *handle, celix_log_level_e level, long logServi EXPECT_STREQ("test::Log1", logServiceName); } - vfprintf(stdout, format, formatArgs); - - fprintf(stdout, "\n"); + if (format) { Review Comment: This also reminds me that we should annotate `logSink.sinkLog` with `__attribute__((format))`. ########## bundles/logging/log_admin/gtest/src/LogAdminTestSuite.cc: ########## @@ -290,9 +290,10 @@ static void logSinkFunction(void *handle, celix_log_level_e level, long logServi EXPECT_STREQ("test::Log1", logServiceName); } - vfprintf(stdout, format, formatArgs); - - fprintf(stdout, "\n"); + if (format) { Review Comment: How about using __attribute__((nonnull)), which is supported by both gcc and clang, to give the compiler a clue of the nullability of the parameters? In theory, it should eliminate the `if(format)` check. Moreover, it also enable clang static analyzer(CSA)'s nullability checker to work. I happened to work on CSA recently. ########## .github/workflows/coverage.yml: ########## @@ -72,7 +72,7 @@ jobs: source generators/conanrun.sh make coverage source generators/deactivate_conanrun.sh - lcx="lcov --output-file=coverage.info " && for i in `find . -name "*.info.cleaned"`; do lcx+=" --add-tracefile=$i"; done && $lcx + lcx="lcov --output-file=coverage.info --ignore-errors inconsistent,mismatch" && for i in `find . -name "*.info.cleaned"`; do lcx+=" --add-tracefile=$i"; done && $lcx Review Comment: We'd better fix it in cmake/celix_project/CodeCoverage.cmake as in https://github.com/apache/celix/pull/842/ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
