diff --git a/Source/cmFileAPI.cxx b/Source/cmFileAPI.cxx index 6fb9b4b7ac..df9131e696 100644 --- a/Source/cmFileAPI.cxx +++ b/Source/cmFileAPI.cxx @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -155,12 +156,20 @@ std::vector cmFileAPI::LoadDir(std::string const& dir) void cmFileAPI::RemoveOldReplyFiles() { std::string const reply_dir = this->APIv1 + "/reply"; - std::vector files = this->LoadDir(reply_dir); - for (std::string const& f : files) { - if (this->ReplyFiles.find(f) == this->ReplyFiles.end()) { - std::string file = cmStrCat(reply_dir, '/', f); - cmSystemTools::RemoveFile(file); - } + std::vector const files = this->LoadDir(reply_dir); + + // Reply names embed the configuration verbatim, so on a case-insensitive + // filesystem a "debug" reply can alias a just-written "Debug" one; deleting + // by name would strip a file the index still cites. Decide by identity. + std::vector const toRemove = + cmFileAPI::FilesToRemove( + files, this->ReplyFiles, + [&reply_dir](std::string const& name, + cmSystemTools::FileId& id) -> bool { + return cmSystemTools::GetFileId(cmStrCat(reply_dir, '/', name), id); + }); + for (std::string const& f : toRemove) { + cmSystemTools::RemoveFile(cmStrCat(reply_dir, '/', f)); } } diff --git a/Source/cmFileAPI.h b/Source/cmFileAPI.h index 9aad2ca226..b83e070c23 100644 --- a/Source/cmFileAPI.h +++ b/Source/cmFileAPI.h @@ -4,6 +4,7 @@ #include "cmConfigure.h" // IWYU pragma: keep +#include #include #include #include @@ -66,6 +67,49 @@ public: /** Build a JSON object with major and minor fields. */ static Json::Value BuildVersion(unsigned int major, unsigned int minor); + /** Return the subset of 'entries' to delete after a configure: those not + named in 'replyNames' whose identity ('getId') also matches no reply. + Deciding by identity rather than name keeps an entry that aliases a + just-written reply on a case-insensitive filesystem; an entry whose + identity cannot be obtained is retained. Static/templated for tests. */ + template + static std::vector FilesToRemove( + std::vector const& entries, + std::unordered_set const& replyNames, + std::function const& getId) + { + std::vector keptIds; + for (std::string const& name : replyNames) { + FileIdT id; + if (getId(name, id)) { + keptIds.push_back(id); + } + } + + std::vector toRemove; + for (std::string const& entry : entries) { + if (replyNames.find(entry) != replyNames.end()) { + continue; + } + FileIdT id; + if (!getId(entry, id)) { + continue; + } + bool aliasesKept = false; + for (FileIdT const& keptId : keptIds) { + if (id == keptId) { + aliasesKept = true; + break; + } + } + if (aliasesKept) { + continue; + } + toRemove.push_back(entry); + } + return toRemove; + } + private: cmake* CMakeInstance; diff --git a/Tests/CMakeLib/CMakeLists.txt b/Tests/CMakeLib/CMakeLists.txt index 78fe8d7d45..083449a76e 100644 --- a/Tests/CMakeLib/CMakeLists.txt +++ b/Tests/CMakeLib/CMakeLists.txt @@ -12,6 +12,7 @@ set(CMakeLib_TESTS testDateTime.cxx testDebug.cxx testDocumentationFormatter.cxx + testCMFileAPI.cxx testGccDepfileReader.cxx testGeneratedFileStream.cxx testGenExBoundOperand.cxx diff --git a/Tests/CMakeLib/testCMFileAPI.cxx b/Tests/CMakeLib/testCMFileAPI.cxx new file mode 100644 index 0000000000..de795d5cda --- /dev/null +++ b/Tests/CMakeLib/testCMFileAPI.cxx @@ -0,0 +1,102 @@ +/* Distributed under the OSI-approved BSD 3-Clause License. See accompanying + file LICENSE.rst or https://cmake.org/licensing for details. */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include "cmFileAPI.h" + +namespace { + +// Identity oracle over a name->id map. A name absent from the map models a +// file whose identity cannot be obtained (GetFileId failure). +std::function makeOracle( + std::map const& ids) +{ + return [ids](std::string const& name, int& id) -> bool { + auto const it = ids.find(name); + if (it == ids.end()) { + return false; + } + id = it->second; + return true; + }; +} + +bool checkCase(char const* label, std::vector const& entries, + std::unordered_set const& replyNames, + std::map const& ids, + std::set const& expected) +{ + std::vector const removed = + cmFileAPI::FilesToRemove(entries, replyNames, makeOracle(ids)); + std::set const actual(removed.begin(), removed.end()); + if (actual != expected) { + std::cout << "FAILED: " << label << "\n expected removals:"; + for (std::string const& e : expected) { + std::cout << ' ' << e; + } + std::cout << "\n actual removals: "; + for (std::string const& a : actual) { + std::cout << ' ' << a; + } + std::cout << '\n'; + return false; + } + return true; +} + +} // namespace + +int testCMFileAPI(int /*unused*/, char* /*unused*/[]) +{ + bool ok = true; + + // An on-disk entry that resolves to the same identity as a just-written + // reply (a case-only-different name aliasing one inode) is kept. + ok &= checkCase( + "case-variant alias kept", { "target-foo-Debug-H.json" }, + { "target-foo-debug-H.json" }, + { { "target-foo-debug-H.json", 1 }, { "target-foo-Debug-H.json", 1 } }, + {}); + + // A genuinely stale entry with a distinct identity is removed. + ok &= checkCase("distinct stale removed", { "target-old-H.json" }, + { "target-new-H.json" }, + { { "target-new-H.json", 1 }, { "target-old-H.json", 2 } }, + { "target-old-H.json" }); + + // An entry whose exact name was just written is kept without consulting the + // identity oracle (its name is intentionally absent from the id map). + ok &= checkCase("exact-name match kept", { "index-x.json" }, + { "index-x.json" }, {}, {}); + + // Fail-safe: an entry whose identity cannot be obtained is retained. + ok &= checkCase("candidate id failure retained", { "unreadable.json" }, + { "target-new-H.json" }, { { "target-new-H.json", 1 } }, {}); + + // No kept-side fail-safe is needed: an alias shares its inode, so when a + // kept reply's identity is unknown the candidate's is too and the + // candidate-side fail-safe retains it. Both names are absent to model it. + ok &= checkCase("unknown kept id: correlated alias retained", + { "alias-of-unidentified.json" }, + { "target-new-H.json", "unidentified-kept.json" }, + { { "target-new-H.json", 1 } }, {}); + + // Mixed: an aliasing entry is kept while an unrelated stale entry is + // removed. + ok &= checkCase("mixed alias and stale", { "a-alias.json", "stale.json" }, + { "a.json", "b.json" }, + { { "a.json", 1 }, + { "b.json", 2 }, + { "a-alias.json", 1 }, + { "stale.json", 3 } }, + { "stale.json" }); + + return ok ? 0 : 1; +} diff --git a/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-prep.cmake b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-prep.cmake new file mode 100644 index 0000000000..7be4b3946e --- /dev/null +++ b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-prep.cmake @@ -0,0 +1,2 @@ +file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/query") +file(WRITE "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/query/codemodel-v2" "") diff --git a/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-recon-check.cmake b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-recon-check.cmake new file mode 100644 index 0000000000..c3c7dbacc7 --- /dev/null +++ b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure-recon-check.cmake @@ -0,0 +1,21 @@ +set(reply_dir "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/reply") + +# Every reply file referenced from the index must still exist on disk after +# the reconfigure. A dangling "jsonFile" reference is the bug this guards; it +# manifests only on a case-insensitive filesystem (Windows, default macOS). +file(GLOB reply_files "${reply_dir}/*.json") +set(dangling "") +foreach(reply_file IN LISTS reply_files) + file(READ "${reply_file}" content) + string(REGEX MATCHALL "\"jsonFile\"[ \t]*:[ \t]*\"[^\"]+\"" refs "${content}") + foreach(ref IN LISTS refs) + string(REGEX REPLACE "\"jsonFile\"[ \t]*:[ \t]*\"([^\"]+)\"" "\\1" name "${ref}") + if(NOT EXISTS "${reply_dir}/${name}") + get_filename_component(from "${reply_file}" NAME) + string(APPEND dangling "\n '${name}' referenced by ${from} is missing") + endif() + endforeach() +endforeach() +if(dangling) + set(RunCMake_TEST_FAILED "Dangling File API reply references:${dangling}") +endif() diff --git a/Tests/RunCMake/FileAPI/ConfigCaseReconfigure.cmake b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure.cmake new file mode 100644 index 0000000000..8f21663888 --- /dev/null +++ b/Tests/RunCMake/FileAPI/ConfigCaseReconfigure.cmake @@ -0,0 +1,2 @@ +enable_language(C) +add_library(foo STATIC empty.c) diff --git a/Tests/RunCMake/FileAPI/RunCMakeTest.cmake b/Tests/RunCMake/FileAPI/RunCMakeTest.cmake index e9fbf25746..c640b694a6 100644 --- a/Tests/RunCMake/FileAPI/RunCMakeTest.cmake +++ b/Tests/RunCMake/FileAPI/RunCMakeTest.cmake @@ -125,6 +125,19 @@ run_cmake(ProjectQueryGood) run_cmake(ProjectQueryBad) run_cmake(FailConfigure) +# Reconfiguring with a case-only build-type change (Debug -> debug) must not +# leave the reply index citing a target file that cleanup deleted on a +# case-insensitive filesystem (Windows, default macOS). +function(run_config_case) + if(NOT RunCMake_GENERATOR_IS_MULTI_CONFIG) + set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/ConfigCaseReconfigure-build) + run_cmake_with_options(ConfigCaseReconfigure -DCMAKE_BUILD_TYPE=Debug) + set(RunCMake_TEST_NO_CLEAN 1) + run_cmake_command(ConfigCaseReconfigure-recon ${CMAKE_COMMAND} . -DCMAKE_BUILD_TYPE=debug) + endif() +endfunction() +run_config_case() + function(run_object object) set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${object}-build) list(APPEND RunCMake_TEST_OPTIONS ${ARGN} -DCMAKE_POLICY_DEFAULT_CMP0118=NEW)