Fix windows libc++ `fs::path` `fstream` compile errors

ryanofsky commented at 3:08 pm on October 6, 2025: contributor

Drop support for passing fs::path directly to std::ifstream and std::ofstream constructors and open() functions, because as reported by hebasto in #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.

DrahtBot commented at 3:08 pm on October 6, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33550.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Stale ACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto commented at 3:14 pm on October 6, 2025: member

Concept ACK.

Maybe consider removing these lines now:https://github.com/bitcoin/bitcoin/blob/f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f/src/util/fs.h#L79-L81 ?

hebasto commented at 3:17 pm on October 6, 2025: member

CI fails:

0 /home/admin/actions-runner/_work/_temp/src/wallet/test/init_test_fixture.cpp:39:21: error: conversion function from 'mapped_type' (aka 'fs::path') to 'const string' (aka 'const basic_string<char>') invokes a deleted function
1   39 |     std::ofstream f{m_walletdir_path_cases["file"]};
2      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3/home/admin/actions-runner/_work/_temp/src/util/fs.h:65:5: note: 'operator basic_string' has been explicitly marked deleted here
4   65 |     operator string_type() const = delete;
5      |     ^
6/cxx_build/include/c++/v1/fstream:1303:63: note: passing argument to parameter '__s' here
7 1303 |   _LIBCPP_HIDE_FROM_ABI explicit basic_ofstream(const string& __s, ios_base::openmode __mode = ios_base::out);
8      |                                                               ^
91 error generated.

maflcko commented at 3:24 pm on October 6, 2025: member

review ACK f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f, but CI fails

DrahtBot requested review from hebasto on Oct 6, 2025

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.

b0113afd44

ryanofsky force-pushed on Oct 6, 2025

ryanofsky commented at 3:27 pm on October 6, 2025: contributor

Updated f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f -> b0113afd44b4c7c0d0da9883424bd2978de3d18c (pr/winstream.1 -> pr/winstream.2, compare) to fix CI failures caused by missing std_path replacement

ryanofsky commented at 3:43 pm on October 6, 2025: contributor

re: #33550#pullrequestreview-3305372031

Maybe consider removing these lines now:

Nice suggestion. Attempted this in new commit.

Added 1 commits b0113afd44b4c7c0d0da9883424bd2978de3d18c -> d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d (pr/winstream.2 -> pr/winstream.3, compare) simplifying fs::path as suggested

hebasto commented at 3:44 pm on October 6, 2025: member

re: #33550 (review)

Maybe consider removing these lines now:

Nice suggestion. Attempted this in new commit.

Added 1 commits b0113af -> d405c76 (pr/winstream.2 -> pr/winstream.3, compare) simplifying fs::path as suggested

Thanks for taking this.

I assume we still need fs::path::filename() though.

hebasto commented at 4:01 pm on October 6, 2025: member

Tested b0113afd44b4c7c0d0da9883424bd2978de3d18c by cross-compiling for x86_64-w64-mingw32 and aarch64-w64-mingw32 targets using the LLVM MinGW toolchain.

in src/util/fs.h:71 in d405c76d2e outdated

74@@ -65,11 +75,6 @@ class path : public std::filesystem::path
75         const std::u8string& utf8_str{std::filesystem::path::u8string()};
76         return std::string{utf8_str.begin(), utf8_str.end()};
77     }
78-
79-    // Required for path overloads in <fstream>.
80-    // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190
81-    path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }

hebasto commented at 4:11 pm on October 6, 2025:

This might require a bit more adjustments:

 0--- a/src/common/settings.cpp
 1+++ b/src/common/settings.cpp
 2@@ -78,7 +78,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
 3     if (!fs::exists(path)) return true;
 4 
 5     std::ifstream file;
 6-    file.open(path);
 7+    file.open(path.std_path());
 8     if (!file.is_open()) {
 9       errors.emplace_back(strprintf("%s. Please check permissions.", fs::PathToString(path)));
10       return false;
11@@ -133,7 +133,7 @@ bool WriteSettings(const fs::path& path,
12         out.pushKVEnd(value.first, value.second);
13     }
14     std::ofstream file;
15-    file.open(path);
16+    file.open(path.std_path());
17     if (file.fail()) {
18         errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", fs::PathToString(path)));
19         return false;
20--- a/src/rpc/request.cpp
21+++ b/src/rpc/request.cpp
22@@ -114,7 +114,7 @@ GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cook
23     if (filepath_tmp.empty()) {
24         return GenerateAuthCookieResult::DISABLED; // -norpccookiefile
25     }
26-    file.open(filepath_tmp);
27+    file.open(filepath_tmp.std_path());
28     if (!file.is_open()) {
29         LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
30         return GenerateAuthCookieResult::ERR;
31@@ -153,7 +153,7 @@ bool GetAuthCookie(std::string *cookie_out)
32     if (filepath.empty()) {
33         return true; // -norpccookiefile
34     }
35-    file.open(filepath);
36+    file.open(filepath.std_path());
37     if (!file.is_open())
38         return false;
39     std::getline(file, cookie);
40--- a/src/test/settings_tests.cpp
41+++ b/src/test/settings_tests.cpp
42@@ -45,7 +45,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::pair<std::string, c
43 inline void WriteText(const fs::path& path, const std::string& text)
44 {
45     std::ofstream file;
46-    file.open(path);
47+    file.open(path.std_path());
48     file << text;
49 }
50 
51--- a/src/wallet/dump.cpp
52+++ b/src/wallet/dump.cpp
53@@ -37,7 +37,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
54         return false;
55     }
56     std::ofstream dump_file;
57-    dump_file.open(path);
58+    dump_file.open(path.std_path());
59     if (dump_file.fail()) {
60         error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path));
61         return false;

ryanofsky commented at 4:13 pm on October 6, 2025: contributor

I assume we still need fs::path::filename() though.

I think we could overload it but it seems a little consistent to do that without a clear reason and without also overloading parent_path. For now just dropped some code in bitcoin.cpp which was using it.

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.

c864a4c194

ryanofsky force-pushed on Oct 6, 2025

ryanofsky commented at 4:15 pm on October 6, 2025: contributor

Updated d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d -> c864a4c1940d682f7eb6fdb3b91b18d638b59330 (pr/winstream.3 -> pr/winstream.4, compare) with CI fixes for second commit

hebasto approved

hebasto commented at 4:31 pm on October 6, 2025: member

ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330.

DrahtBot requested review from maflcko on Oct 6, 2025

hebasto commented at 8:58 pm on October 6, 2025: member

… even though there is not currently a CI job testing Windows libc++ builds.

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

fanquake commented at 3:49 pm on October 7, 2025: member

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
1test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
2test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
3...

hebasto commented at 3:51 pm on October 7, 2025: member

Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0
1test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
2
3test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
4
5test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
6
7...

Correct. I’m looking into them.

However, I believe those failures are not related to this PR changes.

hebasto commented at 7:33 pm on October 7, 2025: member

Added to the nightly builds in hebasto/bitcoin-core-nightly#74.

Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):

0test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
1test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
2test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
3...

The issue appears to be caused by this commit.

Friendly ping @l0rinc @hodlinator as co-authors of that commit :)

l0rinc commented at 10:58 pm on October 7, 2025: contributor

I’m on it, it seems related to the discussion in the original PR which already had some Windows build problems: #30546 (review)

I have reproduced the error in https://github.com/l0rinc/bitcoin-core-nightly/pull/2 with @hebasto’s nightly clone redirected to l0rinc/master.

I pushed a potential fix that adds a const char * overload to HasReason in https://github.com/l0rinc/bitcoin/pull/43 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/1.

Also added an alternative which replaces all remaining const char * throws with std::runtime_error in https://github.com/l0rinc/bitcoin/pull/44 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/3.

Based on your feedback and on what the CI gods think, I will push one of them as a PR.

Also note that the windows build indicates additional warnings (unused vars because of ifndef __USING_WINDOWS__)

 0/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/randomenv.cpp:61:15: warning: '__p__environ' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
 1   61 | extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms
 2      |               ^
 3/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:656:17: note: expanded from macro 'environ'
 4  656 | #define environ _environ
 5      |                 ^
 6/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:225:21: note: expanded from macro '_environ'
 7  225 | #define _environ (* __p__environ())
 8      |                     ^
 9/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:27: note: previous declaration is here
10  221 |   _CRTIMP char ***__cdecl __p__environ(void);
11      |                           ^
12/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:3: note: previous attribute is here
13  221 |   _CRTIMP char ***__cdecl __p__environ(void);
14      |   ^
15/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/_mingw.h:52:40: note: expanded from macro '_CRTIMP'
16   52 | #      define _CRTIMP  __attribute__ ((__dllimport__))
17      |

and

0In file included from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/exec.cpp:8:
1/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:759:10: warning: private field 'parent_' is not used [-Wunused-private-field]
2  759 |   Popen* parent_ = nullptr;
3      |          ^
4/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:760:7: warning: private field 'err_wr_pipe_' is not used [-Wunused-private-field]
5  760 |   int err_wr_pipe_ = -1;
6      |       ^
7/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:1038:7: warning: private field 'child_pid_' is not used [-Wunused-private-field]
8 1038 |   int child_pid_ = -1;
9      |       ^

We should likely fix these as well - I don’t mind pushing a separate PR for this after the other one.

hebasto commented at 11:25 pm on October 7, 2025: member

I’ve submitted an issue to https://github.com/mstorsjo/llvm-mingw. @l0rinc

I have reproduced the error in l0rinc/bitcoin-core-nightly#2 with @hebasto’s nightly clone redirected to l0rinc/master.

I pushed a potential fix that adds a const char * overload to HasReason in l0rinc#43 - tested through l0rinc/bitcoin-core-nightly#1.

Also added an alternative which replaces all remaining const char * throws with std::runtime_error in l0rinc#44 - tested through l0rinc/bitcoin-core-nightly#3.

Based on your feedback and on what the CI gods think, I will push one of them as a PR.

I believe that this is also a good point. Because of array-to-pointer conversion, I’d lean to the second option you mentioned.

Also note that the windows build indicates additional warnings (unused vars because of ifndef __USING_WINDOWS__)

We should likely fix these as well - I don’t mind pushing a separate PR for this after the other one.

Yes. These warnings are unrelated to the current PR changes, and I’d prefer to see them fixed in separated PR(s).

l0rinc commented at 0:56 am on October 8, 2025: contributor

I’d lean to the second option you mentioned

Thanks, closed the other ones and opened #33569 - the dedicated nightly build passed: https://github.com/l0rinc/bitcoin-core-nightly/pull/3

I’d prefer to see them fixed in separated PR(s)

Pushed #33570 as well for extern char** environ - but the one in subprocess.h seems more involved, will leave that to someone else.

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

Code Coverage & Benchmarks

Reviews

Fix windows libc++ `fs::path` `fstream` compile errors #33550