utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

ken2812221 · 2018-08-04T16:49:34Z

If compiled with mingw, use glibc++ extension stdio_filebuf to open the file by FILE* instead of filename.

In other condition, we can use boost::fstream.

DrahtBot · 2018-08-04T20:12:42Z

Reviewers, this pull request conflicts with the following ones:

msvc: build secp256k1 and leveldb locally #14372 (msvc: build secp256k1 and leveldb locally by ken2812221)
rpc: Early call once CWallet::MarkDirty in import calls #14303 (rpc: Early call once CWallet::MarkDirty in import calls by promag)
windows: Fix remaining compiler warnings (MSVC) #14151 (windows: Fix remaining compiler warnings (MSVC) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

alexeyneu · 2018-08-15T17:12:00Z

but there's override:

#include <fstream>
int main(int argc, char* argv[])
{	
	std::fstream tr(L"hk");
	return 0;
}

compiles fine.
( msys2 )

c++ json_c.cpp -lws2_32 -static-libgcc -static-libstdc++ -static-libgcc -static-libstdc++ -Wl,-Bstatic -lstdc++ -lpthread

ken2812221 · 2018-08-15T17:26:20Z

This is for mingw

alexeyneu · 2018-08-15T19:10:41Z

@ken2812221 unexpected statement

ken2812221 · 2018-08-16T02:55:27Z

@alexeyneu To be clear, this is for the mingw on Ubuntu 18.04 which is what we use for gitian to build release binaries. If you can test this on Ubuntu, that would be great.

alexeyneu · 2018-08-16T10:32:25Z

msys2 .h files are nothing more then msvc copy-paste. it may not work but linux edition has same headers .
i've used it in msvc with np. this applies to constructor only. this stuff isn't avaible for
.open() and others. last time i've seen there smth own-written was windows edition of watcom c .

alexeyneu · 2018-08-16T23:39:39Z

Which is to say, i dont see much sense in testing it coz it will work .
it's not my PR anyway.

ryanofsky · 2018-09-05T21:27:48Z

I don't think I understand how this PR works (does it depend on #13866?), or why reimplementing fstream classes is a good solution compared to alternatives. If this is a bug in boost, wouldn't it be better to fix the bug upstream and maybe patch it locally? Or could we drop the use of boost here and instead use std::ifstream and std::ofstream?

It would be helpful to have a clear description of the bug and possible workarounds.

ryanofsky · 2018-09-05T21:36:57Z

src/fs.h

@@ -38,6 +42,41 @@ namespace fsbridge {
        void* hFile = (void*)-1; // INVALID_HANDLE_VALUE
 #endif
    };
+
+
+#if defined WIN32 && defined __GNUC__


Should this be checking for __GLIBCXX__ instead of __GNUC__? It looks like __gnu_cxx::stdio_filebuf might be present in libstdc++ but not libc++ (https://stackoverflow.com/questions/22624503/how-to-get-a-file-descriptor-from-a-stdbasic-ios-for-clang-on-os-x).

Will different workarounds be needed for MSVC and clang?

ken2812221 · 2018-09-05T23:18:32Z

boost::filesystem::i/ofstream calls std::filebuf internally. So if std::filebuf(wchar_t*) exist, boost will call that function (MSVC). Otherwise it will call std::filebuf(char*) which can lead to encoding issue. After c++17, we can use std::i/ofstream(std::filesystem::path). But before then, we can't.

If building with libstdc++, we can use its extension stdio_filebuf by passing FILE *.

Will different workarounds be needed for MSVC and clang?

I don't know about clang for Windows. But for MSVC, we can use boost::filesystem::i/ofstream.

does it depend on #13866?

Without #13866, it would work. But to solve the encoding issue, it requires #13866.

ryanofsky

utACK f2b00f16cc5ca12fa0cabc2bedfbf7e0ba094dfa, but only if #13866 is merged first, since this change by itself doesn't fix anything or really make sense without #13866.

ryanofsky · 2018-09-06T20:59:13Z

src/fs.cpp

+        fclose(file);
+    }
+    file = nullptr;
+}


I think it would be good to add a static assert here to detect if this problem happens with other libraries or versions of libraries:

#if defined WIN32 && defined __GLIBCXX__ ... #elif WIN32 static_assert(sizeof(*fs::path().BOOST_FILESYSTEM_C_STR) == sizeof(wchar_t), "Warning: This build is using boost::filesystem ofstream and ifstream " "implementations which will fail to open paths containing multibyte " "characters. You should delete this static_assert to ignore this warning, " "or switch to a different C++ standard library like the Microsoft C++ " "Standard Library (where boost uses non-standard extensions to construct " "stream objects with wide filenames), or the GNU libstdc++ library (where " "a more complicated workaround has been implemented above)."); #endif

src/fs.h

ryanofsky · 2018-09-06T22:09:57Z

src/fs.cpp

+/**
+*  reference: https://github.com/gcc-mirror/gcc/blob/571ee70a6d4226145f77b885937a8eda678e0fb2/libstdc%2B%2B-v3/include/std/fstream#L283
+*
+*  +---------------------------------------------------------+


I think I would drop this table but keep the URL link, since the table just duplicates information provided by the code below, and seems actually less readable than the code.

For reference, https://en.cppreference.com/w/cpp/io/basic_filebuf/open also has a similar table.

ryanofsky · 2018-09-06T22:15:20Z

src/fs.cpp

+void ifstream::open(const fs::path& p, std::ios_base::openmode mode)
+{
+    close();
+    file = fopen(p, openmodeToStr(mode).c_str());


I don't think it's obvious that this is intended to call the fsbridge::fopen function above rather than ::fopen. Would suggesting adding a comment here or writing the namespace.

Could also do the same in ofstream::open below.

ryanofsky · 2018-09-06T22:16:50Z

src/fs.cpp

+    if (file == nullptr) {
+        return;
+    }
+    filebuf = std::move(__gnu_cxx::stdio_filebuf<char>(file, mode));


std::move can be dropped here. Argument is already an rvalue so it has no effect.

Same applies in ofstream::open below.

ryanofsky · 2018-09-06T22:19:47Z

src/fs.h

+        void close();
+
+    private:
+        __gnu_cxx::stdio_filebuf<char> filebuf;


Could you prefix these members with m_ to match recommended style?

src/fs.cpp

ken2812221 · 2018-09-07T22:58:31Z

@ryanofsky requested change included and unit tests added.

ryanofsky

utACK 6b6c598ab55c60ed5050c0ba0a817dba899384c0. Changes since previous review were various suggestions like adding static_assert, m_ prefixes, comments, and new tests. Thanks especially for the new tests.

ryanofsky · 2018-09-14T18:32:17Z

src/fs.h

+// ifstream/ofstream `wchar_t` constructors, and the GNU library doesn't
+// provide them (in contrast to the Microsoft C++ library, see
+// https://stackoverflow.com/questions/821873/how-to-open-an-stdfstream-ofstream-or-ifstream-with-a-unicode-filename/822032#822032),
+// Boost is forced to fall back to `char` APIs which may not work properly.


In commit "utils: Add fsbridge fstream function wrapper" (0548f48f19d33cb57b8279d7a647e91d375a916e)

I know this is my own comment, maybe replace "APIs" with "constructors" on this to make clearer this is referring to the same constructors previously mentioned above.

ryanofsky · 2018-09-14T18:34:03Z

src/test/fs_tests.cpp

+
+BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(fsbridge_fstream)


In commit "tests: Add test case for std::ios_base::ate" (6b6c598ab55c60ed5050c0ba0a817dba899384c0)

Would it be possible to add a test accessing a utf8 filesystem path now that #13866 is merged?

Sure. But this seems only work if #13877 merged, so change this PR to based on it.

ryanofsky

utACK b11cf20f8255bf20fc3c80117308fcf77424545a. Only changes since last review were tweaking a comment and updating the tests to use utf8 paths for better coverage. Making the test work required #13877, so that PR is now included here (and hopefully can be merged first).

ryanofsky · 2018-09-21T18:40:17Z

src/fs.h

+    // GNU libstdc++ specific workaround for opening UTF-8 paths on Windows.
+    //
+    // On Windows, it is only possible to reliably access multibyte file paths through
+    // `wchar_t` constructors, not `char` APIs. But because the C++ standard doesn't


Word "constructors" on this line should be changed back to "APIs", because this is just referring in general to how multibyte paths need to be accessed on windows, not to stream object constructors in particular.

practicalswift · 2018-09-23T08:10:15Z

src/fs.h

+    {
+    public:
+        ifstream() = default;
+        ifstream(const fs::path& p, std::ios_base::openmode mode = std::ios_base::in) { open(p, mode); }


2018-09-22 21:43:16 cpplint(pr=13878): src/fs.h:65: Constructors callable with one argument should be marked explicit. [runtime/explicit] [5]

practicalswift · 2018-09-23T08:10:27Z

src/fs.h

+    {
+    public:
+        ofstream() = default;
+        ofstream(const fs::path& p, std::ios_base::openmode mode = std::ios_base::out) { open(p, mode); }


2018-09-22 21:43:16 cpplint(pr=13878): src/fs.h:79: Constructors callable with one argument should be marked explicit. [runtime/explicit] [5]

ken2812221 · 2018-09-26T00:49:29Z

Rebased and fix all nits from the comment

practicalswift · 2018-10-02T05:18:49Z

build_msvc/common.vcxproj

+    <ClCompile>
+      <AdditionalOptions>/utf-8 %(AdditionalOptions)</AdditionalOptions>
+    </ClCompile>
+  </ItemDefinitionGroup>
 </Project>


Add missing newline at end of file :-)

That seems unnecessary for MSVC project file, they can work without the extra newline. But you can add it in your PR. I don't have strong opinion.

laanwj · 2018-10-18T08:35:52Z

utACK 86eb3b3

…ename on Windows 43c7fbb Make MSVC compiler read the source code using utf-8 (Chun Kuan Lee) f86a571 tests: Add test case for std::ios_base::ate (Chun Kuan Lee) a554cc9 Move boost/std fstream to fsbridge (Chun Kuan Lee) 86eb3b3 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee) Pull request description: If compiled with mingw, use glibc++ extension `stdio_filebuf` to open the file by `FILE*` instead of filename. In other condition, we can use boost::fstream. Tree-SHA512: b5dbd83e347fb9b2a0c8b1c2c7bd71a272e839ec0617883b2a0ec12506ae9e825373cf6e95b9bcc91d7edc85bf51580a7716b56a9ecaad776bc3ae61638cb3da

Backports bitcoin#13878

Summary: ``` If compiled with mingw, use glibc++ extension stdio_filebuf to open the file by FILE* instead of filename. In other condition, we can use boost::fstream. ``` Backport of core [[bitcoin/bitcoin#13878 | PR13878]] and [[bitcoin/bitcoin#15468 | PR15468]] (related bug fix). Depends on D5539. Test Plan: ninja check make check Run the Gitian builds. Build and run for Windows. Run `test_bitcoin` on Windows. Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5539

…on Windows

merge bitcoin#14209, bitcoin#13878, bitcoin#14192, bitcoin#17086: logging and filesystem updates

63e0be6 [Remove] By-pass logprint-scanner restriction. (furszy) 280ced3 utils: Fix broken Windows filelock (Chun Kuan Lee) be89860 utils: Convert Windows args to utf-8 string (Chun Kuan Lee) e8cfa6e Call unicode API on Windows (Chun Kuan Lee) 1a02a8a tests: Add test case for std::ios_base::ate (Chun Kuan Lee) 2e57cd4 Move boost/std fstream to fsbridge (furszy) 9d8bcd4 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee) d59d48d utils: Convert fs error messages from multibyte to utf-8 (ken2812221) 9ef58cc Logging: use "fmterr" variable name for errors instead of general "e" that can be used by any other function. (furszy) dd94241 utils: Use _wfopen and _wreopen on Windows (Chun Kuan Lee) 3993641 add unicode compatible file_lock for Windows (Chun Kuan Lee) 48349f8 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: As the software is currently using the ANSI encoding on Windows, the user's language settings could affect the proper functioning of the node/wallet, to the point of not be able to open some non-ASCII name files and directories. This solves the Windows encoding issues, completing the entire bitcoin#13869 work path (and some other required backports). Enabling for example users that use non-english characters in directories and file names to be accepted. Backported PRs: * bitcoin#12422. * bitcoin#12630. * bitcoin#13862. * bitcoin#13866. * bitcoin#13877. * bitcoin#13878. * bitcoin#13883. * bitcoin#13884. * bitcoin#13886. * bitcoin#13888. * bitcoin#14192. * bitcoin#13734. * bitcoin#14426. This is built on top of other two PRs that i have open #2423 and #2369. Solves old issues #940 and #2163. TODO: * Backport `assert_start_raises_init_error` and `ErrorMatch` in TestNode` (bitcoin#12718) ACKs for top commit: Fuzzbawls: ACK 63e0be6 random-zebra: ACK 63e0be6 and merging... Tree-SHA512: cb1f7c23abb5b7b3af50bba18652cc2cad93fd7c2fca9c16ffd3fee34c4c152a3b666dfa87fe6b44c430064dcdee4367144dcb4a41203c91b0173b805bdb3d7d

ken2812221 mentioned this pull request Aug 4, 2018

Filename and command line encoding issue on Windows #13869

Closed

12 tasks

This was referenced Aug 4, 2018

utils: drop boost::interprocess::file_lock #13862

Merged

Test for Windows encoding issue #13787

Merged

Check for datadir after the config files were read #13621

Closed

[bugfix] Fix encoding issue for Windows #13426

Closed

ken2812221 changed the title ~~utils: Add fstream wrapper to allow to pass unicode filename~~ utils: Add fstream wrapper to allow to pass unicode filename on Windows Aug 5, 2018

laanwj added Windows Utils/log/libs labels Aug 6, 2018

DrahtBot mentioned this pull request Aug 15, 2018

refactor: use fs:: over boost::filesystem:: #13982

Merged

DrahtBot added the Needs rebase label Aug 15, 2018

DrahtBot removed the Needs rebase label Aug 15, 2018

DrahtBot added Needs rebase and removed Needs rebase labels Aug 29, 2018

ryanofsky reviewed Sep 5, 2018

View reviewed changes

ryanofsky approved these changes Sep 6, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 10, 2018

utils: Convert fs::filesystem_error messages from local multibyte to utf-8 on Windows #14192

Merged

DrahtBot added Needs rebase and removed Needs rebase labels Sep 13, 2018

ryanofsky approved these changes Sep 14, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 14, 2018

windows: Fix remaining compiler warnings (MSVC) #14151

Merged

DrahtBot mentioned this pull request Sep 14, 2018

gui: Add GUIUtil::bringToFront #14123

Merged

ryanofsky mentioned this pull request Sep 21, 2018

utils: Make fs::path::string() always return utf-8 string on Windows #13877

Merged

ryanofsky reviewed Sep 21, 2018

View reviewed changes

practicalswift reviewed Sep 23, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 24, 2018

rpc: Early call once CWallet::MarkDirty in import calls #14303

Closed

ken2812221 added 4 commits September 26, 2018 08:47

utils: Add fsbridge fstream function wrapper

86eb3b3

Move boost/std fstream to fsbridge

a554cc9

tests: Add test case for std::ios_base::ate

f86a571

Make MSVC compiler read the source code using utf-8

43c7fbb

practicalswift reviewed Oct 2, 2018

View reviewed changes

DrahtBot mentioned this pull request Oct 2, 2018

msvc: build secp256k1 and leveldb locally #14372

Merged

laanwj merged commit 43c7fbb into bitcoin:master Oct 18, 2018

laanwj mentioned this pull request Oct 18, 2018

wallet: Add ListWalletDir utility function #14291

Merged

ken2812221 deleted the iofstream-custom branch October 18, 2018 08:41

Warrows added a commit to Warrows/PIVX that referenced this pull request Oct 14, 2019

[Utils] Use fstream wrapper to allow to pass unicode filename on Windows

1d90b06

Backports bitcoin#13878

Warrows added a commit to Warrows/PIVX that referenced this pull request Nov 23, 2019

[Utils] Use fstream wrapper to allow to pass unicode filename on Windows

9a4b384

Backports bitcoin#13878

kwvg added a commit to kwvg/dash that referenced this pull request Jun 15, 2021

bitcoin#13878: Add fstream wrapper to allow to pass unicode filename …

16fe512

…on Windows

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

bitcoin#13878: Add fstream wrapper to allow to pass unicode filename …

6a7202d

…on Windows

furszy mentioned this pull request Jun 24, 2021

Solve filename and command line encoding issues on Windows PIVX-Project/PIVX#2449

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 24, 2021

bitcoin#13878: Add fstream wrapper to allow to pass unicode filename …

93d7eca

…on Windows

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Jun 25, 2021

Merge pull request #4199 from kittywhiskers/fstreamWrapper

e887596

merge bitcoin#14209, bitcoin#13878, bitcoin#14192, bitcoin#17086: logging and filesystem updates

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)

		BOOST_AUTO_TEST_CASE(fsbridge_fstream)

utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

Uh oh!

Conversation

ken2812221 commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeyneu commented Aug 15, 2018

Uh oh!

ken2812221 commented Aug 15, 2018

Uh oh!

alexeyneu commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Aug 16, 2018

Uh oh!

alexeyneu commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeyneu commented Aug 16, 2018

Uh oh!

ryanofsky commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ken2812221 commented Sep 7, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 26, 2018

Uh oh!

practicalswift Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 18, 2018

Uh oh!

Uh oh!

ken2812221 commented Aug 4, 2018 •

edited

Loading

DrahtBot commented Aug 4, 2018 •

edited

Loading

alexeyneu commented Aug 15, 2018 •

edited

Loading

alexeyneu commented Aug 16, 2018 •

edited

Loading

ken2812221 commented Sep 5, 2018 •

edited

Loading

practicalswift Oct 2, 2018 •

edited

Loading

ken2812221 Oct 3, 2018 •

edited

Loading