-
Notifications
You must be signed in to change notification settings - Fork 37.7k
utils: Make fs::path::string() always return utf-8 string on Windows #13877
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
Conversation
Note to reviewers: This pull request conflicts with the following ones:
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. |
src/util.cpp
Outdated
@@ -58,6 +58,7 @@ | |||
#ifndef NOMINMAX | |||
#define NOMINMAX | |||
#endif | |||
#include <codecvt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe include only if WIN32
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. But I'll wait for gitian build done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already be defined only if WIN32. Forgot about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously applied patch detected.
Needs rebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased
Done quick code review (on other commits as well), seems ok, I will test more deeply later this week. Very excited about this. |
@@ -62,8 +62,6 @@ | |||
#include <QFontDatabase> | |||
#endif | |||
|
|||
static fs::detail::utf8_codecvt_facet utf8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.
This is documented at https://www.boost.org/doc/libs/1_68_0/boost/detail/utf8_codecvt_facet.hpp, in case anybody else is curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 26d6fe4a10c2c8cd16303835dd7cd5289f2d3b67
@@ -62,8 +62,6 @@ | |||
#include <QFontDatabase> | |||
#endif | |||
|
|||
static fs::detail::utf8_codecvt_facet utf8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.
This is documented at https://www.boost.org/doc/libs/1_68_0/boost/detail/utf8_codecvt_facet.hpp, in case anybody else is curious.
@NicolasDorier, are you still planning on testing this? If not, I think it would be good to get this merged since #13878 depends on it. Note to reviewers: even though fix here is pretty esoteric, it should be hopefully should be clear that it doesn't effect anything other than fs::path encodings used on windows. |
Gitian builds for commit 920c090 (master):
Gitian builds for commit af0794a4f1e0a892780150c6069154f8327cb1e2 (master and this pull):
|
utACK 2c3eade (Only checked that this does't affect linux) |
utACK 2c3eade |
…ng on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118
Backports bitcoin#13877 Imbue fs::path with std::codecvt_utf8_utf16 at SetupEnvironment(), so that default string encoding will be utf-8 inside fs::path.
Backports bitcoin#13877 Imbue fs::path with std::codecvt_utf8_utf16 at SetupEnvironment(), so that default string encoding will be utf-8 inside fs::path.
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
…-8 string on Windows 2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee) Pull request description: Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`. Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118 # Conflicts: # src/qt/guiutil.cpp
Merge bitcoin#13877: utils: Make fs::path::string() always return utf-8 string on Windows
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
Imbue
fs::path
withstd::codecvt_utf8_utf16
atSetupEnvironment()
, so that default string encoding will be utf-8 insidefs::path
.