From 41ba374183647dac40cca8dce93708ce9814465c Mon Sep 17 00:00:00 2001 From: Peguen <73380451+Peguen@users.noreply.github.com> Date: Tue, 5 May 2026 11:39:02 +0200 Subject: [PATCH 1/3] [core] Refactor `ecal_path_processing.cpp` to C++17 `std::filesystem` (#2608) --- ecal/core/src/config/ecal_path_processing.cpp | 190 ++++++++---------- .../config_test/src/path_processing_test.cpp | 106 ++++++++++ 2 files changed, 189 insertions(+), 107 deletions(-) diff --git a/ecal/core/src/config/ecal_path_processing.cpp b/ecal/core/src/config/ecal_path_processing.cpp index ef1ec9b833..6bc3b49480 100644 --- a/ecal/core/src/config/ecal_path_processing.cpp +++ b/ecal/core/src/config/ecal_path_processing.cpp @@ -24,50 +24,61 @@ #include "ecal_path_processing.h" #include "ecal_def.h" -#include "ecal_utils/ecal_utils.h" -#include "ecal_utils/filesystem.h" #include "ecal/config.h" + #include "ecal/util.h" #include "util/getenvvar.h" +#include +#include #include +#include +#include +#include #include -// for cwd +// OS-specific includes for path/temp/registry APIs #ifdef ECAL_OS_WINDOWS #include #include #endif #ifdef ECAL_OS_LINUX - #include - #include #include - #include #endif namespace { - // get the path separator from the current OS (win: "\\", unix: "/") - const std::string path_separator(1, EcalUtils::Filesystem::NativeSeparator()); + // Converts a UTF-8 encoded std::string to a std::filesystem::path. + // std::filesystem::u8path() is the standard C++17 way to do this correctly + // on all platforms, including Windows where path(std::string) uses the ANSI + // code page rather than UTF-8. + std::filesystem::path utf8ToPath(const std::string& utf8_str_) + { + return std::filesystem::u8path(utf8_str_); + } + + // Converts a std::filesystem::path to a UTF-8 encoded std::string. + std::string pathToUtf8(const std::filesystem::path& p_) + { + return p_.u8string(); + } - // returns empty if str1_ is empty. otherwise returns str1_ + path_separator + str2_ + // returns empty if str1_ is empty. otherwise returns str1_ / str2_ (native path separator) std::string buildPath(const std::string& str1_, const std::string& str2_) { - if (str1_.empty()) return ""; - return EcalUtils::String::Join(path_separator, std::vector{str1_, str2_}); + if (str1_.empty()) return {}; + return pathToUtf8(utf8ToPath(str1_) / utf8ToPath(str2_)); } // checks if the specified file is a proper file bool isValidFile(const std::string& full_file_path_) { - const EcalUtils::Filesystem::FileStatus file_status(full_file_path_, EcalUtils::Filesystem::Current); - return file_status.IsOk() && (file_status.GetType() == EcalUtils::Filesystem::Type::RegularFile); + return std::filesystem::is_regular_file(utf8ToPath(full_file_path_)); } #ifdef ECAL_OS_WINDOWS std::string getKnownFolderPath(REFKNOWNFOLDERID id_) { - std::string return_path; PWSTR path_tmp = nullptr; // Retrieve the known folder path: users local app data @@ -75,23 +86,13 @@ namespace if (ret != S_OK) { - if (path_tmp != nullptr) - // Free the memory allocated by SHGetKnownFolderPath - CoTaskMemFree(path_tmp); - + CoTaskMemFree(path_tmp); // safe to call with nullptr return {}; } - // Convert the wide-character string to a multi-byte string - // For supporting full Unicode compatibility - int size_needed = WideCharToMultiByte(CP_UTF8, 0, path_tmp, -1, nullptr, 0, nullptr, nullptr); - if (size_needed > 0) - { - // Exclude the null terminator from the size - return_path.resize(size_needed - 1); - WideCharToMultiByte(CP_UTF8, 0, path_tmp, -1, &return_path[0], size_needed, nullptr, nullptr); - } - + // Construct a path from the wide string and convert to UTF-8. + std::string return_path = std::filesystem::path(path_tmp).u8string(); + // Free the memory allocated by SHGetKnownFolderPath CoTaskMemFree(path_tmp); @@ -122,54 +123,25 @@ namespace // returns temp dir, e.g. /tmp in linux or C:\Users\username\AppData\Local\Temp in windows // never returns an empty string, if there is no valid temp dir found, fallback /ecal_tmp is returned - std::string getTempDir(const eCAL::Util::IDirManager& dir_manager_) + std::string getTempDir() { - #ifdef ECAL_OS_WINDOWS - - char temp_path_buffer[MAX_PATH]; - DWORD path_length = GetTempPathA(MAX_PATH, temp_path_buffer); - if (path_length > 0 && path_length < MAX_PATH) - { - return std::string(temp_path_buffer, path_length); - } - else - { - std::string appdata_path = getKnownFolderPath(FOLDERID_LocalAppData); - if (!appdata_path.empty()) - { - std::string apdata_tmp_path = buildPath(appdata_path, ECAL_FOLDER_NAME_TMP_WINDOWS); - if (dir_manager_.dirExists(apdata_tmp_path)) - { - return apdata_tmp_path; - } - } - } - - #elif defined(ECAL_OS_LINUX) - - std::string env_tmp_dir = getEnvVar(ECAL_LINUX_TMP_VAR); - if (!env_tmp_dir.empty() && dir_manager_.dirExists(env_tmp_dir)) - { - return env_tmp_dir; - } - - #endif /* ECAL_OS_LINUX */ - + std::error_code ec; + const std::filesystem::path tmp = std::filesystem::temp_directory_path(ec); + if (!ec && !tmp.empty()) + return pathToUtf8(tmp); return ECAL_FALLBACK_TMP_DIR; } - std::string eCALPlatformSpecificFolder(const std::string& path_, const std::string& linux_folder_name_ = ECAL_FOLDER_NAME_HOME_LINUX, const std::string& win_folder_name_ = ECAL_FOLDER_NAME_WINDOWS) + std::string eCALPlatformSpecificFolder(const std::string& path_, [[maybe_unused]] const std::string& linux_folder_name_ = ECAL_FOLDER_NAME_HOME_LINUX, [[maybe_unused]] const std::string& win_folder_name_ = ECAL_FOLDER_NAME_WINDOWS) { if (path_.empty()) return {}; #ifdef ECAL_OS_WINDOWS - (void)linux_folder_name_; // suppress unused warning return buildPath(path_, win_folder_name_); #elif defined(ECAL_OS_LINUX) - (void)win_folder_name_; // suppress unused warning return buildPath(path_, linux_folder_name_); #else @@ -210,13 +182,13 @@ namespace eCAL { bool DirManager::dirExists(const std::string& path_) const { - const EcalUtils::Filesystem::FileStatus status(path_, EcalUtils::Filesystem::Current); - return (status.IsOk() && (status.GetType() == EcalUtils::Filesystem::Type::Dir)); + return std::filesystem::is_directory(utf8ToPath(path_)); } bool DirManager::createDir(const std::string& path_) const { - return EcalUtils::Filesystem::MkDir(path_); + std::error_code ec; + return std::filesystem::create_directory(utf8ToPath(path_), ec); } bool DirManager::dirExistsOrCreate(const std::string& path_) const @@ -250,7 +222,7 @@ namespace eCAL // We should have encountered a valid path if (it != paths_.end()) - return (*it); + return *it; // If valid path is not encountered, defaults should be used return {}; @@ -258,26 +230,29 @@ namespace eCAL bool DirManager::canWriteToDirectory(const std::string& path_) const { - const std::string testFilePath = path_ + "/test_file.txt"; - std::ofstream testFile(testFilePath); - - if (testFile) + // Attempt-the-write is the most reliable cross-platform check: + // - std::filesystem::perms doesn't model Windows ACLs + // - access(W_OK) is POSIX-only + // Use a thread-id-qualified name to avoid collisions under concurrent calls. + const std::string test_file_name = "ecal_write_test_" + std::to_string(std::hash{}(std::this_thread::get_id())) + ".tmp"; + const std::filesystem::path test_file_path = utf8ToPath(path_) / utf8ToPath(test_file_name); + std::ofstream test_file(test_file_path); + + if (test_file) { - testFile.close(); - std::remove(testFilePath.c_str()); + test_file.close(); + std::error_code ec; + std::filesystem::remove(test_file_path, ec); return true; - } - else - { - return false; } + + return false; } // returns the directory path of the specified file std::string DirManager::getDirectoryPath(const std::string& file_path_) const { - const size_t pos = file_path_.find_last_of("/\\"); - return (std::string::npos == pos) ? "" : file_path_.substr(0, pos); + return pathToUtf8(utf8ToPath(file_path_).parent_path()); } // return a unique temporary folder name @@ -285,32 +260,41 @@ namespace eCAL // returns an empty string if the folder could not be created std::string DirProvider::uniqueTmpDir(const eCAL::Util::IDirManager& dir_manager_) const { - const std::string tmp_dir = getTempDir(dir_manager_); + const std::string tmp_dir = getTempDir(); + + // Ensure the base tmp directory exists (e.g. when falling back to /ecal_tmp) + if (!dir_manager_.dirExistsOrCreate(tmp_dir)) + return {}; + #ifdef ECAL_OS_WINDOWS - char unique_path[MAX_PATH]; - if (!GetTempFileNameA(tmp_dir.c_str(), "ecal", 0, unique_path) != 0) + std::wstring wide_tmp_dir = utf8ToPath(tmp_dir).wstring(); + wchar_t unique_path_buf[MAX_PATH]; + if (GetTempFileNameW(wide_tmp_dir.c_str(), L"ecal", 0, unique_path_buf) == 0) { // failed to generate the path return {}; } - // delete the temporary file and use the name as a directory - DeleteFileA(unique_path); - if (!CreateDirectoryA(unique_path, nullptr)) { - return {}; - } + // GetTempFileNameW creates a file to reserve the name; remove it so we + // can create a directory with the same name instead. + const std::filesystem::path unique_path(unique_path_buf); + std::error_code ec; + std::filesystem::remove(unique_path, ec); + if (ec) return {}; - return std::string(unique_path); + std::filesystem::create_directory(unique_path, ec); + if (ec) return {}; + + return pathToUtf8(unique_path); #elif defined(ECAL_OS_LINUX) std::string path_template = buildPath(tmp_dir, "ecal-XXXXXX"); // 'X's will be replaced - char* dir = mkdtemp(&path_template[0]); + char* dir = mkdtemp(path_template.data()); - if (dir == nullptr) { + if (dir == nullptr) return {}; - } return std::string(dir); @@ -328,27 +312,18 @@ namespace eCAL std::string DirProvider::eCALLocalUserDir() const { - const std::string userspace_path = getLocalUserPath(); - - if (!userspace_path.empty()) - { - return eCALPlatformSpecificFolder(userspace_path); - } - - return {}; + return eCALPlatformSpecificFolder(getLocalUserPath()); } - std::string DirProvider::eCALDataSystemDir(const Util::IDirManager& dir_manager_) const + std::string DirProvider::eCALDataSystemDir([[maybe_unused]] const Util::IDirManager& dir_manager_) const { std::string system_dir; #ifdef ECAL_OS_WINDOWS - (void)dir_manager_; // suppress unused warning system_dir = getKnownFolderPath(FOLDERID_ProgramData); #elif defined(ECAL_OS_LINUX) - // TODO PG: Check if we really want to give that back here if (dir_manager_.dirExists(ECAL_LINUX_SYSTEM_PATH)) system_dir = ECAL_LINUX_SYSTEM_PATH; @@ -369,7 +344,7 @@ namespace eCAL { std::string GeteCALLogDirImpl(const Util::IDirProvider& dir_provider_ /* = Util::DirProvider() */, const Util::IDirManager& dir_manager_ /* = Util::DirManager() */, const eCAL::Configuration& config_ /* = eCAL::GetConfiguration() */) { - const std::string config_file_dir = dir_manager_.getDirectoryPath(eCAL::GetConfiguration().GetConfigurationFilePath()); + const std::string config_file_dir = dir_manager_.getDirectoryPath(config_.GetConfigurationFilePath()); const std::string ecal_data_env_dir = dir_provider_.eCALEnvVar(ECAL_DATA_VAR); const std::vector log_paths = { @@ -384,9 +359,7 @@ namespace eCAL for (const auto& path : log_paths) { if (!path.empty() && dir_manager_.dirExists(path) && dir_manager_.canWriteToDirectory(path)) - { return path; - } } // if no path is available, we create temp directories for logging @@ -396,12 +369,15 @@ namespace eCAL std::string checkForValidConfigFilePath(const std::string& config_file_, const Util::DirProvider& dir_provider_ /* = Util::DirProvider() */, const Util::DirManager& dir_manager_ /* = Util::DirManager() */) { - const std::string cwd_directory_path = EcalUtils::Filesystem::CurrentWorkingDir(); + std::error_code ec; + const std::string cwd_directory_path = pathToUtf8(std::filesystem::current_path(ec)); std::vector ecal_default_paths = getEcalDefaultPaths(dir_provider_, dir_manager_); // insert cwd on 2nd position, so that ECAL_DATA dir has precedence - ecal_default_paths.insert(ecal_default_paths.begin() + 1, cwd_directory_path); + // skip if CWD could not be determined (e.g. directory was deleted) + if (!ec) + ecal_default_paths.insert(ecal_default_paths.begin() + 1, cwd_directory_path); const std::string found_path = dir_manager_.findFileInPaths(ecal_default_paths, config_file_); diff --git a/ecal/tests/cpp/config_test/src/path_processing_test.cpp b/ecal/tests/cpp/config_test/src/path_processing_test.cpp index ec6ff300e5..495c7299c7 100644 --- a/ecal/tests/cpp/config_test/src/path_processing_test.cpp +++ b/ecal/tests/cpp/config_test/src/path_processing_test.cpp @@ -28,6 +28,13 @@ #include #include +#include +#include + +#ifdef ECAL_OS_WINDOWS + #include +#endif + #include #include @@ -392,3 +399,102 @@ TEST(core_cpp_path_processing /*unused*/, ecal_log_order_test /*unused*/) EXPECT_EQ(eCAL::Config::GeteCALLogDirImpl(mock_dir_provider, mock_dir_manager, config), ecal_yaml_dir); EXPECT_EQ(eCAL::Config::GeteCALLogDirImpl(mock_dir_provider, mock_dir_manager, config), unique_tmp_dir); } + +// ============================================================================ +// DirManager concrete implementation tests +// ============================================================================ + +// Verify that canWriteToDirectory returns true for a real writable directory and +// false for a non-existent one. +// Note: the specific bug of std::filesystem::remove throwing on failure (vs. the +// error_code overload) cannot be exercised in a normal unit test because it only +// manifests when the OS refuses to delete the test file – an unusual condition +// that cannot be reliably engineered here. The fix (using the error_code overload) +// is validated by code review rather than by this test. +TEST(core_cpp_dir_manager /*unused*/, can_write_to_directory /*unused*/) +{ + const eCAL::Util::DirManager dir_manager; + const std::string temp_dir = std::filesystem::temp_directory_path().u8string(); + + EXPECT_TRUE(dir_manager.canWriteToDirectory(temp_dir)); + EXPECT_FALSE(dir_manager.canWriteToDirectory("ecal_nonexistent_dir_test_67890")); +} + +// Verify that uniqueTmpDir returns a non-empty, existing directory and can be +// cleaned up without issues. +// Note: getTempDir() is a free function that reads the real OS temp directory and +// is not injectable, so the fallback path (ECAL_FALLBACK_TMP_DIR) cannot be +// triggered in a unit test without manipulating environment variables. +TEST(core_cpp_dir_provider /*unused*/, unique_tmp_dir_returns_valid_dir /*unused*/) +{ + const eCAL::Util::DirManager dir_manager; + const eCAL::Util::DirProvider dir_provider; + + const std::string unique_dir = dir_provider.uniqueTmpDir(dir_manager); + EXPECT_FALSE(unique_dir.empty()); + + if (!unique_dir.empty()) + { + EXPECT_TRUE(dir_manager.dirExists(unique_dir)); + + std::error_code ec; + std::filesystem::remove_all(std::filesystem::u8path(unique_dir), ec); + EXPECT_FALSE(ec) << "Cleanup of unique tmp dir failed: " << ec.message(); + } +} + +// Verify that uniqueTmpDir returns an empty string when the base tmp directory +// cannot be accessed or created (new guard: dirExistsOrCreate fails). +TEST(core_cpp_dir_provider /*unused*/, unique_tmp_dir_returns_empty_when_dir_not_creatable /*unused*/) +{ + NiceMock mock_dir_manager; + const eCAL::Util::DirProvider dir_provider; + + ON_CALL(mock_dir_manager, dirExistsOrCreate(testing::_)).WillByDefault(testing::Return(false)); + + EXPECT_TRUE(dir_provider.uniqueTmpDir(mock_dir_manager).empty()); +} + +#ifdef ECAL_OS_WINDOWS +// On Windows, DirManager methods receive UTF-8 strings. Before the fix, +// std::filesystem::path(std::string) used the ANSI code page, which caused +// garbled or failed operations for non-ASCII paths. These tests verify the +// correct UTF-8 round-trip behaviour after the fix. + +// Tests that getDirectoryPath correctly round-trips a UTF-8 path containing +// characters outside the ANSI code page (Greek letters used as a proxy). +TEST(core_cpp_dir_manager /*unused*/, get_directory_path_unicode_utf8 /*unused*/) +{ + // UTF-8 encoding of "αβγ" (U+03B1 U+03B2 U+03B3) + const std::string unicode_segment = "\xce\xb1\xce\xb2\xce\xb3"; + + const std::string input_path = "C:\\unicode_test\\" + unicode_segment + "\\ecal.yaml"; + const std::string expected_dir = "C:\\unicode_test\\" + unicode_segment; + + const eCAL::Util::DirManager dir_manager; + EXPECT_EQ(dir_manager.getDirectoryPath(input_path), expected_dir); +} + +// Tests that canWriteToDirectory works correctly when the target directory path +// contains Unicode characters (non-ANSI). +TEST(core_cpp_dir_manager /*unused*/, can_write_to_directory_unicode_path /*unused*/) +{ + // Use a wide path to create the directory reliably, then pass its UTF-8 + // representation to the function under test. + const std::wstring unicode_dir_name = L"ecal_unicode_write_test_\u03B1\u03B2\u03B3"; + const std::filesystem::path unicode_dir_path = + std::filesystem::temp_directory_path() / unicode_dir_name; + + std::error_code ec; + std::filesystem::create_directory(unicode_dir_path, ec); + ASSERT_FALSE(ec) << "Failed to create Unicode test directory: " << ec.message(); + + const std::string unicode_dir_utf8 = + EcalUtils::StrConvert::WideToUtf8(unicode_dir_path.wstring()); + + const eCAL::Util::DirManager dir_manager; + EXPECT_TRUE(dir_manager.canWriteToDirectory(unicode_dir_utf8)); + + std::filesystem::remove_all(unicode_dir_path, ec); +} +#endif /* ECAL_OS_WINDOWS */ From 454622767ad4f8edc6881ac2802afab67884ab66 Mon Sep 17 00:00:00 2001 From: Peter Guenschmann <73380451+Peguen@users.noreply.github.com> Date: Thu, 7 May 2026 09:03:10 +0200 Subject: [PATCH 2/3] Added c++ 17 standard to config generation. --- ecal/core/cfg/CMakeLists.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ecal/core/cfg/CMakeLists.txt b/ecal/core/cfg/CMakeLists.txt index 3a14094db3..e7334f29b1 100644 --- a/ecal/core/cfg/CMakeLists.txt +++ b/ecal/core/cfg/CMakeLists.txt @@ -63,6 +63,8 @@ target_include_directories(${PROJECT_NAME} PRIVATE ${ECAL_CORE_PROJECT_ROOT}/core/include ) +target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17) + target_link_libraries(${PROJECT_NAME} PRIVATE $<$,$>>:dl> From c9f4961cf834f43345fed4ce70564819ddf79ee2 Mon Sep 17 00:00:00 2001 From: Peter Guenschmann <73380451+Peguen@users.noreply.github.com> Date: Thu, 7 May 2026 09:13:05 +0200 Subject: [PATCH 3/3] Added missing linux header for pathprocessing. --- ecal/core/src/config/ecal_path_processing.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/ecal/core/src/config/ecal_path_processing.cpp b/ecal/core/src/config/ecal_path_processing.cpp index 6bc3b49480..1e43096c6e 100644 --- a/ecal/core/src/config/ecal_path_processing.cpp +++ b/ecal/core/src/config/ecal_path_processing.cpp @@ -44,6 +44,7 @@ #endif #ifdef ECAL_OS_LINUX #include + #include #endif namespace