[bugfix] Fix encoding issue for Windows #13426

ken2812221 · 2018-06-10T16:23:56Z

From #13107 (comment), @laanwj suggested that we should keep our internal strings to be always utf-8 encoded.

This PR adds two C++17 compatible macros: u8string and u8path. The reason that I use macros is that developers might not want to pass second parameter utf8 everytime when they call string or create path objects.

I tried to find all string methods and convert them to u8string except for external api calls. Also tried to convert explicit or implicit path to u8path. Tell me if you find something that I missed.

Required: bitcoin-core/leveldb-subtree#18

See #13787 for travis run result

Caution: The user must change their config file to be utf-8 encoded if the file contains any non-ascii characters in it. Before 0.18, they are read as ansi-encoded characters on Windows.

Empact · 2018-06-10T23:24:50Z

Can we use path.imbued.locale instead for this?
https://www.boost.org/doc/libs/1_64_0/libs/filesystem/doc/reference.html#path-imbued-locale
https://www.boost.org/doc/libs/1_64_0/libs/locale/doc/html/default_encoding_under_windows.html

ken2812221 · 2018-06-11T00:20:04Z

Can we use path.imbued.locale instead for this?

We still need to use ansi string when we call bdb and leveldb api, so I think that adding a new function is needed. Also, it needs to add boost::locale dependency in order to use boost locale generator.

theuni · 2018-06-12T20:27:27Z

I'm not really comfortable patching boost to make this work. Firstly because it would mean that only depends-builds can build for Windows, but also because I'm just not familiar with it.

Maybe start by upstreaming it and see where the discussion goes?

ken2812221 · 2018-06-12T20:32:57Z

Firstly because it would mean that only depends-builds can build for Windows

Is there any way to build Bitcoin Core for Windows without depends-builds?
Also, I created a PR boostorg/filesystem#75

achow101 · 2018-06-12T23:24:03Z

Is there any way to build Bitcoin Core for Windows without depends-builds?

Supposedly there is a way to build with MSVC and Visual Studio which doesn't use the depends system.

ken2812221 · 2018-06-13T00:22:09Z

I found a way not to patch boost library, it should work with msvc now.

DrahtBot · 2018-06-14T20:25:08Z

Note to reviewers: This pull request conflicts with the following ones:

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 by ken2812221)
utils: Make fs::path::string() always return utf-8 string on Windows #13877 (utils: Make fs::path::string() always return utf-8 string on Windows by ken2812221)
utils: Use _wfopen and _wfreopen on Windows #13866 (utils: Use _wfopen and _wreopen on Windows by ken2812221)
validation: Document where we are intentionally ignoring bool return values from validation related functions #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)
utils: drop boost::interprocess::file_lock #13862 (utils: drop boost::interprocess::file_lock by ken2812221)
Move src/tinyformat.h to src/tinyformat/tinyformat.h #13846 (Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact)
Include tinyformat as a subtree #13845 (Include tinyformat as a subtree by Empact)
util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
Docs: Create default bitcoin.conf file on startup #13761 (Docs: Create default bitcoin.conf file on startup by leishman)
wallet: "avoid_reuse" wallet flag for improved privacy #13756 (wallet: -avoidreuse feature for improved privacy by kallewoof)
Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
-masterdatadir for datadir bootstrapping #13746 (-masterdatadir for datadir bootstrapping by kallewoof)
gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows #13734 (gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows by ken2812221)
PSBT key path cleanups #13723 (PSBT key path cleanups by sipa)
bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords #13716 (bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords by kallewoof)
Remove the boost/algorithm/string/case_conv.hpp dependency #13671 (Remove the boost/algorithm/string/case_conv.hpp dependency by 251Labs)
wallet: Fix backupwallet for multiwallets #13667 (wallet: Fix backupwallet for multiwallets by domob1812)
[refactor] Fix the chainparamsbase -> util circular dependency #13639 ([refactor] Fix the chainparamsbase -> util circular dependency by Empact)
Check for datadir after the config files were read #13621 (Check for datadir after the config files were read by Flowdalic)
Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() #13389 (Utils and libraries: Fix -sysperms=false (default) doesn't appear to do anything #13371 - move umask operation earlier in AppInit() by n2yen)
Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249 (Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. by practicalswift)
Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) #13159 (Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) by practicalswift)
gui: Add dynamic wallets support #13100 (gui: Add dynamic wallets support by promag)
Log early messages with -printtoconsole #13088 (Log early messages with -printtoconsole by ajtowns)
macOS: disable AppNap during sync #12783 (macOS: Disable AppNap by krab)
Free BerkeleyEnvironment instances when not in use #11911 (Free CDBEnv instances when not in use by ryanofsky)
Add BitcoinApplication & RPCConsole tests #11625 (WIP: Add BitcoinApplication & RPCConsole tests by ryanofsky)
Refactor: separate wallet from node #10973 (Refactor: separate wallet from node by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Empact · 2018-06-15T21:49:31Z

src/util.cpp

@@ -85,6 +85,8 @@ const int64_t nStartupTime = GetTime();
 const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf";
 const char * const BITCOIN_PID_FILENAME = "bitcoind.pid";

+fs::detail::utf8_codecvt_facet g_utf8;


g_utf8 should be accessible globally.

Looks like the only use is in util.h, if you implement those as methods in util.cpp, declared in util.h, you can limit variable access to here util.cpp. I may be wrong though! Reviewing on a cellphone. :P

Empact · 2018-06-15T21:51:13Z

src/util.h

+extern fs::detail::utf8_codecvt_facet g_utf8;
+
+#define u8string() string(g_utf8)
+#define u8path(str) path(str, g_utf8)


Is it better to do these as methods in the namespace? Preprocessor use should be minimized IMO.

I just thought that pass the second parameter at every .string() and path() is pretty annoying and easy to forget. If most developers don't want to use macro function, I could do a scripted-diff.

I mean define these macros as functions in the fs namespace that call through as defined here.

We could include u8path into fs namespace, however for u8string we couldn't since it's a member function.

I'll defer to others as to whether this is worthwhile, but absent other options, I would make a function ala u8path for string as well, and hide g_utf8

ken2812221 · 2018-06-21T15:11:22Z

Rebased

ken2812221 · 2018-07-12T13:36:50Z

src/bitcoin-cli.cpp

+#ifdef WIN32
+        for (std::string& arg : args)
+            arg = AnsiToUtf8(arg);
+#endif


In order to solve this problem, there are three ways to do.

The first one is above, which might get ? if the user enter any character that can't present by user's code page.

The second one is using GetCommandLineW and CommandLineToArgvW to get command line options in type wchar_t, which can present more Unicode characters.

The third one is adding wmain to get command line in type wchar_t directly by "(w)main" function.

Which one is prefered?

I choose the second way

fanquake · 2018-07-21T04:57:57Z

@theuni We probably want your thoughts here again.

For reference nothing has happened at the upstream PR, however that is less relevant now that we aren't patching Boost.

Empact · 2018-08-03T04:43:28Z

If you call CommandLineToArgvW within gArgs.ParseParameters instead, you can avoid touching src/bitcoind.cpp and src/bench/bench_bitcoin.cpp, at least. There may be other opportunities for minimization.

ken2812221 · 2018-08-03T07:17:54Z

If you call CommandLineToArgvW within gArgs.ParseParameters

It would break the ParseParameters in tests if we do that.

ken2812221 · 2018-08-05T15:27:31Z

I'll seperate these into many PRs.

1661a47 add unicode compatible file_lock for Windows (Chun Kuan Lee) Pull request description: boost::interprocess::file_lock cannot open the files that contain characters which cannot be parsed by the user's code page on Windows. This PR is seperated from #13426 for easier review. Tree-SHA512: e240479cda65958bf6e1319840b83928b2b50da81d99f4f002fb3b62621370bcd4bcfacd2b8c0678c443a650d6ba53d9d12618b591e5bfd67ac14388a18fd822

laanwj added the Bug label Jun 11, 2018

ken2812221 mentioned this pull request Jun 11, 2018

Fix Windows locale problem #13107

Closed

ken2812221 changed the title ~~[WIP, bugfix] Add u8path and u8string to boost to fix #13103~~ [bugfix] Add u8path and u8string to boost to fix #13103 Jun 12, 2018

fanquake requested a review from theuni June 12, 2018 13:29

ken2812221 changed the title ~~[bugfix] Add u8path and u8string to boost to fix #13103~~ [bugfix] Add u8path and u8string to fix #13103 Jun 13, 2018

DrahtBot mentioned this pull request Jun 14, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

Empact reviewed Jun 15, 2018

View reviewed changes

DrahtBot mentioned this pull request Jun 19, 2018

gui: Add leading 0 to QT_VERSION check for QFontDatabase #13493

Closed

ghost approved these changes Jun 25, 2018

View reviewed changes

DrahtBot mentioned this pull request Jul 9, 2018

release: require macOS 10.10+ #13617

Merged

DrahtBot added the Needs rebase label Jul 12, 2018

ken2812221 commented Jul 12, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Jul 12, 2018

DrahtBot mentioned this pull request Jul 16, 2018

wallet: Fix backupwallet for multiwallets #13667

Merged

ken2812221 changed the title ~~[bugfix] Add u8path and u8string to fix #13103~~ [bugfix] Add u8path and u8string to fix encoding issue for Windows Jul 17, 2018

laanwj added this to the 0.17.0 milestone Jul 19, 2018

DrahtBot added the Needs rebase label Jul 20, 2018

fanquake removed the Needs rebase label Jul 21, 2018

This was referenced Jul 22, 2018

PSBT key path cleanups #13723

Merged

bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords #13716

Merged

ken2812221 added 6 commits July 31, 2018 08:24

Make FileLock support utf8 for Windows

c3e0340

Use _wfopen and _wreopen on Windows

2d86754

Make leveldb works with utf8

087792e

Use Unicode version of MoveFileEx

4723aed

Add fsbridge::i/ofstream class

014aa3e

Change fs/std::i/ofstream to fsbridge::i/ofstream

2fa7d42

DrahtBot mentioned this pull request Jul 31, 2018

Docs: Create default bitcoin.conf file on startup #13761

Closed

ken2812221 mentioned this pull request Aug 1, 2018

gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows #13734

Merged

This was referenced Aug 1, 2018

Include tinyformat as a subtree #13842

Closed

Inline i64tostr and itostr #13840

Closed

Include tinyformat as a subtree #13845

Closed

Move src/tinyformat.h to src/tinyformat/tinyformat.h #13846

Closed

laanwj modified the milestones: 0.17.0, 0.18.0 Aug 2, 2018

laanwj added the Windows label Aug 2, 2018

DrahtBot mentioned this pull request Aug 2, 2018

util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815

Merged

DrahtBot mentioned this pull request Aug 3, 2018

wallet: "avoid_reuse" wallet flag for improved privacy #13756

Merged

ken2812221 mentioned this pull request Aug 3, 2018

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

Merged

DrahtBot mentioned this pull request Aug 3, 2018

validation: Document where we are intentionally ignoring bool return values from validation related functions #13864

Closed

ken2812221 mentioned this pull request Aug 3, 2018

Filename and command line encoding issue on Windows #13869

Closed

12 tasks

This was referenced Aug 3, 2018

utils: Use _wfopen and _wfreopen on Windows #13866

Merged

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

Merged

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

Merged

ken2812221 closed this Aug 5, 2018

ken2812221 deleted the u8path_u8string branch August 6, 2018 02:49

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

[bugfix] Fix encoding issue for Windows #13426

[bugfix] Fix encoding issue for Windows #13426

Uh oh!

Conversation

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

Uh oh!

Caution: The user must change their config file to be utf-8 encoded if the file contains any non-ascii characters in it. Before 0.18, they are read as ansi-encoded characters on Windows.

Uh oh!

Empact commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Jun 11, 2018

Uh oh!

theuni commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

achow101 commented Jun 12, 2018

Uh oh!

ken2812221 commented Jun 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

Empact Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Jun 15, 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!

Empact Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Jun 21, 2018

Uh oh!

ken2812221 Jul 12, 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 Jul 26, 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!

fanquake commented Jul 21, 2018

Uh oh!

Empact commented Aug 3, 2018

Uh oh!

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

Uh oh!

Uh oh!

ken2812221 commented Aug 5, 2018

Uh oh!

Uh oh!

ken2812221 commented Jun 10, 2018 •

edited

Loading

Empact commented Jun 10, 2018 •

edited

Loading

ken2812221 commented Jun 12, 2018 •

edited

Loading

DrahtBot commented Jun 14, 2018 •

edited

Loading

ken2812221 Jun 15, 2018 •

edited

Loading

ken2812221 Jul 12, 2018 •

edited

Loading

ken2812221 Jul 26, 2018 •

edited

Loading

ken2812221 commented Aug 3, 2018 •

edited

Loading