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

MSVC link output improve #62021

Merged
merged 3 commits into from
Jul 3, 2019
Merged

Conversation

crlf0710
Copy link
Member

Resolves #35785.

However i haven't come up with a idea to add test case for this :(

r? @retep998

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2019
@crlf0710
Copy link
Member Author

i did some small experiments with link.exe, it seems to me it's like this:

  1. When the oem cp and console cp is the same, link.exe outputs in the localized way.
  2. When they're different, link.exe outputs in neither of them, but english.

@retep998
Copy link
Member

By english do you also mean UTF-8?

@crlf0710
Copy link
Member Author

No i don't, i just mean these software stopped using any localized resources, and (coincidentally) the output can be handled the existing utf-8 handling code path.

@crlf0710
Copy link
Member Author

Found a better way. Just set this environment variable and the output will always be in English.

See also
https://bugreports.qt.io/browse/QTCREATORBUG-20327

@crlf0710
Copy link
Member Author

cc @alexcrichton

@retep998
Copy link
Member

Having the output always be in English seems suboptimal for users who don't really know English, because now they won't understand the linker messages.

@crlf0710
Copy link
Member Author

I think this is fine, since Rust compiler output is also always in English now. This can be changed when Rust supports multilingual compiler output in the future though.

@alexcrichton
Copy link
Member

Thanks for the PR! I don't really think I fully understand the issue here though and how this fixes it. Is there documentation for what this env var is? Can you describe how it's fixing the issue? Can you describe a bit as to why the underlying issue is occuring?

It seems like link.exe is producing encodings that we don't understand, but does that mean we should use Windows built-in functions for converting to utf-8?

@crlf0710
Copy link
Member Author

Hi,

let me first try to describe the issue. Visual C++ build tools, like cl.exe and link.exe, have multilingual error output strings support. So when you have a linker error and have a language pack installed and certain conditions meet, the linker will try to output error descriptions in the corresponding language. However it never does so in UTF-8. Instead it is outputted in some corresponding codepage. And rustc will output "Non-UTF-8 output: %xx%xx....", which is not directly readable. Causing problems like #35785 and #53382.

Since Microsoft-product localization is English-centric, the current PR changes simply suppresses those behavior, by adding an environment variable VSLANG=1033 (English). In this way, the output error messages are in ASCII, so they're no longer influenced by language packs and code pages. There might still be Non UTF-8 data in paths and code text, but most of the time it will just work. And this is the approach that is adopted by Qt Creators and many other tools.

For the environment variable, i don't think Microsoft has public documentation. However many tools, both Qt from outside Microsoft, and Visual Studio, Dotnet CLI, MSBuild is respecting this behavior. Within dotnet/msbuild#1596, there're links that seems to give more detailed information but is not accessible to people outside Microsoft (like me).

@alexcrichton
Copy link
Member

Thanks for the explainer! Is there any way we can force utf-8 output though? Or otherwise is it possible to use kernel32 functions to decode what the linker/tooling is handing us?

@crlf0710
Copy link
Member Author

We cannot force utf-8 output. However we actually can do the conversion, i actually did so in the first version of this PR. The code is here(on this branch of my fork):
https://github.com/crlf0710/rust/pull/1/files

The cons is that we cannot prove that this conversion always work (e.g. cannot prove the source output is always in CP_OEMCP). Besides that, it's a little strange to mix localized messages into Rust compiler's always outputting English output session.

I think you can compare the code to see which is better. I think both are better than the status quo.

@alexcrichton
Copy link
Member

Ok thanks again for that! I agree that it's probably a bit too unnecessarily complicated to add that in. I think we could perhaps try to decode with a few options (one being CP_OEMCP) and maybe try to find which is closest to something we understand (e.g. utf-8), but I'm otherwise convinced that all other messages coming from rustc are English so we may as well just do the same for the linker.

@crlf0710 could you add a comment to this new env var indicating what it's doing and why it's there? (basically what you've already mentioned in this PR). After that I think this is good to go!

@crlf0710
Copy link
Member Author

@alexcrichton done~

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 27, 2019

📌 Commit 8339211 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2019
@crlf0710
Copy link
Member Author

crlf0710 commented Jul 1, 2019

@Centril maybe rollup this in next rollup, too?

Centril added a commit to Centril/rust that referenced this pull request Jul 1, 2019
…r=alexcrichton

MSVC link output improve

Resolves rust-lang#35785.

However i haven't come up with a idea to add test case for this :(

r? @retep998
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 3, 2019
…r=alexcrichton

MSVC link output improve

Resolves rust-lang#35785.

However i haven't come up with a idea to add test case for this :(

r? @retep998
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 3, 2019
…r=alexcrichton

MSVC link output improve

Resolves rust-lang#35785.

However i haven't come up with a idea to add test case for this :(

r? @retep998
bors added a commit that referenced this pull request Jul 3, 2019
Rollup of 15 pull requests

Successful merges:

 - #62021 (MSVC link output improve)
 - #62064 (nth_back for chunks_exact)
 - #62128 (Adjust warning of -C extra-filename with -o.)
 - #62161 (Add missing links for TryFrom docs)
 - #62183 (std: Move a process test out of libstd)
 - #62186 (Add missing type urls in Into trait)
 - #62196 (Add Vec::leak)
 - #62199 (import gdb for explicit access to gdb.current_objfile())
 - #62229 (Enable intptrcast for explicit casts)
 - #62250 (Improve box clone doctests to ensure the documentation is valid)
 - #62255 (Switch tracking issue for `#![feature(slice_patterns)]`)
 - #62285 (Fix michaelwoerister's mailmap)
 - #62304 (HashMap is UnwindSafe)
 - #62319 (Fix mismatching Kleene operators)
 - #62327 (Fixed document bug, those replaced each other)

Failed merges:

r? @ghost
@bors bors merged commit 8339211 into rust-lang:master Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unreadable non-UTF-8 output on localized MSVC
5 participants