From 92e042add14cac3d9d1a6cdbf7a72eaf0749b33c Mon Sep 17 00:00:00 2001 From: Lefucjusz Date: Wed, 4 Sep 2024 15:41:14 +0200 Subject: [PATCH] [BH-2065] Fix race condition between CMake targets * Fix for the issue that resulted in race condition between target creating and populating databases and the one copying created databases to required directories. The issue was caused by lack of dependency between those targets, what allowed CMake engine to parallelize them. As a result, databases were copied before their creation has been finished, what resulted in weird discrepancies in databases content and occasional rsync errors. * Removed unused multicomp_install function. * Refactored DB tests readContent helper. --- CMakeLists.txt | 2 +- cmake/modules/Assets.cmake | 4 ++-- cmake/modules/DownloadAsset.cmake | 2 -- cmake/modules/Utils.cmake | 14 ----------- image/CMakeLists.txt | 1 + module-db/tests/Helpers.cpp | 37 +++++++++++++----------------- products/BellHybrid/CMakeLists.txt | 2 -- products/PurePhone/CMakeLists.txt | 2 -- test/CMakeLists.txt | 2 +- 9 files changed, 21 insertions(+), 45 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e095cec255ad68c891da47d6e90e65173a5b47c0..fc53246785703b718f1404c4a1567ecb102cc457 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,7 @@ endif() if (${ENABLE_TESTS}) enable_testing() - add_custom_target(check ${CMAKE_CTEST_COMMAND} -V DEPENDS create_test_product_databases) + add_custom_target(check ${CMAKE_CTEST_COMMAND} -V) add_subdirectory(test) include(PureCoverage) endif () diff --git a/cmake/modules/Assets.cmake b/cmake/modules/Assets.cmake index f66aa2b4135018e22e021f4e77610af4daf5521d..c5e31ce135b35e774f65077e6e9e779469f5f031 100644 --- a/cmake/modules/Assets.cmake +++ b/cmake/modules/Assets.cmake @@ -28,8 +28,8 @@ function(add_assets_target) ${_ASSETS_SYSTEM_DEST_DIR} # Create 'golden copy' of DBs - COMMAND mkdir -p ${_ASSETS_SYSTEM_DEST_DIR}/db/factory - COMMAND rsync -qlptgoDu + # 'v' flag intentionally left for debugging purposes, can be removed if you're sure it's no longer needed + COMMAND rsync -vlptgoDu ${_ASSETS_SYSTEM_DEST_DIR}/db/* ${_ASSETS_SYSTEM_DEST_DIR}/db/factory diff --git a/cmake/modules/DownloadAsset.cmake b/cmake/modules/DownloadAsset.cmake index 9fa61168b53eaff227674dda0985fab01a3554af..57790246ef7fb0903dfe2bb983097193fe7688cb 100644 --- a/cmake/modules/DownloadAsset.cmake +++ b/cmake/modules/DownloadAsset.cmake @@ -17,8 +17,6 @@ function(download_asset_release asset_name_in asset_name_out asset_repo asset_ve ) add_custom_target(${asset_name_out}-target DEPENDS ${asset_repo}) - -# multicomp_install(PROGRAMS ${CMAKE_BINARY_DIR}/${asset_repo} DESTINATION "./" COMPONENTS Standalone Update) endfunction() function(download_asset_release_json diff --git a/cmake/modules/Utils.cmake b/cmake/modules/Utils.cmake index 198d5abbb8f4550a3b75aa4dd9cb3028d845084d..28ad3458e59268be343a0104d8e4b3dea3affc2b 100644 --- a/cmake/modules/Utils.cmake +++ b/cmake/modules/Utils.cmake @@ -1,17 +1,3 @@ -# An equivalent of install() which allows to declare multiple components using -# a custom 'COMPONENTS' clause. This clause must be the last on the -# argument list. The original 'COMPONENT' from install() clause must not appear -# on the argument list. -function(multicomp_install) - list(FIND ARGN "COMPONENTS" CLAUSE_INDEX) - list(SUBLIST ARGN 0 ${CLAUSE_INDEX} INSTALL_ARGN) - math(EXPR COMPS_INDEX "${CLAUSE_INDEX}+1") - list(SUBLIST ARGN ${COMPS_INDEX} ${ARGC} COMPONENTS) - foreach(COMP IN LISTS COMPONENTS) - install(${INSTALL_ARGN} COMPONENT ${COMP}) - endforeach() -endfunction() - macro(print_var VARIABLE) if(${VARIABLE}) message(STATUS "${Green}${VARIABLE}: '${Orange}${${VARIABLE}}${ColourReset}'") diff --git a/image/CMakeLists.txt b/image/CMakeLists.txt index 77b33e293ba3fd3eb1c4b06840f13c97638c7059..8ae0a4ba10fb3873c7865a97bed01a224c081a57 100644 --- a/image/CMakeLists.txt +++ b/image/CMakeLists.txt @@ -5,6 +5,7 @@ set(SYSTEM_DEST_DIR ${SYSROOT_PATH}/system_a) set(USER_DEST_DIR ${SYSROOT_PATH}/user) set(ASSETS_DEPENDENCIES "json-common-target") +list(APPEND ASSETS_DEPENDENCIES "create_product_databases") if (${ASSETS_TYPE} STREQUAL "Proprietary") list(APPEND ASSETS_DEPENDENCIES "json-proprietary-target") diff --git a/module-db/tests/Helpers.cpp b/module-db/tests/Helpers.cpp index 9e0f3cba17a79c74a53f820e6542540cd823a87d..a7cd29b6fbf5921748278371bfaee0eedf310db7 100644 --- a/module-db/tests/Helpers.cpp +++ b/module-db/tests/Helpers.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2024, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include "Helpers.hpp" @@ -8,39 +8,34 @@ #include #include #include +#include namespace { - std::string readContent(const std::string &filename) noexcept + std::string readContent(const std::string &filename) { - auto getFileSize = [](FILE *fd) -> std::size_t { - std::fseek(fd, 0, SEEK_END); - const auto size = std::ftell(fd); - std::rewind(fd); - return size; - }; - - std::vector fileContent; - if (const auto fp = std::fopen(filename.c_str(), "r")) { - const auto fileSize = getFileSize(fp); - - fileContent.reserve(fileSize + 1); - std::fread(fileContent.data(), 1, fileSize, fp); - std::fclose(fp); - fileContent[fileSize] = '\0'; - return fileContent.data(); + std::ifstream file{filename}; + if (!file.is_open()) { + return {}; } - return {}; + + const auto fileSize = std::filesystem::file_size(filename); + auto fileContent = std::make_unique(fileSize + 1); + + file.read(fileContent.get(), fileSize); + fileContent[fileSize] = '\0'; + + return std::string{fileContent.get()}; } std::vector readCommands(const std::filesystem::path &filePath) { - const auto fileContent = readContent(filePath.c_str()); + const auto &fileContent = readContent(filePath.c_str()); std::string currentStatement{}; std::vector statements{}; std::string line{}; - for (const auto &c : fileContent) { + for (const auto c : fileContent) { if (c != '\n') { line += c; } diff --git a/products/BellHybrid/CMakeLists.txt b/products/BellHybrid/CMakeLists.txt index 8f90b42dcb871e16bd3e9d7a81f1f4f4005c899f..224413a19da33084083c62354f4f1ccfbf052318 100644 --- a/products/BellHybrid/CMakeLists.txt +++ b/products/BellHybrid/CMakeLists.txt @@ -105,7 +105,6 @@ if (${PROJECT_TARGET} STREQUAL "TARGET_RT1051") SYSROOT sysroot DEPENDS install_scripts - create_product_databases create_user_directories assets recovery.bin-target @@ -121,7 +120,6 @@ else() LUTS "" DEPENDS install_scripts - create_product_databases remove_var_directory create_user_directories assets diff --git a/products/PurePhone/CMakeLists.txt b/products/PurePhone/CMakeLists.txt index bd9c4387a529ba4cf4caa08330f13340c198a41f..948c235c150317a847c9b0bca9cc4aac0cabff7c 100644 --- a/products/PurePhone/CMakeLists.txt +++ b/products/PurePhone/CMakeLists.txt @@ -121,7 +121,6 @@ if (${PROJECT_TARGET} STREQUAL "TARGET_RT1051") SYSROOT sysroot DEPENDS install_scripts - create_product_databases create_user_directories assets recovery.bin-target @@ -136,7 +135,6 @@ else() SYSROOT sysroot DEPENDS install_scripts - create_product_databases remove_var_directory create_user_directories assets diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index cbb7726512740d259de6a93044ee93749b52c725..6a2d32ef540ee79c0cd26399d63397f7a3f32d9b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -46,7 +46,7 @@ download_asset_json(json-test-proprietary-target ${MUDITA_CACHE_DIR} ) -set(ASSETS_DEPENDENCIES "") +set(ASSETS_DEPENDENCIES "create_test_product_databases") if (${ASSETS_TYPE} STREQUAL "Proprietary") list(APPEND ASSETS_DEPENDENCIES "json-test-proprietary-target")