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

ken2812221 · 2018-07-21T22:24:11Z

Drop boost::scoped_array and simplify the code.

TCHAR should be defined as wchar_t if UNICODE is defined. So we can use .toStdWString().c_str() to get wchar_t C-style string.

Fix #13819

fanquake · 2018-07-22T04:08:10Z

This was introduced in #5793. Looks like something similar to what you are doing was proposed at the time as well.

ken2812221 · 2018-07-22T04:47:27Z

Hope there is a way to see code that is before he force push.

practicalswift · 2018-07-22T05:42:06Z

Concept ACK

Thanks for removing Boost dependencies. Keep it coming! :-)

maflcko · 2018-07-22T12:37:17Z

Any suggestion on how to test this?

ken2812221 · 2018-07-22T13:50:32Z

This is Windows-only code, can be tested as same as #5793 (comment)

Sjors

Should the reference also be removed from test/lint/lint-includes.sh?

Tested that make still works on macOS.

Tested the gitian binary on a Windows 10 VM, by launching with bitcoin-qt -wallet=你好, assuming that was the problem? This leads to a crash, but that's also the case on master:

I have a version from earlier this year where it leads to a slightly nicer crash:

(seems unrelated, so I made a ticket #13754)

bitcoin-qt -wallet=test works fine by the way, maybe that was all.

Sjors · 2018-07-24T15:35:22Z

src/qt/guiutil.cpp

@@ -625,7 +615,7 @@ bool SetStartOnSystemStartup(bool fAutoStart)
 #ifndef UNICODE
            psl->SetArguments(strArgs.toStdString().c_str());
 #else


Add a comment that UNICODE is only defined for Windows? (if that's the case)

ken2812221 · 2018-07-29T11:19:09Z

Update: Since the goal is to make it unicode compatible, just make it use wchar_t version of api explicitly.

ken2812221 · 2018-07-29T11:25:07Z

src/qt/guiutil.cpp

            reinterpret_cast<void**>(&psl));

        if (SUCCEEDED(hres))
        {
            // Get the current executable path
-            TCHAR pszExePath[MAX_PATH];
-            GetModuleFileName(nullptr, pszExePath, sizeof(pszExePath));


This is bug before, if TCHAR is wchar_t, then sizeof(pszExePath)= 2*MAX_PATH, which can result memory overflow.

Sjors · 2018-07-30T13:23:00Z

Tested that make still works on macOS with 5d70ab0. I have no opinion on the code itself, other than it seems good to keep making progress here: https://github.com/bitcoin/bitcoin/projects/3#card-205363

maflcko · 2018-07-31T14:15:55Z

Tested that for commit 6031f7f (master and this pull) I can launch regtest and testnet after re-login for a user with only ascii character in the name.

(See lower left of screenshot, Compare to #5793 (comment))

ken2812221 · 2018-07-31T15:20:00Z

@MarcoFalke Can you test this again? Now it should fix #13819.

maflcko · 2018-08-01T15:36:58Z

Now it fails for commit d41914b (master and this pull) that it can not write to the directory:

(though the folders are created, as can be seen in the explorer in the background)

ken2812221 · 2018-08-01T15:52:41Z

@MarcoFalke That should fixed by #13426. You can merge these two PR and test locally.

Empact · 2018-08-03T04:22:15Z

src/qt/guiutil.cpp

            reinterpret_cast<void**>(&psl));

        if (SUCCEEDED(hres))
        {
            // Get the current executable path
-            TCHAR pszExePath[MAX_PATH];
-            GetModuleFileName(nullptr, pszExePath, sizeof(pszExePath));
+            wchar_t pszExePath[MAX_PATH];


I think safer to use WCHAR here, as I'm seeing differing descriptions of its definition, e.g. unsigned wchar_t here: https://docs.microsoft.com/en-us/windows/desktop/intl/windows-data-types-for-strings

Empact · 2018-08-03T04:23:04Z

src/util.cpp

@@ -1118,9 +1118,9 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length) {
 #ifdef WIN32
 fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
 {
-    char pszPath[MAX_PATH] = "";
+    wchar_t pszPath[MAX_PATH] = L"";


Same, WCHAR seems preferable.

laanwj · 2018-09-11T08:57:46Z

nice cleanup, too

utACK bb6ca65

…citly on Windows bb6ca65 gui: get special folder in unicode (Chun Kuan Lee) 1c5d225 Drop boost::scoped_array (Chun Kuan Lee) Pull request description: Drop boost::scoped_array and simplify the code. `TCHAR` should be defined as `wchar_t` if `UNICODE` is defined. So we can use `.toStdWString().c_str()` to get wchar_t C-style string. Fix #13819 Tree-SHA512: 3fd4aa784129c9d1576b01e6ee27faa42d793e152d132f2dde504d917dad3a8e95e065fcbc54a3895d74fb6b2a9ed4f5ec67d893395552f585e225486a84a454

Drop boost::scoped_array and simplify the code. TCHAR should be defined as wchar_t if UNICODE is defined. So we can use .toStdWString().c_str() to get wchar_t C-style string. Backports bitcoin#13734

Summary: * Drop boost::scoped_array * gui: get special folder in unicode This is a backport of Core [[bitcoin/bitcoin#13734 | PR13734]] Test Plan: make check Run bitcoin-qt on win64 Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5889

Backport Boost removal PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7613 - bitcoin/bitcoin#10502 - bitcoin/bitcoin#10193 - bitcoin/bitcoin#13961 - bitcoin/bitcoin#13734 - Only the second commit (we don't need the first). - bitcoin/bitcoin#14480 Part of #4819.

e2fee5c [GUI] Use wchar_t API explicitly on Windows (Fuzzbawls) Pull request description: backport of bitcoin#13734 to use the wchar API for windows startup function. This is the first in a long list of upstream PRs that aim to bring full Unicode support to Windows clients as detailed in bitcoin#13869 Also included here is the relevant parts of bitcoin#5793, to use per-network auto startup shortcuts. ACKs for top commit: furszy: utACK e2fee5c . random-zebra: utACK e2fee5c and merging... Tree-SHA512: dc4ea84ee10199bdf293977f00c09caf65cbdd1ee2ae14b1adb34fd75c05b847d5971abd17732a77be88bed9e06092b670b09c5760a686b29d64ba8f02d2e4b2

…I explicitly on Windows bb6ca65 gui: get special folder in unicode (Chun Kuan Lee) 1c5d225 Drop boost::scoped_array (Chun Kuan Lee) Pull request description: Drop boost::scoped_array and simplify the code. `TCHAR` should be defined as `wchar_t` if `UNICODE` is defined. So we can use `.toStdWString().c_str()` to get wchar_t C-style string. Fix bitcoin#13819 Tree-SHA512: 3fd4aa784129c9d1576b01e6ee27faa42d793e152d132f2dde504d917dad3a8e95e065fcbc54a3895d74fb6b2a9ed4f5ec67d893395552f585e225486a84a454 # Conflicts: # src/qt/guiutil.cpp

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

maflcko added the GUI label Jul 22, 2018

maflcko changed the title ~~Drop boost::scoped_array~~ gui: Drop boost::scoped_array Jul 22, 2018

maflcko added the Needs gitian build label Jul 22, 2018

DrahtBot removed the Needs gitian build label Jul 22, 2018

Sjors reviewed Jul 24, 2018

View reviewed changes

ken2812221 changed the title ~~gui: Drop boost::scoped_array~~ gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows Jul 29, 2018

ken2812221 commented Jul 29, 2018

View reviewed changes

bitcoin deleted a comment from DrahtBot Jul 29, 2018

maflcko added Windows Needs gitian build labels Jul 29, 2018

DrahtBot removed the Needs gitian build label Jul 29, 2018

maflcko requested a review from jonasschnelli July 31, 2018 14:26

maflcko added the Needs gitian build label Jul 31, 2018

maflcko added this to the 0.17.0 milestone Jul 31, 2018

DrahtBot removed the Needs gitian build label Aug 1, 2018

maflcko modified the milestones: 0.17.0, 0.18.0 Aug 1, 2018

Empact reviewed Aug 3, 2018

View reviewed changes

ken2812221 reopened this Aug 21, 2018

bitcoin deleted a comment from DrahtBot Aug 21, 2018

laanwj merged commit bb6ca65 into bitcoin:master Sep 11, 2018

ken2812221 deleted the drop-boost-scoped-array branch September 11, 2018 12:08

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

str4d mentioned this pull request Nov 23, 2020

Backport Boost removal PRs zcash/zcash#4877

Merged

Fuzzbawls mentioned this pull request Jan 30, 2021

[GUI] Use wchar_t API explicitly on Windows PIVX-Project/PIVX#2174

Merged

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 Jul 13, 2021

merge bitcoin#13734: Drop boost::scoped_array and use wchar_t API

000fdcb

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#13734: Drop boost::scoped_array and use wchar_t API

98ae047

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#13734: Drop boost::scoped_array and use wchar_t API

0e863d1

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

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 #13734

Uh oh!

Conversation

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

Uh oh!

Uh oh!

fanquake commented Jul 22, 2018

Uh oh!

ken2812221 commented Jul 22, 2018

Uh oh!

practicalswift commented Jul 22, 2018

Uh oh!

maflcko commented Jul 22, 2018

Uh oh!

ken2812221 commented Jul 22, 2018

Uh oh!

Sjors left a comment • 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!

Sjors Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Jul 29, 2018

Uh oh!

ken2812221 Jul 29, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jul 30, 2018

Uh oh!

maflcko commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Jul 31, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Empact Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

ken2812221 commented Jul 21, 2018 •

edited

Loading

Sjors left a comment •

edited

Loading

maflcko commented Jul 31, 2018 •

edited

Loading

maflcko commented Aug 1, 2018 •

edited

Loading

ken2812221 commented Aug 1, 2018 •

edited

Loading

laanwj commented Sep 11, 2018 •

edited

Loading