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: Convert Windows args to utf-8 string #13883

Merged
merged 1 commit into from Oct 8, 2018

Conversation

Projects
None yet
8 participants
@ken2812221
Copy link
Member

commented Aug 5, 2018

Create a new class WinCmdLineArgs when building for Windows. It converts all command line arguments to utf8 string.

@ken2812221 ken2812221 referenced this pull request Aug 5, 2018

Closed

Filename and command line encoding issue on Windows #13869

11 of 12 tasks complete
@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

utACK f059e7a

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

Reviewers, this pull request conflicts with the following ones:
  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)
  • #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.

@alexeyneu

This comment has been minimized.

Copy link

commented Aug 13, 2018

no-bowler
https://imageshack.com/a/img922/8836/nesafY.jpg

	std::wstring h;
	h=GetCommandLineW();

	std::wcout << h;

it's for wwinmain
UPD:
it's smth else here .
works only after setlocale(LC_ALL, "Portuguese_Portugal.1252"); with main/wmain no matter
(?)
upd2:

_setmode(_fileno(stdout), _O_U16TEXT);
std::wcout << L"curaçao";

seems to be only way. Will be handy when verbose log dubbed to stdout .not sure if btc wallet has this option but monero has smth like that

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch Aug 24, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch Aug 31, 2018

src/bitcoin-cli.cpp Outdated
@@ -15,6 +15,7 @@
#include <util.h>
#include <utilstrencodings.h>

#include <tuple>

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 11, 2018

Member
src/bitcoin-cli.cpp:18:1: warning: #includes are not sorted properly [llvm-include-order]

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch 2 times, most recently Sep 11, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

1e23b1cba1645b50570e1b4ca4f2da03e000f732 is adding a binary file. Is that wanted?

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch Sep 11, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

@MarcoFalke Sorry, forgot to delete that

src/bitcoin-cli.cpp Outdated
@@ -16,6 +16,7 @@
#include <utilstrencodings.h>

#include <memory>
#include <tuple>

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 13, 2018

Member

Minor nit:

2018-09-13 21:33:38 clang-tidy(pr=13883): src/bitcoin-cli.cpp:18:1: warning: #includes are not sorted properly [llvm-include-order]

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 14, 2018

Author Member

Would you prefer to put tuple after stdio.h? I think we should seperate C and C++ headers.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 14, 2018

Member

I don't think we separate C and C++ headers elsewhere, but please sort them.

src/util.cpp Outdated
return std::make_pair(argc, argv);
}
#endif
}

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 14, 2018

Member
2018-09-13 21:33:38 clang-tidy(pr=13883): src/util.cpp:1290:2: warning: namespace 'util' not terminated with a closing comment [google-readability-namespace-comments]

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch Sep 14, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch 2 times, most recently Sep 27, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch Sep 28, 2018

@ken2812221 ken2812221 changed the title utils: Convert Windows args to utf-8 string [WIP] utils: Convert Windows args to utf-8 string Sep 28, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch 2 times, most recently Sep 28, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch 2 times, most recently Sep 28, 2018

@ken2812221 ken2812221 changed the title [WIP] utils: Convert Windows args to utf-8 string utils: Convert Windows args to utf-8 string Sep 28, 2018

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

Can you add a python test on this? You can probably highjack https://github.com/bitcoin/bitcoin/blob/78dae8caccd82cfbfd76557f1fb7d7557c7b5edb/test/functional/feature_uacomment.py by trying to set a chinese char.

@ken2812221 ken2812221 force-pushed the ken2812221:windows-command-line-args branch to 380c843 Sep 30, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Concept ACK 380c843 (Only checked that this should only affect Windows. Didn't review nor checked that the tests fail without this fix on windows.)

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

tACK 380c843 . The test is breaking without the fix.

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Oct 1, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 8, 2018

Merge bitcoin#13883: utils: Convert Windows args to utf-8 string
380c843 utils: Convert Windows args to utf-8 string (Chun Kuan Lee)

Pull request description:

  Create a new class `WinCmdLineArgs` when building for Windows. It converts all command line arguments to utf8 string.

Tree-SHA512: f098520fd123a8a452bc84a55dc8c0b88f0c475410efe57f2ccc393f86c396eed59ea1575ddc1b920323792e390fdb092061d80cdcd9b682f0ac79a22a22ff82

@MarcoFalke MarcoFalke merged commit 380c843 into bitcoin:master Oct 8, 2018

2 checks passed

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

@ken2812221 ken2812221 deleted the ken2812221:windows-command-line-args branch Oct 8, 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.