From fad5ed6b5e5dc88814eb0cbb09ded92c7d515807 Mon Sep 17 00:00:00 2001 From: 4ment Date: Sun, 20 Sep 2026 06:41:16 +1000 Subject: [PATCH 01/12] Enhance Windows compatibility in app.cpp and string_metrics.cpp with proper handling of console dimensions and bit manipulation intrinsics. --- src/cpplink/app.cpp | 25 ++++++++++++++++++++++++- src/cpplink/string_metrics.cpp | 27 +++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/cpplink/app.cpp b/src/cpplink/app.cpp index 23578de..bb0a51d 100644 --- a/src/cpplink/app.cpp +++ b/src/cpplink/app.cpp @@ -14,8 +14,22 @@ #include #include + +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX // windows.h's min/max macros would break std::min below. +#endif +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#include + +#include +#include +#else #include #include +#endif #include "cpplink/blocking.hpp" #include "cpplink/cluster.hpp" @@ -910,10 +924,19 @@ int RunExplain(const std::vector& args, std::ostream& out, // string stream -- it is printed as lines. The stream is compared to the process's // own stderr because an ostream carries no descriptor to ask. unsigned TerminalColumns(const std::ostream& stream) { - if (&stream != &std::cerr || isatty(STDERR_FILENO) == 0) return 0; + if (&stream != &std::cerr) return 0; +#ifdef _WIN32 + if (_isatty(_fileno(stderr)) == 0) return 0; + CONSOLE_SCREEN_BUFFER_INFO info{}; + if (GetConsoleScreenBufferInfo(GetStdHandle(STD_ERROR_HANDLE), &info) == 0) return 80; + const int columns = info.srWindow.Right - info.srWindow.Left + 1; + return columns > 0 ? static_cast(columns) : 80; +#else + if (isatty(STDERR_FILENO) == 0) return 0; winsize size{}; if (ioctl(STDERR_FILENO, TIOCGWINSZ, &size) != 0 || size.ws_col == 0) return 80; return size.ws_col; +#endif } int RunExplainBlocking(const std::vector& args, std::ostream& out, diff --git a/src/cpplink/string_metrics.cpp b/src/cpplink/string_metrics.cpp index f7cc2f5..8ff1436 100644 --- a/src/cpplink/string_metrics.cpp +++ b/src/cpplink/string_metrics.cpp @@ -10,6 +10,10 @@ #include #include +#ifdef _MSC_VER +#include +#endif + namespace cpplink { namespace { @@ -57,7 +61,26 @@ double ApplyWinkler(std::string_view a, std::string_view b, double jaro, // The one instruction, asked for by name. `std::bitset<64>::count()` does not // lower to it here -- it leaves a call to the generic bit-iterator count in the // binary, which a profile of the scoring loop finds among the hot leaves. -int PopCount(uint64_t value) { return __builtin_popcountll(value); } +int PopCount(uint64_t value) { +#if defined(_MSC_VER) && defined(_M_ARM64) + return static_cast(_CountOneBits64(value)); +#elif defined(_MSC_VER) + return static_cast(__popcnt64(value)); +#else + return __builtin_popcountll(value); +#endif +} + +// The index of the lowest set bit; `value` must not be zero. +int TrailingZeros(uint64_t value) { +#ifdef _MSC_VER + unsigned long index = 0; // NOLINT(runtime/int): the intrinsic's own type + _BitScanForward64(&index, value); + return static_cast(index); +#else + return __builtin_ctzll(value); +#endif +} // Myers' bit-vector edit distance (1999). The DP's column of vertical deltas is // carried in two words -- vp for +1, vn for -1 -- so a whole column costs a dozen @@ -187,7 +210,7 @@ double JaroWords(std::string_view a, std::string_view b, double screen) { uint64_t left = claimed; uint64_t right = taken; while (left != 0) { - if (a[__builtin_ctzll(left)] != b[__builtin_ctzll(right)]) ++transpositions; + if (a[TrailingZeros(left)] != b[TrailingZeros(right)]) ++transpositions; left &= left - 1; right &= right - 1; } From fd004c2005f5065c0f6a423669bf68824d38b1d8 Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 05:31:54 +1000 Subject: [PATCH 02/12] Add Windows CI configuration and update documentation for MSVC support - Introduced a new CI job for Windows using MSVC with conda-forge Arrow. - Updated CMake presets for Windows builds and tests. - Enhanced documentation to reflect the new Windows preset and CI workflow. - Minor code adjustments for compatibility with Windows, including string handling. --- .github/workflows/ci.yml | 39 +++++++++++++++++++++++++++++++++- CMakePresets.json | 17 ++++++++++++++- CONTRIBUTING.md | 3 ++- docs/getting-started.md | 3 ++- python/bindings/stages.cpp | 2 +- src/cpplink/app.cpp | 38 ++++++++++++++++++--------------- src/cpplink/inspect.cpp | 1 + src/cpplink/schema.cpp | 1 + src/cpplink/string_metrics.cpp | 5 ++++- 9 files changed, 86 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1ce24b8..7729cd5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,43 @@ jobs: - name: Test run: ctest --preset ${{ matrix.preset }} -j "$(nproc 2>/dev/null || sysctl -n hw.ncpu)" + # MSVC, against the conda-forge Arrow for win-64. The Visual Studio generator is + # multi-config, so the preset names the configuration at build and test time + # rather than through CMAKE_BUILD_TYPE; conda puts every library under + # Library/, which is what the preset's prefix path points at. cmd rather than + # bash so that CONDA_PREFIX reaches CMake as a Windows path. + windows: + name: msvc on windows-latest + runs-on: windows-latest + defaults: + run: + shell: cmd /C call {0} + steps: + - uses: actions/checkout@v4 + + - uses: mamba-org/setup-micromamba@v2 + with: + environment-file: environment.yml + init-shell: cmd.exe + cache-environment: true + + - name: Configure + run: cmake --preset windows + + - name: Build + run: cmake --build --preset windows -j %NUMBER_OF_PROCESSORS% + + - name: Test + run: ctest --preset windows -j %NUMBER_OF_PROCESSORS% + + - name: Install the package + run: >- + pip install -e . --no-build-isolation + -Ccmake.define.CMAKE_PREFIX_PATH=%CONDA_PREFIX%/Library + + - name: Test the package + run: python -m pytest python/tests + python: name: python on ${{ matrix.os }} runs-on: ${{ matrix.os }} @@ -81,7 +118,7 @@ jobs: - name: clang-format run: >- clang-format --dry-run --Werror - src/main.cpp src/cpplink/* tests/*.cpp python/bindings/* + src/main.cpp src/cpplink/* tests/*.cpp tests/*.hpp python/bindings/* - name: cpplint run: cpplint --recursive src tests python/bindings diff --git a/CMakePresets.json b/CMakePresets.json index d4ccdf8..ab171e6 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -42,13 +42,22 @@ "CMAKE_BUILD_TYPE": "RelWithDebInfo", "CPPLINK_SANITIZE": "thread" } + }, + { + "name": "windows", + "displayName": "MSVC x64, with tests", + "inherits": "base", + "generator": "Visual Studio 17 2022", + "architecture": {"value": "x64", "strategy": "set"}, + "cacheVariables": {"CMAKE_PREFIX_PATH": "$env{CONDA_PREFIX}/Library"} } ], "buildPresets": [ {"name": "release", "configurePreset": "release"}, {"name": "debug", "configurePreset": "debug"}, {"name": "asan", "configurePreset": "asan"}, - {"name": "tsan", "configurePreset": "tsan"} + {"name": "tsan", "configurePreset": "tsan"}, + {"name": "windows", "configurePreset": "windows", "configuration": "Release"} ], "testPresets": [ { @@ -73,6 +82,12 @@ "inherits": "base", "configurePreset": "tsan", "environment": {"TSAN_OPTIONS": "halt_on_error=1:second_deadlock_stack=1"} + }, + { + "name": "windows", + "inherits": "base", + "configurePreset": "windows", + "configuration": "Release" } ] } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e7cfd7..cfb315b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,9 +36,10 @@ Two of them run the suite under a sanitizer, which is what checks the claim that ```sh cmake --preset asan && cmake --build --preset asan && ctest --preset asan # ASan + UBSan, about 30 s cmake --preset tsan && cmake --build --preset tsan && ctest --preset tsan # ThreadSanitizer, about 5 min +cmake --preset windows && cmake --build --preset windows && ctest --preset windows # MSVC, from an activated conda prompt ``` -Both are clean over the whole suite. +The two sanitizer presets are clean over the whole suite; `windows` is the same suite under MSVC, and CI runs it on `windows-latest`. CI ([.github/workflows/ci.yml](.github/workflows/ci.yml)) runs `release`, `asan` and `tsan` on Linux and macOS, then the Python suite and the format and lint checks below. ## The Python package diff --git a/docs/getting-started.md b/docs/getting-started.md index 5d7c5fc..3dba2ea 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -42,7 +42,8 @@ ctest --preset asan ``` `release` and `debug` are the plain builds with tests on, and `ctest --preset ` runs the suite with output on failure. -The GitHub Actions workflow runs `release`, `asan` and `tsan` on Linux and macOS, then the Python suite and the format and lint checks. +On Windows the `windows` preset builds with Visual Studio 2022 against the conda-forge Arrow, which lives under `%CONDA_PREFIX%\Library`, and the build and test presets of the same name select the `Release` configuration, since the Visual Studio generator holds every configuration in one tree. +The GitHub Actions workflow runs `release`, `asan` and `tsan` on Linux and macOS and `windows` on Windows, then the Python suite and the format and lint checks. ## The whole pipeline in seven commands diff --git a/python/bindings/stages.cpp b/python/bindings/stages.cpp index 75accb1..2458936 100644 --- a/python/bindings/stages.cpp +++ b/python/bindings/stages.cpp @@ -277,7 +277,7 @@ MergeOutcome MergePredictions(const std::string& shards, const std::string& out, options.out_path = out; options.batch_rows = batch_rows; if (format.is_none()) { - const std::string suffix = std::filesystem::path(out).extension(); + const std::string suffix = std::filesystem::path(out).extension().string(); if (suffix == ".parquet" || suffix == ".pq") options.format = MergeFormat::kParquet; } else { diff --git a/src/cpplink/app.cpp b/src/cpplink/app.cpp index bb0a51d..0be8670 100644 --- a/src/cpplink/app.cpp +++ b/src/cpplink/app.cpp @@ -15,22 +15,6 @@ #include -#ifdef _WIN32 -#ifndef NOMINMAX -#define NOMINMAX // windows.h's min/max macros would break std::min below. -#endif -#ifndef WIN32_LEAN_AND_MEAN -#define WIN32_LEAN_AND_MEAN -#endif -#include - -#include -#include -#else -#include -#include -#endif - #include "cpplink/blocking.hpp" #include "cpplink/cluster.hpp" #include "cpplink/comparison.hpp" @@ -58,6 +42,25 @@ #include "cpplink/simplify.hpp" #include "cpplink/waterfall.hpp" +// The terminal query below is the one platform-specific call. windows.h comes after +// every project header so that its macros -- and it defines several hundred, from +// ERROR to far -- rewrite nothing the project declares. +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX // windows.h's min/max macros would break std::min below. +#endif +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#include + +#include +#include +#else +#include +#include +#endif + namespace cpplink { const char* const kVersion = "0.1.0"; @@ -1648,7 +1651,8 @@ int RunMergeEdges(const std::vector& args, std::ostream& out, // The extension is what a user means by the format; --format is for a name // that does not carry one. if (!format_given) { - const std::string suffix = std::filesystem::path(options.out_path).extension(); + const std::string suffix = + std::filesystem::path(options.out_path).extension().string(); if (suffix == ".parquet" || suffix == ".pq") { options.format = MergeFormat::kParquet; } diff --git a/src/cpplink/inspect.cpp b/src/cpplink/inspect.cpp index 789ed9f..64e4d31 100644 --- a/src/cpplink/inspect.cpp +++ b/src/cpplink/inspect.cpp @@ -4,6 +4,7 @@ #include "cpplink/inspect.hpp" #include +#include #include #include #include diff --git a/src/cpplink/schema.cpp b/src/cpplink/schema.cpp index b8ce078..0a0dada 100644 --- a/src/cpplink/schema.cpp +++ b/src/cpplink/schema.cpp @@ -3,6 +3,7 @@ #include "cpplink/schema.hpp" +#include #include #include #include diff --git a/src/cpplink/string_metrics.cpp b/src/cpplink/string_metrics.cpp index 8ff1436..19b6737 100644 --- a/src/cpplink/string_metrics.cpp +++ b/src/cpplink/string_metrics.cpp @@ -60,7 +60,10 @@ double ApplyWinkler(std::string_view a, std::string_view b, double jaro, // The one instruction, asked for by name. `std::bitset<64>::count()` does not // lower to it here -- it leaves a call to the generic bit-iterator count in the -// binary, which a profile of the scoring loop finds among the hot leaves. +// binary, which a profile of the scoring loop finds among the hot leaves. MSVC's +// `__popcnt64` emits the instruction with no fallback, so an x64 build assumes +// POPCNT, which every processor Windows 11 runs on has; gcc and clang substitute +// a bit-trick sequence where the target lacks it. int PopCount(uint64_t value) { #if defined(_MSC_VER) && defined(_M_ARM64) return static_cast(_CountOneBits64(value)); From 3d57d4549a0d3f894e4f2847054e1351def48175 Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 05:48:29 +1000 Subject: [PATCH 03/12] Update Windows preset to Visual Studio 18 2026 --- CMakePresets.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakePresets.json b/CMakePresets.json index ab171e6..b0ab907 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -47,7 +47,7 @@ "name": "windows", "displayName": "MSVC x64, with tests", "inherits": "base", - "generator": "Visual Studio 17 2022", + "generator": "Visual Studio 18 2026", "architecture": {"value": "x64", "strategy": "set"}, "cacheVariables": {"CMAKE_PREFIX_PATH": "$env{CONDA_PREFIX}/Library"} } From f2b2fa7137efc06d5fb401c1f1f25d75245f4a38 Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 05:58:34 +1000 Subject: [PATCH 04/12] Implement move semantics and delete copy constructors in Dictionary class --- src/cpplink/dictionary.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cpplink/dictionary.hpp b/src/cpplink/dictionary.hpp index 0884701..610950e 100644 --- a/src/cpplink/dictionary.hpp +++ b/src/cpplink/dictionary.hpp @@ -23,6 +23,12 @@ inline constexpr uint32_t kNullId = 0xFFFFFFFFu; // by the index stay valid as the arena grows. class Dictionary { public: + Dictionary() = default; + Dictionary(const Dictionary&) = delete; + Dictionary& operator=(const Dictionary&) = delete; + Dictionary(Dictionary&&) = default; + Dictionary& operator=(Dictionary&&) = default; + uint32_t Intern(std::string_view value); std::string_view Value(uint32_t id) const { return entries_[id]; } uint32_t Size() const { return static_cast(entries_.size()); } From 659ccd94b3d84d5f3da0faad753c7b209477683a Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 06:02:42 +1000 Subject: [PATCH 05/12] Update Windows preset to Visual Studio 2026 and refactor test directory naming --- docs/getting-started.md | 2 +- tests/boolean_test.cpp | 4 ++-- tests/cluster_test.cpp | 4 ++-- tests/explain_batch_test.cpp | 4 ++-- tests/explain_test.cpp | 1 + tests/fit_test.cpp | 7 +++---- tests/id_index_test.cpp | 6 +++--- tests/init_test.cpp | 4 ++-- tests/loader_types_test.cpp | 4 ++-- tests/merge_edges_test.cpp | 4 ++-- tests/minhash_test.cpp | 1 + tests/predict_test.cpp | 4 ++-- tests/process_id.hpp | 27 +++++++++++++++++++++++++++ tests/rescore_test.cpp | 4 ++-- tests/roundtrip_test.cpp | 4 ++-- 15 files changed, 54 insertions(+), 26 deletions(-) create mode 100644 tests/process_id.hpp diff --git a/docs/getting-started.md b/docs/getting-started.md index 3dba2ea..dc85096 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -42,7 +42,7 @@ ctest --preset asan ``` `release` and `debug` are the plain builds with tests on, and `ctest --preset ` runs the suite with output on failure. -On Windows the `windows` preset builds with Visual Studio 2022 against the conda-forge Arrow, which lives under `%CONDA_PREFIX%\Library`, and the build and test presets of the same name select the `Release` configuration, since the Visual Studio generator holds every configuration in one tree. +On Windows the `windows` preset builds with Visual Studio 2026 against the conda-forge Arrow, which lives under `%CONDA_PREFIX%\Library`, and the build and test presets of the same name select the `Release` configuration, since the Visual Studio generator holds every configuration in one tree. The GitHub Actions workflow runs `release`, `asan` and `tsan` on Linux and macOS and `windows` on Windows, then the Python suite and the format and lint checks. ## The whole pipeline in seven commands diff --git a/tests/boolean_test.cpp b/tests/boolean_test.cpp index 23f3702..704ba0d 100644 --- a/tests/boolean_test.cpp +++ b/tests/boolean_test.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -35,6 +34,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" namespace { @@ -292,7 +292,7 @@ class BooleanLoadFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_boolean_" + std::to_string(::getpid())); + ("cpplink_boolean_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "flags.parquet").string(); diff --git a/tests/cluster_test.cpp b/tests/cluster_test.cpp index 396e004..4049fdd 100644 --- a/tests/cluster_test.cpp +++ b/tests/cluster_test.cpp @@ -15,13 +15,13 @@ #include #include #include -#include #include "cpplink/merge_edges.hpp" #include "cpplink/predict.hpp" #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -40,7 +40,7 @@ class ClusterFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_cluster_" + std::to_string(::getpid())); + ("cpplink_cluster_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); diff --git a/tests/explain_batch_test.cpp b/tests/explain_batch_test.cpp index 20e1268..2014d90 100644 --- a/tests/explain_batch_test.cpp +++ b/tests/explain_batch_test.cpp @@ -21,11 +21,11 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/model.hpp" #include "cpplink/sample_data.hpp" +#include "tests/process_id.hpp" namespace { @@ -67,7 +67,7 @@ class ExplainBatch : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_explain_batch_" + std::to_string(::getpid())); + ("cpplink_explain_batch_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); diff --git a/tests/explain_test.cpp b/tests/explain_test.cpp index 66f9d1d..f87a78a 100644 --- a/tests/explain_test.cpp +++ b/tests/explain_test.cpp @@ -3,6 +3,7 @@ #include "cpplink/explain.hpp" +#include #include #include #include diff --git a/tests/fit_test.cpp b/tests/fit_test.cpp index 84122b0..5ee0bdb 100644 --- a/tests/fit_test.cpp +++ b/tests/fit_test.cpp @@ -14,7 +14,6 @@ #include #include -#include #include "cpplink/app.hpp" #include "cpplink/blocking.hpp" @@ -24,6 +23,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -290,9 +290,8 @@ TEST_F(FitFixture, TheReportPrintsTheFit) { // --report writes the full report beside the compact one on the terminal. TEST(FitReport, ReportOptionWritesTheFullReport) { - const std::filesystem::path dir = - std::filesystem::temp_directory_path() / - ("cpplink_fit_report_" + std::to_string(::getpid())); + const std::filesystem::path dir = std::filesystem::temp_directory_path() / + ("cpplink_fit_report_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string parquet = (dir / "sample.parquet").string(); cpplink::SampleOptions sample; diff --git a/tests/id_index_test.cpp b/tests/id_index_test.cpp index 379e6e8..b38c4a7 100644 --- a/tests/id_index_test.cpp +++ b/tests/id_index_test.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/cluster.hpp" @@ -33,6 +32,7 @@ #include "cpplink/recall.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -181,7 +181,7 @@ class SharedIdFiles : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_shared_ids_" + std::to_string(::getpid())); + ("cpplink_shared_ids_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); store_ = SharedIdStore(); @@ -465,7 +465,7 @@ class SharedIdCommands : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_shared_id_cli_" + std::to_string(::getpid())); + ("cpplink_shared_id_cli_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); left_ = (dir_ / "left.parquet").string(); diff --git a/tests/init_test.cpp b/tests/init_test.cpp index 8cea506..d34a26a 100644 --- a/tests/init_test.cpp +++ b/tests/init_test.cpp @@ -14,13 +14,13 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -93,7 +93,7 @@ class DraftFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_init_" + std::to_string(::getpid())); + ("cpplink_init_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir_); path_ = (dir_ / "sample.parquet").string(); cpplink::SampleOptions options; diff --git a/tests/loader_types_test.cpp b/tests/loader_types_test.cpp index 7f8a457..676a335 100644 --- a/tests/loader_types_test.cpp +++ b/tests/loader_types_test.cpp @@ -20,11 +20,11 @@ #include #include #include -#include #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -32,7 +32,7 @@ class LoaderTypes : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_loader_types_" + std::to_string(::getpid())); + ("cpplink_loader_types_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "typed.parquet").string(); diff --git a/tests/merge_edges_test.cpp b/tests/merge_edges_test.cpp index bed9c80..3d4d624 100644 --- a/tests/merge_edges_test.cpp +++ b/tests/merge_edges_test.cpp @@ -15,11 +15,11 @@ #include #include #include -#include #include "cpplink/predict.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -42,7 +42,7 @@ class MergeFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. root_ = std::filesystem::temp_directory_path() / - ("cpplink_merge_" + std::to_string(::getpid())); + ("cpplink_merge_" + cpplink_test::ProcessId()); std::filesystem::remove_all(root_); std::filesystem::create_directories(root_); diff --git a/tests/minhash_test.cpp b/tests/minhash_test.cpp index a463004..f08fb5f 100644 --- a/tests/minhash_test.cpp +++ b/tests/minhash_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include diff --git a/tests/predict_test.cpp b/tests/predict_test.cpp index e74e690..2d40851 100644 --- a/tests/predict_test.cpp +++ b/tests/predict_test.cpp @@ -15,7 +15,6 @@ #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -24,6 +23,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" namespace { @@ -65,7 +65,7 @@ class PredictFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_predict_" + std::to_string(::getpid())); + ("cpplink_predict_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); diff --git a/tests/process_id.hpp b/tests/process_id.hpp new file mode 100644 index 0000000..c5f9e2d --- /dev/null +++ b/tests/process_id.hpp @@ -0,0 +1,27 @@ +// Copyright 2026 Mathieu Fourment +// SPDX-License-Identifier: MIT + +#pragma once + +#include + +#ifdef _WIN32 +#include +#else +#include +#endif + +namespace cpplink_test { + +// The process id as text, for naming a temporary directory that no other run of +// the test binary shares. Windows spells the call with a leading underscore and +// declares it in a different header, which is the whole reason this exists. +inline std::string ProcessId() { +#ifdef _WIN32 + return std::to_string(_getpid()); +#else + return std::to_string(::getpid()); +#endif +} + +} // namespace cpplink_test diff --git a/tests/rescore_test.cpp b/tests/rescore_test.cpp index e73bf7f..cfa3ac4 100644 --- a/tests/rescore_test.cpp +++ b/tests/rescore_test.cpp @@ -16,7 +16,6 @@ #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -26,6 +25,7 @@ #include "cpplink/schema.hpp" #include "cpplink/score.hpp" #include "cpplink/spill.hpp" +#include "tests/process_id.hpp" namespace { @@ -67,7 +67,7 @@ class RescoreFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_rescore_" + std::to_string(::getpid())); + ("cpplink_rescore_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); diff --git a/tests/roundtrip_test.cpp b/tests/roundtrip_test.cpp index ca5d133..514cdf6 100644 --- a/tests/roundtrip_test.cpp +++ b/tests/roundtrip_test.cpp @@ -12,7 +12,6 @@ #include #include -#include #include "cpplink/app.hpp" #include "cpplink/blocking.hpp" @@ -25,6 +24,7 @@ #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" #include "cpplink/string_metrics.hpp" +#include "tests/process_id.hpp" namespace { @@ -73,7 +73,7 @@ class RoundTrip : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_roundtrip_" + std::to_string(::getpid())); + ("cpplink_roundtrip_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); From fc69301dc41174013735ddc89db06a8eaeb5e44a Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 06:51:54 +1000 Subject: [PATCH 06/12] Add project root to run_tests include path The tests include tests/process_id.hpp rooted at the project directory, but run_tests only inherited src/ from cpplink_core, so the header was not found. Co-Authored-By: Claude Opus 5 (1M context) --- CMakeLists.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 60930e7..d211d17 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -61,6 +61,9 @@ if(BUILD_TESTING) file(GLOB TEST_SOURCES CONFIGURE_DEPENDS tests/*.cpp) add_executable(run_tests ${TEST_SOURCES}) target_link_libraries(run_tests PRIVATE cpplink_core GTest::gtest_main) + # Test-only headers are included as "tests/x.hpp", rooted at the project + # directory, the way the library's are rooted at src/. + target_include_directories(run_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) # A test reading a checked-in fixture names it from the source tree, so the # suite passes from ctest's working directory as well as from the root. target_compile_definitions(run_tests PRIVATE From afbcb92335a8bda7728dbeb7ff2a398ec67eafc2 Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 21:17:50 +1000 Subject: [PATCH 07/12] Adding targets to the build preset for windows --- .github/workflows/ci.yml | 6 +++++- CMakePresets.json | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7729cd5..f938155 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,7 +48,11 @@ jobs: # multi-config, so the preset names the configuration at build and test time # rather than through CMAKE_BUILD_TYPE; conda puts every library under # Library/, which is what the preset's prefix path points at. cmd rather than - # bash so that CONDA_PREFIX reaches CMake as a Windows path. + # bash so that CONDA_PREFIX reaches CMake as a Windows path. The build preset + # names its targets rather than taking the solution's default build, which on + # this generator includes the RUN_TESTS project: that runs ctest before + # anything is compiled, so the build aborts on a test executable it has not + # built yet. windows: name: msvc on windows-latest runs-on: windows-latest diff --git a/CMakePresets.json b/CMakePresets.json index b0ab907..bfe0a83 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -57,7 +57,12 @@ {"name": "debug", "configurePreset": "debug"}, {"name": "asan", "configurePreset": "asan"}, {"name": "tsan", "configurePreset": "tsan"}, - {"name": "windows", "configurePreset": "windows", "configuration": "Release"} + { + "name": "windows", + "configurePreset": "windows", + "configuration": "Release", + "targets": ["cpplink", "run_tests"] + } ], "testPresets": [ { From 157f77befb475cd51ce48b4c98dae413e9c11a3c Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 21:36:45 +1000 Subject: [PATCH 08/12] Refactor test executable target to avoid naming conflicts --- CMakeLists.txt | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d211d17..596fbaf 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -59,19 +59,26 @@ if(BUILD_TESTING) include(GoogleTest) file(GLOB TEST_SOURCES CONFIGURE_DEPENDS tests/*.cpp) - add_executable(run_tests ${TEST_SOURCES}) - target_link_libraries(run_tests PRIVATE cpplink_core GTest::gtest_main) + # The target is cpplink_tests and the binary is run_tests: enable_testing() + # defines a target called RUN_TESTS, and the Visual Studio generator names a + # project file after its target, so a target called run_tests and that one + # are the same file on a case-insensitive filesystem. Whichever is written + # second wins, and building the test executable then runs ctest against a + # tree with no test executable in it. + add_executable(cpplink_tests ${TEST_SOURCES}) + set_target_properties(cpplink_tests PROPERTIES OUTPUT_NAME run_tests) + target_link_libraries(cpplink_tests PRIVATE cpplink_core GTest::gtest_main) # Test-only headers are included as "tests/x.hpp", rooted at the project # directory, the way the library's are rooted at src/. - target_include_directories(run_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) + target_include_directories(cpplink_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) # A test reading a checked-in fixture names it from the source tree, so the # suite passes from ctest's working directory as well as from the root. - target_compile_definitions(run_tests PRIVATE + target_compile_definitions(cpplink_tests PRIVATE CPPLINK_SOURCE_DIR="${CMAKE_CURRENT_SOURCE_DIR}") # Arrow pulls in a large shared library graph; discovery needs longer than the # 5 s default to load it. - gtest_discover_tests(run_tests DISCOVERY_TIMEOUT 120) + gtest_discover_tests(cpplink_tests DISCOVERY_TIMEOUT 120) endif() # clang-format target From e3004c5ffda94f8166f6a15caa824ca90f6e309c Mon Sep 17 00:00:00 2001 From: 4ment Date: Tue, 22 Sep 2026 21:44:48 +1000 Subject: [PATCH 09/12] Update Windows test preset to use cpplink_tests target --- CMakePresets.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakePresets.json b/CMakePresets.json index bfe0a83..d78d595 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -61,7 +61,7 @@ "name": "windows", "configurePreset": "windows", "configuration": "Release", - "targets": ["cpplink", "run_tests"] + "targets": ["cpplink", "cpplink_tests"] } ], "testPresets": [ From 4bcf4661933a610fde80eebc66128e06e08b985d Mon Sep 17 00:00:00 2001 From: 4ment Date: Wed, 23 Sep 2026 06:02:09 +1000 Subject: [PATCH 10/12] Refactor test file process ID retrieval to use cpplink_test::ProcessId() for consistency --- .github/workflows/ci.yml | 10 ++++++---- tests/arrow_export_test.cpp | 4 ++-- tests/batch_loader_test.cpp | 4 ++-- tests/edge_table_test.cpp | 4 ++-- tests/levels_test.cpp | 2 +- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 09b6544..e866aee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,10 +49,12 @@ jobs: # rather than through CMAKE_BUILD_TYPE; conda puts every library under # Library/, which is what the preset's prefix path points at. cmd rather than # bash so that CONDA_PREFIX reaches CMake as a Windows path. The build preset - # names its targets rather than taking the solution's default build, which on - # this generator includes the RUN_TESTS project: that runs ctest before - # anything is compiled, so the build aborts on a test executable it has not - # built yet. + # names its targets so that the build never enters the RUN_TESTS project + # enable_testing() defines, which runs ctest and so would test the tree from + # inside the build that is meant to produce it. Those names are CMake target + # names, not file names: MSBuild matches them against the generated project + # files, which are case-insensitive here, so a target called run_tests would + # select RUN_TESTS. windows: name: msvc on windows-latest runs-on: windows-latest diff --git a/tests/arrow_export_test.cpp b/tests/arrow_export_test.cpp index e53017b..45e46d9 100644 --- a/tests/arrow_export_test.cpp +++ b/tests/arrow_export_test.cpp @@ -19,12 +19,12 @@ #include #include #include -#include #include "cpplink/batch_loader.hpp" #include "cpplink/parquet_io.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -223,7 +223,7 @@ TEST(ArrowExport, ABatchCanBeReusedAfterExport) { // same batches, through nothing but the C structs on either side. TEST(ArrowExport, ParquetRoundTrip) { const auto dir = std::filesystem::temp_directory_path() / - ("cpplink_arrow_export_" + std::to_string(::getpid())); + ("cpplink_arrow_export_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string path = (dir / "rows.parquet").string(); diff --git a/tests/batch_loader_test.cpp b/tests/batch_loader_test.cpp index 309f37d..1a3b656 100644 --- a/tests/batch_loader_test.cpp +++ b/tests/batch_loader_test.cpp @@ -24,11 +24,11 @@ #include #include #include -#include #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -505,7 +505,7 @@ TEST(BatchLoader, MatchesTheParquetPathMemberForMember) { {true, true, false, true})}); const auto dir = std::filesystem::temp_directory_path() / - ("cpplink_batch_loader_" + std::to_string(::getpid())); + ("cpplink_batch_loader_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string path = (dir / "table.parquet").string(); auto sink = arrow::io::FileOutputStream::Open(path); diff --git a/tests/edge_table_test.cpp b/tests/edge_table_test.cpp index 9ab2bdc..1258a83 100644 --- a/tests/edge_table_test.cpp +++ b/tests/edge_table_test.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include "cpplink/arrow_export.hpp" #include "cpplink/blocking.hpp" @@ -31,6 +30,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" namespace { @@ -69,7 +69,7 @@ class EdgeTableFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_edge_table_" + std::to_string(::getpid())); + ("cpplink_edge_table_" + cpplink_test::ProcessId()); std::filesystem::remove_all(dir_); std::filesystem::create_directories(dir_); std::string error; diff --git a/tests/levels_test.cpp b/tests/levels_test.cpp index 8afc647..137cebe 100644 --- a/tests/levels_test.cpp +++ b/tests/levels_test.cpp @@ -389,7 +389,7 @@ TEST_F(LevelsFixture, ReportsPrintAndParse) { std::ostringstream json; WriteLevelsJson(report, json); - EXPECT_NO_THROW(nlohmann::json::parse(json.str())); + EXPECT_NO_THROW((void)nlohmann::json::parse(json.str())); } TEST(LevelsCommand, NeedsSchemaAndData) { From ecf44268fccf703e114a16496d15e55635c76da4 Mon Sep 17 00:00:00 2001 From: 4ment Date: Wed, 23 Sep 2026 06:32:15 +1000 Subject: [PATCH 11/12] Add temp_dir utility for robust temporary directory cleanup in tests --- tests/arrow_export_test.cpp | 3 +- tests/batch_loader_test.cpp | 3 +- tests/boolean_test.cpp | 3 +- tests/cluster_test.cpp | 5 ++-- tests/edge_table_test.cpp | 3 +- tests/explain_batch_test.cpp | 3 +- tests/id_index_test.cpp | 22 +++++++++++--- tests/init_test.cpp | 3 +- tests/loader_types_test.cpp | 3 +- tests/merge_edges_test.cpp | 5 ++-- tests/predict_test.cpp | 3 +- tests/rescore_test.cpp | 3 +- tests/roundtrip_test.cpp | 3 +- tests/temp_dir.hpp | 56 ++++++++++++++++++++++++++++++++++++ 14 files changed, 100 insertions(+), 18 deletions(-) create mode 100644 tests/temp_dir.hpp diff --git a/tests/arrow_export_test.cpp b/tests/arrow_export_test.cpp index 45e46d9..61e2144 100644 --- a/tests/arrow_export_test.cpp +++ b/tests/arrow_export_test.cpp @@ -25,6 +25,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -274,5 +275,5 @@ TEST(ArrowExport, ParquetRoundTrip) { ArrowArrayStream missing; EXPECT_FALSE(cpplink::OpenParquetStream(path, {"gone"}, &missing, &error)); EXPECT_NE(error.find("\"gone\" is not in"), std::string::npos) << error; - std::filesystem::remove_all(dir); + cpplink_test::RemoveAll(dir); } diff --git a/tests/batch_loader_test.cpp b/tests/batch_loader_test.cpp index 1a3b656..26ec399 100644 --- a/tests/batch_loader_test.cpp +++ b/tests/batch_loader_test.cpp @@ -29,6 +29,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -517,7 +518,7 @@ TEST(BatchLoader, MatchesTheParquetPathMemberForMember) { cpplink::RecordStore from_file(schema); std::string error; ASSERT_TRUE(cpplink::LoadParquet(path, schema, &from_file, nullptr, &error)) << error; - std::filesystem::remove_all(dir); + cpplink_test::RemoveAll(dir); // The same table, decoded from its buffers in batches of two. cpplink::RecordStore from_buffers(schema); diff --git a/tests/boolean_test.cpp b/tests/boolean_test.cpp index 704ba0d..fb66bb6 100644 --- a/tests/boolean_test.cpp +++ b/tests/boolean_test.cpp @@ -35,6 +35,7 @@ #include "cpplink/schema.hpp" #include "cpplink/score.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -293,7 +294,7 @@ class BooleanLoadFixture : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_boolean_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "flags.parquet").string(); } diff --git a/tests/cluster_test.cpp b/tests/cluster_test.cpp index 4049fdd..52a5e17 100644 --- a/tests/cluster_test.cpp +++ b/tests/cluster_test.cpp @@ -22,6 +22,7 @@ #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -41,7 +42,7 @@ class ClusterFixture : public ::testing::Test { // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / ("cpplink_cluster_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); cpplink::Schema schema; @@ -54,7 +55,7 @@ class ClusterFixture : public ::testing::Test { store_->Finalize(); } - void TearDown() override { std::filesystem::remove_all(dir_); } + void TearDown() override { cpplink_test::RemoveAll(dir_); } // Writes one shard in the exact form predict emits. std::string WriteShard(const std::string& name, diff --git a/tests/edge_table_test.cpp b/tests/edge_table_test.cpp index 1258a83..caa29e9 100644 --- a/tests/edge_table_test.cpp +++ b/tests/edge_table_test.cpp @@ -31,6 +31,7 @@ #include "cpplink/schema.hpp" #include "cpplink/score.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -70,7 +71,7 @@ class EdgeTableFixture : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_edge_table_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; ASSERT_TRUE(cpplink::ParseSchema(kSchemaJson, &schema_, &error)) << error; diff --git a/tests/explain_batch_test.cpp b/tests/explain_batch_test.cpp index 2014d90..409927b 100644 --- a/tests/explain_batch_test.cpp +++ b/tests/explain_batch_test.cpp @@ -26,6 +26,7 @@ #include "cpplink/model.hpp" #include "cpplink/sample_data.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -68,7 +69,7 @@ class ExplainBatch : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_explain_batch_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); schema_ = (dir_ / "schema.json").string(); diff --git a/tests/id_index_test.cpp b/tests/id_index_test.cpp index b38c4a7..5cbfb8f 100644 --- a/tests/id_index_test.cpp +++ b/tests/id_index_test.cpp @@ -33,6 +33,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -55,14 +56,27 @@ std::unique_ptr SharedIdStore() { TEST(DatasetNames, AreTheFileStemsMadeDistinctAndCsvSafe) { EXPECT_TRUE(cpplink::DatasetNamesFor({"only.parquet"}).empty()); const std::vector names = cpplink::DatasetNamesFor( - {"/data/a.parquet", "b/a.parquet", "x,y:z.parquet", "/other/b.parquet"}); + {"/data/a.parquet", "b/a.parquet", "x,y.parquet", "/other/b.parquet"}); ASSERT_EQ(names.size(), 4u); EXPECT_EQ(names[0], "a"); EXPECT_EQ(names[1], "a#1"); - EXPECT_EQ(names[2], "x_y_z"); + EXPECT_EQ(names[2], "x_y"); EXPECT_EQ(names[3], "b"); } +// The other character a name may not carry, checked where a file can carry it. +// Windows forbids a colon in a filename and its path parser reads one in a path +// as punctuation rather than as part of the stem, so there the rule guards +// against a name no file can have. +#ifndef _WIN32 +TEST(DatasetNames, AColonInAStemIsReplaced) { + const std::vector names = + cpplink::DatasetNamesFor({"x:y.parquet", "b.parquet"}); + ASSERT_EQ(names.size(), 2u); + EXPECT_EQ(names[0], "x_y") << "a name is written into `dataset:id`"; +} +#endif + TEST(DatasetNames, ASingleInputHasNoNameAndNoQualifier) { cpplink::Schema schema; schema.unique_id = "id"; @@ -182,7 +196,7 @@ class SharedIdFiles : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_shared_ids_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); store_ = SharedIdStore(); } @@ -466,7 +480,7 @@ class SharedIdCommands : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_shared_id_cli_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); left_ = (dir_ / "left.parquet").string(); right_ = (dir_ / "right.parquet").string(); diff --git a/tests/init_test.cpp b/tests/init_test.cpp index d34a26a..e0f5700 100644 --- a/tests/init_test.cpp +++ b/tests/init_test.cpp @@ -21,6 +21,7 @@ #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -102,7 +103,7 @@ class DraftFixture : public ::testing::Test { std::string error; ASSERT_TRUE(cpplink::WriteSampleParquet(path_, options, &error)) << error; } - void TearDown() override { std::filesystem::remove_all(dir_); } + void TearDown() override { cpplink_test::RemoveAll(dir_); } const cpplink::DraftColumn* Column(const cpplink::DraftReport& report, const std::string& name) { diff --git a/tests/loader_types_test.cpp b/tests/loader_types_test.cpp index 676a335..4814db7 100644 --- a/tests/loader_types_test.cpp +++ b/tests/loader_types_test.cpp @@ -25,6 +25,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -33,7 +34,7 @@ class LoaderTypes : public ::testing::Test { void SetUp() override { dir_ = std::filesystem::temp_directory_path() / ("cpplink_loader_types_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "typed.parquet").string(); } diff --git a/tests/merge_edges_test.cpp b/tests/merge_edges_test.cpp index 3d4d624..8920d93 100644 --- a/tests/merge_edges_test.cpp +++ b/tests/merge_edges_test.cpp @@ -20,6 +20,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -43,7 +44,7 @@ class MergeFixture : public ::testing::Test { // and deleting the directory under one another. root_ = std::filesystem::temp_directory_path() / ("cpplink_merge_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(root_); + cpplink_test::RemoveAll(root_); std::filesystem::create_directories(root_); cpplink::Schema schema; @@ -56,7 +57,7 @@ class MergeFixture : public ::testing::Test { store_->Finalize(); } - void TearDown() override { std::filesystem::remove_all(root_); } + void TearDown() override { cpplink_test::RemoveAll(root_); } std::filesystem::path MakeDir(const std::string& name) const { const std::filesystem::path dir = root_ / name; diff --git a/tests/predict_test.cpp b/tests/predict_test.cpp index 2d40851..be88e26 100644 --- a/tests/predict_test.cpp +++ b/tests/predict_test.cpp @@ -24,6 +24,7 @@ #include "cpplink/schema.hpp" #include "cpplink/score.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -66,7 +67,7 @@ class PredictFixture : public ::testing::Test { // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / ("cpplink_predict_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; diff --git a/tests/rescore_test.cpp b/tests/rescore_test.cpp index cfa3ac4..7f6efb9 100644 --- a/tests/rescore_test.cpp +++ b/tests/rescore_test.cpp @@ -26,6 +26,7 @@ #include "cpplink/score.hpp" #include "cpplink/spill.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -68,7 +69,7 @@ class RescoreFixture : public ::testing::Test { // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / ("cpplink_rescore_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; diff --git a/tests/roundtrip_test.cpp b/tests/roundtrip_test.cpp index 514cdf6..bb69309 100644 --- a/tests/roundtrip_test.cpp +++ b/tests/roundtrip_test.cpp @@ -25,6 +25,7 @@ #include "cpplink/schema.hpp" #include "cpplink/string_metrics.hpp" #include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -74,7 +75,7 @@ class RoundTrip : public ::testing::Test { // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / ("cpplink_roundtrip_" + cpplink_test::ProcessId()); - std::filesystem::remove_all(dir_); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); link_ = (dir_ / "sample.b.parquet").string(); diff --git a/tests/temp_dir.hpp b/tests/temp_dir.hpp new file mode 100644 index 0000000..89319d2 --- /dev/null +++ b/tests/temp_dir.hpp @@ -0,0 +1,56 @@ +// Copyright 2026 Mathieu Fourment +// SPDX-License-Identifier: MIT + +#pragma once + +#include +#include +#include +#include +#include +#include + +namespace cpplink_test { + +// Removes a temporary directory a test owns, retrying while the platform says a +// file in it is still in use. Windows refuses the delete until the last handle +// on a file is gone, and the last handle is not always one the test can see: a +// reader's read-ahead can outlive the object that owns it, and a virus scanner +// opening a file the test has just written is enough on its own. Posix unlinks +// an open file without complaint, so the same lingering handle costs nothing +// there and this loop returns on its first attempt. +// +// A handle the process really leaked never clears, and that is a bug worth +// failing on, so giving up names the entry that is still held rather than the +// directory the caller passed -- which is all the platform's own message says. +inline void RemoveAll(const std::filesystem::path& dir) { + constexpr int kAttempts = 40; + constexpr auto kPause = std::chrono::milliseconds(50); + for (int attempt = 0; attempt < kAttempts; ++attempt) { + std::error_code ec; + std::filesystem::remove_all(dir, ec); + if (!ec) return; + std::this_thread::sleep_for(kPause); + } + // Still there after the whole budget: remove the entries one at a time, so + // the failure names the file rather than its directory. The paths are taken + // first and removed deepest first, because the walk cannot outlive the + // entries it is walking. + std::error_code ec; + std::vector entries; + for (const auto& entry : std::filesystem::recursive_directory_iterator(dir, ec)) { + entries.push_back(entry.path()); + } + for (auto it = entries.rbegin(); it != entries.rend(); ++it) { + std::error_code removed; + std::filesystem::remove(*it, removed); + if (removed) { + throw std::filesystem::filesystem_error( + "still held after " + std::to_string(kAttempts * kPause.count()) + " ms", + *it, removed); + } + } + std::filesystem::remove_all(dir); +} + +} // namespace cpplink_test From c5036a6e208860611ce561d53b6407918fffe9c5 Mon Sep 17 00:00:00 2001 From: 4ment Date: Wed, 23 Sep 2026 07:13:31 +1000 Subject: [PATCH 12/12] Ensure proper stream closure after WriteTable in tests --- tests/batch_loader_test.cpp | 3 +++ tests/boolean_test.cpp | 2 ++ tests/id_index_test.cpp | 2 ++ tests/init_test.cpp | 2 ++ tests/loader_types_test.cpp | 2 ++ 5 files changed, 11 insertions(+) diff --git a/tests/batch_loader_test.cpp b/tests/batch_loader_test.cpp index 26ec399..98da528 100644 --- a/tests/batch_loader_test.cpp +++ b/tests/batch_loader_test.cpp @@ -514,6 +514,9 @@ TEST(BatchLoader, MatchesTheParquetPathMemberForMember) { ASSERT_TRUE(parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, /*chunk_size=*/3) .ok()); + // `WriteTable` does not close a stream it was handed, and a file with a + // handle still on it is one Windows will not let the removal below delete. + ASSERT_TRUE((*sink)->Close().ok()); cpplink::RecordStore from_file(schema); std::string error; diff --git a/tests/boolean_test.cpp b/tests/boolean_test.cpp index fb66bb6..0c0c6ec 100644 --- a/tests/boolean_test.cpp +++ b/tests/boolean_test.cpp @@ -320,6 +320,8 @@ class BooleanLoadFixture : public ::testing::Test { *sink, /*chunk_size=*/3) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } std::filesystem::path dir_; diff --git a/tests/id_index_test.cpp b/tests/id_index_test.cpp index 5cbfb8f..da3d552 100644 --- a/tests/id_index_test.cpp +++ b/tests/id_index_test.cpp @@ -524,6 +524,8 @@ class SharedIdCommands : public ::testing::Test { ASSERT_TRUE( parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, 1024) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } std::filesystem::path dir_; diff --git a/tests/init_test.cpp b/tests/init_test.cpp index e0f5700..c27881a 100644 --- a/tests/init_test.cpp +++ b/tests/init_test.cpp @@ -290,6 +290,8 @@ TEST_F(DraftFixture, DraftsALinkFromTheFirstInputAndChecksTheOthers) { ASSERT_TRUE(parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, 1024) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); }; write(narrow, {arrow::field("id", arrow::utf8())}, {id_array}); std::vector columns; diff --git a/tests/loader_types_test.cpp b/tests/loader_types_test.cpp index 4814db7..b94e310 100644 --- a/tests/loader_types_test.cpp +++ b/tests/loader_types_test.cpp @@ -53,6 +53,8 @@ class LoaderTypes : public ::testing::Test { *sink, /*chunk_size=*/2) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } template