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

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

Merged
merged 2 commits into from Sep 11, 2018

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Jul 21, 2018

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

@MarcoFalke MarcoFalke added the GUI label Jul 22, 2018
@MarcoFalke MarcoFalke changed the title Drop boost::scoped_array gui: Drop boost::scoped_array Jul 22, 2018
@fanquake fanquake added this to In progress in Boost → C++11 migration Jul 22, 2018
@fanquake
Copy link
Member

@fanquake fanquake commented Jul 22, 2018

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

@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Jul 22, 2018

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

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 22, 2018

Concept ACK

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 22, 2018

Any suggestion on how to test this?

@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Jul 22, 2018

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

Copy link
Member

@Sjors Sjors left a comment

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:

schermafbeelding 2018-07-24 om 17 43 26

I have a version from earlier this year where it leads to a slightly nicer crash:
schermafbeelding 2018-07-24 om 17 46 37

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

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

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

@Sjors Sjors Jul 24, 2018

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

@ken2812221 ken2812221 force-pushed the drop-boost-scoped-array branch from 0643053 to 5d70ab0 Jul 29, 2018
@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Jul 29, 2018

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

@ken2812221 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
reinterpret_cast<void**>(&psl));

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

@ken2812221 ken2812221 Jul 29, 2018

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

@Sjors
Copy link
Member

@Sjors Sjors commented Jul 30, 2018

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 31, 2018

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

screenshot from 2018-07-31 10-13-36

@MarcoFalke MarcoFalke requested a review from jonasschnelli Jul 31, 2018
@ken2812221 ken2812221 force-pushed the drop-boost-scoped-array branch from 499ff78 to cefd242 Jul 31, 2018
@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Jul 31, 2018

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 1, 2018

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)

screenshot from 2018-08-01 11-35-25

@MarcoFalke MarcoFalke removed this from the 0.17.0 milestone Aug 1, 2018
@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Aug 1, 2018
@bitcoin bitcoin deleted a comment from DrahtBot Aug 21, 2018
@bitcoin bitcoin deleted a comment from DrahtBot Aug 21, 2018
@laanwj
Copy link
Member

@laanwj laanwj commented Sep 11, 2018

nice cleanup, too

utACK bb6ca65

@laanwj laanwj merged commit bb6ca65 into bitcoin:master Sep 11, 2018
2 checks passed
laanwj added a commit that referenced this issue Sep 11, 2018
…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
@ken2812221 ken2812221 deleted the drop-boost-scoped-array branch Sep 11, 2018
@fanquake fanquake moved this from In progress to Done in Boost → C++11 migration Sep 13, 2018
Warrows added a commit to Warrows/PIVX that referenced this issue Oct 14, 2019
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
Warrows added a commit to Warrows/PIVX that referenced this issue Nov 23, 2019
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
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Apr 29, 2020
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
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this issue Aug 17, 2020
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
zkbot added a commit to zcash/zcash that referenced this issue Dec 4, 2020
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.
random-zebra added a commit to random-zebra/PIVX that referenced this issue Feb 6, 2021
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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 7, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 8, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 9, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 11, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 12, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 12, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 12, 2021
…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
Munkybooty added a commit to Munkybooty/dash that referenced this issue Jul 13, 2021
…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
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
random-zebra added a commit to PIVX-Project/PIVX that referenced this issue Aug 5, 2021
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

8 participants