Merge bitcoin/bitcoin#33550: Fix windows libc++ fs::path fstream compile errors

c864a4c194 Simplify fs::path by dropping filename() and make_preferred() overloads (Ryan Ofsky)
b0113afd44 Fix windows libc++ fs::path fstream compile errors (Ryan Ofsky)

Pull request description:

  Drop support for passing `fs::path` directly to `std::ifstream` and `std::ofstream` constructors and `open()` functions, because as reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545, after https://wg21.link/lwg3430 there is no way this can continue to work in windows builds, and there are already compile errors compiling for windows with newer versions of libc++.

  Instead, add an `fs::path::std_path()` method that returns `std::filesystem::path` references and use it where needed.

ACKs for top commit:
  hebasto:
    ACK c864a4c194.
  l0rinc:
    Code review ACK c864a4c194
  maflcko:
    re-ACK c864a4c194 🌥

Tree-SHA512: d22372692ab86244e2b2caf4c5e9c9acbd9ba38df5411606b75e428474eabead152fc7ca1afe0bb0df6b818351211a70487e94b40a17b68db5aa757604a0ddf6

src/bench/bench.cpp

@@ -56,7 +56,7 @@ void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& 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;

src/bitcoin.cpp

@@ -209,7 +209,7 @@ static void ExecCommand(const std::vector<const char*>& 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<const char*>& 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())) ||

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;

src/common/settings.cpp

@@ -78,7 +78,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& 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;

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());

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();

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);

src/rpc/request.cpp

@@ -114,7 +114,7 @@ GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& 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);

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);

src/test/logging_tests.cpp

@@ -40,7 +40,7 @@ static void ResetLogger()
 static std::vector<std::string> ReadDebugLogLines()
 {
     std::vector<std::string> 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"));

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();

src/test/settings_tests.cpp

@@ -45,7 +45,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::pair<std::string, c
 inline void WriteText(const fs::path& path, const std::string& text)
 {
     std::ofstream file;
-    file.open(path);
+    file.open(path.std_path());
     file << text;
 }
 

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);

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
@@ -65,11 +75,6 @@ public:
         const std::u8string& utf8_str{std::filesystem::path::u8string()};
         return std::string{utf8_str.begin(), utf8_str.end()};
     }
-
-    // Required for path overloads in <fstream>.
-    // 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)

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

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;
@@ -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{};

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();
 }