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

Suggest installing VS Build Tools in more situations #72296

Merged
merged 1 commit into from
May 21, 2020
Merged

Suggest installing VS Build Tools in more situations #72296

merged 1 commit into from
May 21, 2020

Conversation

ChrisDenton
Copy link
Member

When MSVC's link.exe wasn't found but another link.exe was, the error message given can be impenetrable to many users. The usual suspect is GNU's link tool. In this case, inform the user that they may need to install VS build tools.

This only applies when Microsoft's link tool is expected.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2020
@varkor
Copy link
Member

varkor commented May 18, 2020

This looks reasonable to me, but I'd like to double-check this approach is the right one. Presumably there's not a straightforward way to test this? r? @Mark-Simulacrum

@ChrisDenton
Copy link
Member Author

I did consider a number of approaches.

One alternative approach would be to test the linker before doing the linking and show an error if not MSVC's link.exe. This was my initial thought but I became worried about the potential for false negatives to prevent linking so I went for a softer approach. Maybe this was too cautious (or maybe not).

A variation on this PR would be to read the "logo" displayed when running a link command without the /nologo switch. So, a command like link.exe /DEF will fail but will produce the logo in the output before the error message:

Microsoft (R) Incremental Linker Version 14.25.28614.0
Copyright (C) Microsoft Corporation.  All rights reserved.

Another approach would be for rustc to always test for the existence of MSVC build tools when building with an msvc toolchain (but not msvc compatible toolchains like lld-link). It could then display a note warning of the missing tools. For example, making use of cc::windows_registry::find_tool, if it returns None when looking for link.exe then the C++ build tools aren't installed. However, it may be possible to have the build tools in PATH without them being installed via the installer. Though this is unlikely because Microsoft only makes its tools available via their installer.

@Mark-Simulacrum
Copy link
Member

Doing this after the error occurs and not before does seem like the right approach -- it at least optimizes for the happy path of "everything is good."

However, I'm not familiar with the details of our current linker story (especially on Windows), so let's r? @petrochenkov here perhaps?

@retep998
Copy link
Member

Doing the diagnostic based on the error code seems a bit shaky to me. If link.exe ever decides to exit with a code in the range [0, 1000) then this code will inform the user they don't have VC++ correctly installed, even if the cc crate correctly found a full VC++ toolchain.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented May 18, 2020

That is a concern. My goal with checking link_path was to avoid triggering the message if the cc crate found the linker (under the assumption that cc would return a full path). But I see now that the value is never updated to the found path.

Adding an explicit check using the cc crate would avoid this issue?

@ChrisDenton
Copy link
Member Author

I've updated the diagnostic to be more robust. Sorry about the messy commit history, I need to get better at knowing if I'm in the VM or host when issuing commands.

@ChrisDenton
Copy link
Member Author

I'll explain my reasoning a bit more.

A Microsoft linking error is a four digit number in the range 1000 to 9999 inclusive. So an exit code in this range is a successful failure. Any other error indicates a problem with link.exe itself. This could be caused by a missing DLL, a corrupt system configuration, etc. It could also indicate that the link.exe is not a Microsoft linker.

I've added better checks for the possible ways things could have gone wrong and tailored the notes to each situation (though the messages may need work).

Copy link
Member

@retep998 retep998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is probably good enough for a simple diagnostic to try to help the user figure out what they did wrong. We can always revisit this in a future PR if it turns out some part of this is inaccurate.

@Mark-Simulacrum
Copy link
Member

r=me with commits squashed

@ChrisDenton
Copy link
Member Author

Sorry, I'm new at this. Should the squishing be done from my end?

@retep998
Copy link
Member

Yes. Squashing involves using git rebase to squash down your multiple commits into a single commit.

When MSVC's `link.exe` wasn't found but another `link.exe` was, the error message given can be impenetrable to many users. The usual suspect is GNU's `link` tool. In this case, inform the user that they may need to install VS build tools.

This only applies when Microsoft's link tool is expected. Not `lld-link` or other MSVC compatible linkers.
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 20, 2020

📌 Commit 2fd504c has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented May 20, 2020

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

@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 May 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 20, 2020
…ochenkov

Suggest installing VS Build Tools in more situations

When MSVC's `link.exe` wasn't found but another `link.exe` was, the error message given can be [impenetrable](https://pastebin.com/MRMCr7HM) to many users. The usual suspect is GNU's `link` tool. In this case, inform the user that they may need to install VS build tools.

This only applies when Microsoft's link tool is expected.
RalfJung added a commit to RalfJung/rust that referenced this pull request May 21, 2020
…ochenkov

Suggest installing VS Build Tools in more situations

When MSVC's `link.exe` wasn't found but another `link.exe` was, the error message given can be [impenetrable](https://pastebin.com/MRMCr7HM) to many users. The usual suspect is GNU's `link` tool. In this case, inform the user that they may need to install VS build tools.

This only applies when Microsoft's link tool is expected.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#71854 (Make `std::char` functions and constants associated to `char`.)
 - rust-lang#72111 (rustc-book: Document `-Z strip=val` option)
 - rust-lang#72272 (Fix going back in history to a search result page on firefox)
 - rust-lang#72296 (Suggest installing VS Build Tools in more situations)
 - rust-lang#72365 (Remove unused `StableHashingContext::node_to_hir_id` method)
 - rust-lang#72371 (FIX - Char documentation for unexperienced users)
 - rust-lang#72397 (llvm: Expose tiny code model to users)

Failed merges:

r? @ghost
@bors bors merged commit 85d712c into rust-lang:master May 21, 2020
@ChrisDenton ChrisDenton deleted the msvc-link-check branch May 21, 2020 16:33
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.

7 participants