From b0113afd44b4c7c0d0da9883424bd2978de3d18c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 6 Oct 2025 10:42:56 -0400 Subject: [PATCH 1/2] Fix windows libc++ fs::path fstream compile errors As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545, newer libc++ versions implementing https://wg21.link/lwg3430 will no longer implicitly convert `fs::path` objects to `std::filesystem::path` objects when constructing `std::ifstream` and `std::ofstream` types. This is not a problem in Unix systems since `fs::path` objects use `std::string` as their native string type, but it causes compile errors on Windows which use `std::wstring` as their string type, since `fstream`s can't be constructed from `wstring`s. Fix the windows libc++ compile errors by adding a new `fs::path::std_path()` method and using it construct `fstream`s more portably. Additionally, delete `fs::path`'s implicit `native_string` conversion so these errors will not go undetected in the future, even though there is not currently a CI job testing Windows libc++ builds. --- src/bench/bench.cpp | 2 +- src/common/config.cpp | 4 ++-- src/init.cpp | 2 +- src/qt/guiutil.cpp | 6 +++--- src/qt/test/optiontests.cpp | 2 +- src/test/fs_tests.cpp | 20 ++++++++++---------- src/test/logging_tests.cpp | 4 ++-- src/test/script_assets_tests.cpp | 2 +- src/test/util_tests.cpp | 4 ++-- src/util/fs.h | 10 ++++++++++ src/wallet/db.cpp | 4 ++-- src/wallet/dump.cpp | 2 +- src/wallet/test/init_test_fixture.cpp | 2 +- 13 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index 26daff50705..9cc1f30fd8f 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -56,7 +56,7 @@ void GenerateTemplateResults(const std::vector& bench // nothing to write, bail out return; } - std::ofstream fout{file}; + std::ofstream fout{file.std_path()}; if (fout.is_open()) { ankerl::nanobench::render(tpl, benchmarkResults, fout); std::cout << "Created " << file << std::endl; diff --git a/src/common/config.cpp b/src/common/config.cpp index 42f5e283348..7216022cdd6 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -135,7 +135,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) error = strprintf("Config file \"%s\" is a directory.", fs::PathToString(conf_path)); return false; } - stream = std::ifstream{conf_path}; + stream = std::ifstream{conf_path.std_path()}; // If the file is explicitly specified, it must be readable if (IsArgSet("-conf") && !stream.good()) { error = strprintf("specified config file \"%s\" could not be opened.", fs::PathToString(conf_path)); @@ -187,7 +187,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) error = strprintf("Included config file \"%s\" is a directory.", fs::PathToString(include_conf_path)); return false; } - std::ifstream conf_file_stream{include_conf_path}; + std::ifstream conf_file_stream{include_conf_path.std_path()}; if (conf_file_stream.good()) { if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) { return false; diff --git a/src/init.cpp b/src/init.cpp index dadd56d2469..4f67275190b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -179,7 +179,7 @@ static fs::path GetPidFile(const ArgsManager& args) { if (args.IsArgNegated("-pid")) return true; - std::ofstream file{GetPidFile(args)}; + std::ofstream file{GetPidFile(args).std_path()}; if (file) { #ifdef WIN32 tfm::format(file, "%d\n", GetCurrentProcessId()); diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 7bafb3db853..28610db4512 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -447,7 +447,7 @@ bool openBitcoinConf() fs::path pathConfig = gArgs.GetConfigFilePath(); /* Create the file */ - std::ofstream configFile{pathConfig, std::ios_base::app}; + std::ofstream configFile{pathConfig.std_path(), std::ios_base::app}; if (!configFile.good()) return false; @@ -607,7 +607,7 @@ fs::path static GetAutostartFilePath() bool GetStartOnSystemStartup() { - std::ifstream optionFile{GetAutostartFilePath()}; + std::ifstream optionFile{GetAutostartFilePath().std_path()}; if (!optionFile.good()) return false; // Scan through file for "Hidden=true": @@ -639,7 +639,7 @@ bool SetStartOnSystemStartup(bool fAutoStart) fs::create_directories(GetAutostartDir()); - std::ofstream optionFile{GetAutostartFilePath(), std::ios_base::out | std::ios_base::trunc}; + std::ofstream optionFile{GetAutostartFilePath().std_path(), std::ios_base::out | std::ios_base::trunc}; if (!optionFile.good()) return false; ChainType chain = gArgs.GetChainType(); diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index f8a1da84121..0d522e989d7 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -70,7 +70,7 @@ void OptionTests::migrateSettings() QVERIFY(!settings.contains("fUseSeparateProxyTor")); QVERIFY(!settings.contains("addrSeparateProxyTor")); - std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); + std::ifstream file((gArgs.GetDataDirNet() / "settings.json").std_path()); std::string default_warning = strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " "is running, as any changes might be ignored or overwritten.", CLIENT_NAME); diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp index c237963af3e..28fcf952e52 100644 --- a/src/test/fs_tests.cpp +++ b/src/test/fs_tests.cpp @@ -51,37 +51,37 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) fs::path tmpfile1 = tmpfolder / fs::u8path("fs_tests_₿_🏃"); fs::path tmpfile2 = tmpfolder / fs::path(u8"fs_tests_₿_🏃"); { - std::ofstream file{tmpfile1}; + std::ofstream file{tmpfile1.std_path()}; file << "bitcoin"; } { - std::ifstream file{tmpfile2}; + std::ifstream file{tmpfile2.std_path()}; std::string input_buffer; file >> input_buffer; BOOST_CHECK_EQUAL(input_buffer, "bitcoin"); } { - std::ifstream file{tmpfile1, std::ios_base::in | std::ios_base::ate}; + std::ifstream file{tmpfile1.std_path(), std::ios_base::in | std::ios_base::ate}; std::string input_buffer; file >> input_buffer; BOOST_CHECK_EQUAL(input_buffer, ""); } { - std::ofstream file{tmpfile2, std::ios_base::out | std::ios_base::app}; + std::ofstream file{tmpfile2.std_path(), std::ios_base::out | std::ios_base::app}; file << "tests"; } { - std::ifstream file{tmpfile1}; + std::ifstream file{tmpfile1.std_path()}; std::string input_buffer; file >> input_buffer; BOOST_CHECK_EQUAL(input_buffer, "bitcointests"); } { - std::ofstream file{tmpfile2, std::ios_base::out | std::ios_base::trunc}; + std::ofstream file{tmpfile2.std_path(), std::ios_base::out | std::ios_base::trunc}; file << "bitcoin"; } { - std::ifstream file{tmpfile1}; + std::ifstream file{tmpfile1.std_path()}; std::string input_buffer; file >> input_buffer; BOOST_CHECK_EQUAL(input_buffer, "bitcoin"); @@ -116,12 +116,12 @@ BOOST_AUTO_TEST_CASE(rename) const std::string path2_contents{"2222"}; { - std::ofstream file{path1}; + std::ofstream file{path1.std_path()}; file << path1_contents; } { - std::ofstream file{path2}; + std::ofstream file{path2.std_path()}; file << path2_contents; } @@ -131,7 +131,7 @@ BOOST_AUTO_TEST_CASE(rename) BOOST_CHECK(!fs::exists(path1)); { - std::ifstream file{path2}; + std::ifstream file{path2.std_path()}; std::string contents; file >> contents; BOOST_CHECK_EQUAL(contents, path1_contents); diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index 2e3b1c390b5..3cf261a4293 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -40,7 +40,7 @@ static void ResetLogger() static std::vector ReadDebugLogLines() { std::vector lines; - std::ifstream ifs{LogInstance().m_file_path}; + std::ifstream ifs{LogInstance().m_file_path.std_path()}; for (std::string line; std::getline(ifs, line);) { lines.push_back(std::move(line)); } @@ -421,7 +421,7 @@ void TestLogFromLocation(Location location, const std::string& message, using Status = BCLog::LogRateLimiter::Status; if (!suppressions_active) assert(status == Status::UNSUPPRESSED); // developer error - std::ofstream ofs(LogInstance().m_file_path, std::ios::out | std::ios::trunc); // clear debug log + std::ofstream ofs(LogInstance().m_file_path.std_path(), std::ios::out | std::ios::trunc); // clear debug log LogFromLocation(location, message); auto log_lines{ReadDebugLogLines()}; BOOST_TEST_INFO_SCOPE(log_lines.size() << " log_lines read: \n" << util::Join(log_lines, "\n")); diff --git a/src/test/script_assets_tests.cpp b/src/test/script_assets_tests.cpp index 0e2fec87c03..af96987f0cd 100644 --- a/src/test/script_assets_tests.cpp +++ b/src/test/script_assets_tests.cpp @@ -159,7 +159,7 @@ BOOST_AUTO_TEST_CASE(script_assets_test) bool exists = fs::exists(path); BOOST_WARN_MESSAGE(exists, "File $DIR_UNIT_TEST_DATA/script_assets_test.json not found, skipping script_assets_test"); if (!exists) return; - std::ifstream file{path}; + std::ifstream file{path.std_path()}; BOOST_CHECK(file.is_open()); file.seekg(0, std::ios::end); size_t length = file.tellg(); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index f9038c4fc97..13821db4b95 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1619,7 +1619,7 @@ BOOST_AUTO_TEST_CASE(util_ReadBinaryFile) expected_text += "0123456789"; } { - std::ofstream file{tmpfile}; + std::ofstream file{tmpfile.std_path()}; file << expected_text; } { @@ -1650,7 +1650,7 @@ BOOST_AUTO_TEST_CASE(util_WriteBinaryFile) std::string expected_text = "bitcoin"; auto valid = WriteBinaryFile(tmpfile, expected_text); std::string actual_text; - std::ifstream file{tmpfile}; + std::ifstream file{tmpfile.std_path()}; file >> actual_text; BOOST_CHECK(valid); BOOST_CHECK_EQUAL(actual_text, expected_text); diff --git a/src/util/fs.h b/src/util/fs.h index 7c313e2072c..8ff30482bbd 100644 --- a/src/util/fs.h +++ b/src/util/fs.h @@ -34,6 +34,10 @@ class path : public std::filesystem::path public: using std::filesystem::path::path; + // Convenience method for accessing standard path type without needing a cast. + std::filesystem::path& std_path() { return *this; } + const std::filesystem::path& std_path() const { return *this; } + // Allow path objects arguments for compatibility. path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {} path& operator=(std::filesystem::path path) { std::filesystem::path::operator=(std::move(path)); return *this; } @@ -54,6 +58,12 @@ public: // Disallow std::string conversion method to avoid locale-dependent encoding on windows. std::string string() const = delete; + // Disallow implicit string conversion to ensure code is portable. + // `string_type` may be `string` or `wstring` depending on the platform, so + // using this conversion could result in code that compiles on unix but + // fails to compile on windows, or vice versa. + operator string_type() const = delete; + /** * Return a UTF-8 representation of the path as a std::string, for * compatibility with code using std::string. For code using the newer diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 781b6fa461d..cad3d7b57e7 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -102,7 +102,7 @@ bool IsBDBFile(const fs::path& path) if (ec) LogWarning("Error reading file_size: %s [%s]", ec.message(), fs::PathToString(path)); if (size < 4096) return false; - std::ifstream file{path, std::ios::binary}; + std::ifstream file{path.std_path(), std::ios::binary}; if (!file.is_open()) return false; file.seekg(12, std::ios::beg); // Magic bytes start at offset 12 @@ -126,7 +126,7 @@ bool IsSQLiteFile(const fs::path& path) if (ec) LogWarning("Error reading file_size: %s [%s]", ec.message(), fs::PathToString(path)); if (size < 512) return false; - std::ifstream file{path, std::ios::binary}; + std::ifstream file{path.std_path(), std::ios::binary}; if (!file.is_open()) return false; // Magic is at beginning and is 16 bytes long diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 6b193ad72f3..312753dda10 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -134,7 +134,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: error = strprintf(_("Dump file %s does not exist."), fs::PathToString(dump_path)); return false; } - std::ifstream dump_file{dump_path}; + std::ifstream dump_file{dump_path.std_path()}; // Compute the checksum HashWriter hasher{}; diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp index 5bdf36ec192..a01561df969 100644 --- a/src/wallet/test/init_test_fixture.cpp +++ b/src/wallet/test/init_test_fixture.cpp @@ -36,7 +36,7 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const ChainType chainType) fs::create_directories(m_walletdir_path_cases["default"]); fs::create_directories(m_walletdir_path_cases["custom"]); fs::create_directories(m_walletdir_path_cases["relative"]); - std::ofstream f{m_walletdir_path_cases["file"]}; + std::ofstream f{m_walletdir_path_cases["file"].std_path()}; f.close(); } From c864a4c1940d682f7eb6fdb3b91b18d638b59330 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 6 Oct 2025 11:34:15 -0400 Subject: [PATCH 2/2] Simplify fs::path by dropping filename() and make_preferred() overloads These overloads were needed to allow passing `fs::path` objects directly to libstdc++'s `fstream` constructors, but after the previous commit, there is no longer any remaining code that does pass `fs::path` objects to `fstream` constructors. Writing new code which does this is also discouraged because the standard has been updated in https://wg21.link/lwg3430 to disallow it. Dropping these also means its no longer possible to pass `fs::path` arguments directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not a big loss because it is already not possible to pass them to the constructor. So this commit updates `fstream::open` calls. Additionally, this change required updates to src/bitcoin.cpp since it was relying on the overloaded filename() method. --- src/bitcoin.cpp | 4 ++-- src/common/settings.cpp | 4 ++-- src/rpc/request.cpp | 4 ++-- src/test/settings_tests.cpp | 2 +- src/util/fs.h | 5 ----- src/wallet/dump.cpp | 2 +- 6 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/bitcoin.cpp b/src/bitcoin.cpp index c1a5fce33af..7e4d243a01b 100644 --- a/src/bitcoin.cpp +++ b/src/bitcoin.cpp @@ -209,7 +209,7 @@ static void ExecCommand(const std::vector& args, std::string_view w // Try to resolve any symlinks and figure out the directory containing the wrapper executable. std::error_code ec; - fs::path wrapper_dir{fs::weakly_canonical(wrapper_path, ec)}; + auto wrapper_dir{fs::weakly_canonical(wrapper_path, ec)}; if (wrapper_dir.empty()) wrapper_dir = wrapper_path; // Restore previous path if weakly_canonical failed. wrapper_dir = wrapper_dir.parent_path(); @@ -225,7 +225,7 @@ static void ExecCommand(const std::vector& args, std::string_view w // If wrapper is installed in a bin/ directory, look for target executable // in libexec/ - (wrapper_dir.filename() == "bin" && try_exec(fs::path{wrapper_dir.parent_path()} / "libexec" / arg0.filename())) || + (wrapper_dir.filename() == "bin" && try_exec(wrapper_dir.parent_path() / "libexec" / arg0.filename())) || #ifdef WIN32 // Otherwise check the "daemon" subdirectory in a windows install. (!wrapper_dir.empty() && try_exec(wrapper_dir / "daemon" / arg0.filename())) || diff --git a/src/common/settings.cpp b/src/common/settings.cpp index 046afca15d7..fbe531c73a2 100644 --- a/src/common/settings.cpp +++ b/src/common/settings.cpp @@ -78,7 +78,7 @@ bool ReadSettings(const fs::path& path, std::map& va if (!fs::exists(path)) return true; std::ifstream file; - file.open(path); + file.open(path.std_path()); if (!file.is_open()) { errors.emplace_back(strprintf("%s. Please check permissions.", fs::PathToString(path))); return false; @@ -133,7 +133,7 @@ bool WriteSettings(const fs::path& path, out.pushKVEnd(value.first, value.second); } std::ofstream file; - file.open(path); + file.open(path.std_path()); if (file.fail()) { errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", fs::PathToString(path))); return false; diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index e07072fd045..df28e4ea3ec 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -114,7 +114,7 @@ GenerateAuthCookieResult GenerateAuthCookie(const std::optional& cook if (filepath_tmp.empty()) { return GenerateAuthCookieResult::DISABLED; // -norpccookiefile } - file.open(filepath_tmp); + file.open(filepath_tmp.std_path()); if (!file.is_open()) { LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp)); return GenerateAuthCookieResult::ERR; @@ -153,7 +153,7 @@ bool GetAuthCookie(std::string *cookie_out) if (filepath.empty()) { return true; // -norpccookiefile } - file.open(filepath); + file.open(filepath.std_path()); if (!file.is_open()) return false; std::getline(file, cookie); diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index 95f38fc0ce6..00a566d784e 100644 --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -45,7 +45,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::pair. - // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190 - path& make_preferred() { std::filesystem::path::make_preferred(); return *this; } - path filename() const { return std::filesystem::path::filename(); } }; static inline path u8path(const std::string& utf8_str) diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 312753dda10..c63b95b5d84 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -37,7 +37,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro return false; } std::ofstream dump_file; - dump_file.open(path); + dump_file.open(path.std_path()); if (dump_file.fail()) { error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path)); return false;