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

Add rustc guide to toolstate #59772

Merged
merged 10 commits into from
Jul 6, 2019

Conversation

andrehjr
Copy link
Contributor

@andrehjr andrehjr commented Apr 7, 2019

Closes #59597

@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 @nikomatsakis (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 Apr 7, 2019
@andrehjr andrehjr force-pushed the add-rustc-guide-to-toolstate branch from 519d4be to 8eb9030 Compare April 7, 2019 14:11
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@andrehjr andrehjr force-pushed the add-rustc-guide-to-toolstate branch from d7a0bc4 to 1c531c7 Compare April 7, 2019 16:34
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2019

r? @kennytm who (I think) has been keeping track of this?

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2019

Hmm it looks like libssl-dev is not installed on the Linux build machines. You might need to edit the .travis.yml file to make sure it is installed as a dependency of the build.

@andrehjr
Copy link
Contributor Author

andrehjr commented Apr 8, 2019

I'll take a look into that!

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2019

Hmm... you might need to install both libssl and libssl-dev?

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@andrehjr
Copy link
Contributor Author

andrehjr commented Apr 8, 2019

It seems to me, that libssl-dev and stuff are already installed on the linux machine.. but somehow pkg-config is not being able to pick it up on that part of the build process.. I could try something like you can set the OPENSSL_DIR environment variable for the compilation process. but it seems too much 🤔

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2019

@sfackler Do you know what might be causing this?

@rust-highfive

This comment has been minimized.

@sfackler
Copy link
Member

sfackler commented Apr 8, 2019

Doesn't the build run in the docker image? Installing things on the Travis host isn't going to work in that case.

@andrehjr
Copy link
Contributor Author

andrehjr commented Apr 9, 2019

That's true, the build runs into the docker image. I added the required deps onto the image and the build finally finished with success now! Thanks! ✨

@mark-i-m what are the next steps?

Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

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

@andrehjr Nice work! I left a few comments below, but overall it looks good to me.

The next steps would be to un-WIP the title and wait for a review from somebody with bors permissions (I only have permissions on the rustc-guide repo). This should happen relatively soon. At that point, bors will test and merge PR, and that's it!

src/bootstrap/test.rs Outdated Show resolved Hide resolved
src/tools/rustbook/src/main.rs Outdated Show resolved Hide resolved
src/tools/rustbook/src/main.rs Outdated Show resolved Hide resolved
src/tools/rustbook/src/main.rs Outdated Show resolved Hide resolved
@andrehjr andrehjr force-pushed the add-rustc-guide-to-toolstate branch from 63717d9 to f2f1a2e Compare April 9, 2019 11:02
@andrehjr andrehjr changed the title [WIP] Add rustc guide to toolstate Add rustc guide to toolstate Apr 9, 2019
Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

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

Sorry, I missed one last thing. Other than that I think this is ready 🎉

src/tools/publish_toolstate.py Outdated Show resolved Hide resolved
@andrehjr
Copy link
Contributor Author

andrehjr commented Apr 9, 2019

Awesome! @mark-i-m Thanks so much for guiding me through this PR 🎉 🎉

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2019

My pleasure :) Thanks for contributing.

@bors
Copy link
Contributor

bors commented Apr 13, 2019

☔ The latest upstream changes (presumably #59515) made this pull request unmergeable. Please resolve the merge conflicts.

@andrehjr andrehjr force-pushed the add-rustc-guide-to-toolstate branch from 1de9968 to 4062a71 Compare April 13, 2019 15:19
@bors
Copy link
Contributor

bors commented Jul 6, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout add-rustc-guide-to-toolstate (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self add-rustc-guide-to-toolstate --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2019
@andrehjr andrehjr force-pushed the add-rustc-guide-to-toolstate branch from b6ef091 to b5cd962 Compare July 6, 2019 14:06
@mark-i-m
Copy link
Member

mark-i-m commented Jul 6, 2019

@Centril I understand the importance of the rollups, but could you avoid preempting running builds of PRs with 100s of comments please?

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2019

📌 Commit b5cd962 has been approved by mark-i-m

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2019
@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

@mark-i-m

@Centril I understand the importance of the rollups, but could you avoid preempting running builds of PRs [...]?

Sorry, but no. Maintaining high PR throughput and low latency (a shorter queue) is the 1. priority when managing the bors queue, especially after last week's CI problems after which we had a huge number of PRs in the queue. When one of my rollups fail due to some PR in it or some spurious failure I won't let other PRs leap ahead while I remake the rollup unless there is some special circumstance.

[...] with 100s of comments please?

What's the relevance of 100s of comments?

@mark-i-m
Copy link
Member

mark-i-m commented Jul 6, 2019

@Centril the significance is that a disproportionately high amount of work and waiting has gone into this PR.

Moreover, it seems like every time the PR is approved, it incurs a merge conflict; it's been rebased at least 5 times in the last two weeks. It's just a bit frustrating when such a PR finally makes it to the head of the queue... and is preempted...

@bors
Copy link
Contributor

bors commented Jul 6, 2019

⌛ Testing commit b5cd962 with merge dfd52ba...

bors added a commit that referenced this pull request Jul 6, 2019
@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

@Centril the significance is that a disproportionately high amount of work and waiting has gone into this PR.

The queue system works such that older PRs automatically get ahead in the queue. The PR has also tested 4 times and failed. To me this means that this PR has been treated fairly by the queue.

Moreover, it seems like every time the PR is approved, it incurs a merge conflict; it's been rebased at least 5 times in the last two weeks.

When the queue size is large it contributes towards having to rebase more times but if some PR is especially bitrotty you should raise the priority and write a comment saying it is bitrotty. In this case rebases were necessary due to lockfiles which are updated primarily outside of rollups by PRs that jump the queue with p=1.

It's just a bit frustrating when such a PR finally makes it to the head of the queue... and is preempted...

First, a PR has a better chance of being tested if the queue is small and this is achieved through rollups. Second, it is equally frustrating to baby sit rollups, have them fail, and then be preempted by a PR that just got added to the queue. The alternative would be to treeclose while doing rollup ops but that doesn't seem to work so well.

@bors
Copy link
Contributor

bors commented Jul 6, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: mark-i-m
Pushing dfd52ba to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 6, 2019
@bors bors merged commit b5cd962 into rust-lang:master Jul 6, 2019
@andrehjr andrehjr deleted the add-rustc-guide-to-toolstate branch July 6, 2019 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Tracking Issue: Add rustc-guide to toolstate
9 participants