Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Oct 18, 2018

Conversation

Projects
None yet
6 participants
@ken2812221
Copy link
Member

commented Aug 4, 2018

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.

@ken2812221 ken2812221 referenced this pull request Aug 4, 2018

Closed

Filename and command line encoding issue on Windows #13869

11 of 12 tasks complete

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Aug 4, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14372 (msvc: build secp256k1 and leveldb locally by ken2812221)
  • #14303 (rpc: Early call once CWallet::MarkDirty in import calls by promag)
  • #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.

@ken2812221 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

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Aug 15, 2018

@alexeyneu

This comment has been minimized.

Copy link

commented Aug 15, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

This is for mingw

@DrahtBot DrahtBot removed the Needs rebase label Aug 15, 2018

@alexeyneu

This comment has been minimized.

Copy link

commented Aug 15, 2018

untitled 1
@ken2812221 unexpected statement

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

@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

This comment has been minimized.

Copy link

commented Aug 16, 2018

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

This comment has been minimized.

Copy link

commented Aug 16, 2018

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

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Aug 24, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Aug 29, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 29, 2018

@laanwj laanwj added this to Blockers in High-priority for review Aug 30, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Aug 31, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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.

src/fs.h Outdated
@@ -38,6 +42,41 @@ namespace fsbridge {
void* hFile = (void*)-1; // INVALID_HANDLE_VALUE
#endif
};


#if defined WIN32 && defined __GNUC__

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 5, 2018

Contributor

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

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

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.

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch Sep 5, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:iofstream-custom branch 3 times, most recently Sep 13, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 13, 2018

@ryanofsky
Copy link
Contributor

left a comment

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.

src/fs.h Outdated
// 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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 14, 2018

Contributor

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.


BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(fsbridge_fstream)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 14, 2018

Contributor

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?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 14, 2018

Author Member

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

@ryanofsky
Copy link
Contributor

left a comment

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

src/fs.h Outdated
// 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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 21, 2018

Contributor

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.

src/fs.h Outdated
{
public:
ifstream() = default;
ifstream(const fs::path& p, std::ios_base::openmode mode = std::ios_base::in) { open(p, mode); }

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 23, 2018

Member
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]
src/fs.h Outdated
{
public:
ofstream() = default;
ofstream(const fs::path& p, std::ios_base::openmode mode = std::ios_base::out) { open(p, mode); }

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 23, 2018

Member
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 ken2812221 force-pushed the ken2812221:iofstream-custom branch to 43c7fbb Sep 26, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

Rebased and fix all nits from the comment

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 2, 2018

Member

Add missing newline at end of file :-)

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 3, 2018

Author Member

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

utACK 86eb3b3

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #13878: utils: Add fstream wrapper to allow to pass unicode fil…
…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

@laanwj laanwj removed this from Blockers in High-priority for review Oct 18, 2018

@ken2812221 ken2812221 deleted the ken2812221:iofstream-custom branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.