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

Improve documention troubleshooting missing linker. Fix #32208. #36672

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

pmatos
Copy link
Contributor

@pmatos pmatos commented Sep 23, 2016

@steveklabnik is this in the direction on how you want to see #32208 fixed?

@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 @Manishearth (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.

@steveklabnik
Copy link
Member

@pmatos yes, looks great to me, thank you!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 23, 2016

📌 Commit ec2f778 has been approved by steveklabnik

Linux-based systems, Rust will attempt to call `cc` for linking. On
`windows-msvc` (Rust built on windows with Microsoft Visual Studio),
this depends on having Microsoft Visual Studio installed with its
tools in `%PATH%`, namely `linker.exe`. However, if you have your
Copy link
Member

@retep998 retep998 Sep 23, 2016

Choose a reason for hiding this comment

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

link.exe. Also it doesn't need to be in PATH because rustc can find msvc itself.

Probably would be better to simply say you need to have the "Visual C++ Build Tools" installed, with possibly a link to http://landinghub.visualstudio.com/visual-cpp-build-tools

@steveklabnik
Copy link
Member

@bors: r-

gah, read that one wrong, sorry

@sfackler
Copy link
Member

@bors r-

Pending @retep998's link.exe comment.

@pmatos
Copy link
Contributor Author

pmatos commented Sep 24, 2016

@retep998 thanks, preparing new pull request.

@pmatos
Copy link
Contributor Author

pmatos commented Sep 24, 2016

@retep998 I tried with the new pull request to take your comments into consideration but if there's something else that can be improved, please let me know.

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

One tiny nit, and then if @retep998 is happy, I'm happy.

general discussion [the #rust IRC channel on irc.mozilla.org][irc], which we
installed. Doing so will depend on your specific system. For
Linux-based systems, Rust will attempt to call `cc` for linking. On
`windows-msvc` (Rust built on windows with Microsoft Visual Studio),
Copy link
Member

Choose a reason for hiding this comment

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

Windows, not windows.

installed. Doing so will depend on your specific system. For
Linux-based systems, Rust will attempt to call `cc` for linking. On
`windows-msvc` (Rust built on windows with Microsoft Visual Studio),
this depends on having [Microsoft Visual Build Tools][msvbt]
Copy link
Member

Choose a reason for hiding this comment

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

Microsoft Visual _C++_ Build Tools

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

@steveklabnik Done. @retep998 Done.
The last two commits should have fixed the concerns you had. Thanks for the help on getting this pull request complete.

@steveklabnik
Copy link
Member

Great! Thanks so much for your patience here too.

One last question before I merge: it seems that the commit authorship is different than your github account. Is that what you intended? If so, it's fine, but otherwise, you might want to fix that before we merge 😄

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

You're right. Let me fix authorship. Sorry for this.

@steveklabnik
Copy link
Member

It's no worries! :)

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

@steveklabnik I have rewritten history on my end in order to change commit authorship which worked but now github falsely shows branch conflicts. Would you be ok with accepting this or do you want me to restart a brand new pull request on this?

@steveklabnik
Copy link
Member

Re-writing the history is totally fine :)

@steveklabnik
Copy link
Member

(the message in the bot is meant to be talking about during review, not administrative stuff like this at the end 😄 )

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

Great! So we are good to go. Happy to contribute. Anything else you have in mind that I could tackle next?

On 27 September 2016 21:27:49 CEST, Steve Klabnik [email protected] wrote:

(the message in the bot is meant to be talking about during review,
not administrative stuff like this at the end 😄 )

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#36672 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@steveklabnik
Copy link
Member

(You have to force push, it didn't actually update the PR)

Thanks! We have the A-docs tag for doc stuff, and not all of them are super difficult: https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AA-docs

https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AA-docs+label%3AE-easy are specifically the ones tagged easy. Lots to do!

@steveklabnik
Copy link
Member

Oh, it's not that you didn't force push, it's that when you did, it somehow got on the wrong base. Try to rebase things and force push again.

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

@steveklabnik Is this ok now? I have rebased LinkiTools/rust and pushed (no need to force now).

@steveklabnik
Copy link
Member

Hm, it still says 250+ commits 👀

If you want some help, I can do it: I'd just need you to follow the second half here: https://github.com/blog/2247-improving-collaboration-with-forks

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

@steveklabnik The checkbox to allow edits from maintainers is enabled. Thanks for your help. I have no idea at the moment what kind of weird games git/github is playing with me.

@steveklabnik
Copy link
Member

Whew! @pmatos , I don't know what happened, but something was very wrong. Trying to deal with this made my git go a bit... intense.

2016-09-27-163436_706x57_scrot

However, I managed to make it work. Here's what I did:

  1. git rebase -i to squash the four commits into one.
  2. git remote add upstream https://github.com/rust-lang/rust/
  3. git fetch upstream
  4. git reset --hard upstream/master
  5. git cherry-pick cb3b03c
  6. git push origin master -f

Specifically step 4 says "make this branch equal to master" and then the cherry-pick lets you apply a random commit on top. Whew!

Can you verify that this commit is what you wanted to send in? It looks okay with me, but since I just re-wrote your history, it's important that you read it before we merge, since it's in your name 😄

@pmatos
Copy link
Contributor Author

pmatos commented Sep 27, 2016

@steveklabnik many thanks for your time. roll it up. :) see in the next pull request.

@steveklabnik
Copy link
Member

Great! I think one of the problems might have been that it was on master instead of a branch, so maybe make a branch per PR next time? 😄

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 27, 2016

📌 Commit cb3b03c has been approved by steveklabnik

@pmatos
Copy link
Contributor Author

pmatos commented Sep 28, 2016

@steveklabnik I will use a branch per PR next time. Thanks.

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 28, 2016
Improve documention troubleshooting missing linker. Fix rust-lang#32208.

@steveklabnik is this in the direction on how you want to see rust-lang#32208 fixed?
bors added a commit that referenced this pull request Sep 28, 2016
Rollup of 11 pull requests

- Successful merges: #36376, #36672, #36740, #36757, #36765, #36769, #36782, #36783, #36784, #36795, #36796
- Failed merges:
@bors bors merged commit cb3b03c into rust-lang:master Sep 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants