-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Refactor tools/build-mainfest #58995
Conversation
We unfortunately don't have a lot of testing for this, so can you confirm you've built and run this locally in simple scenarios to make sure nothing is accidentally broken? Otherwise the refactorings look good to me, thanks! |
@alexcrichton Following the instructions in the README seems to give the right structure for things. I reran the instructions on master and compared the diffs which produced no changes. |
This comment has been minimized.
This comment has been minimized.
72b7d0f
to
3d4a2ef
Compare
97e0808
to
b74f510
Compare
b74f510
to
8353487
Compare
@alexcrichton Rebased; should be good to go now. |
@bors: r+ 👍 |
📌 Commit 8353487 has been approved by |
…alexcrichton Refactor tools/build-mainfest I saw some duplication in rust-lang#58990 and got an itch... Will likely need to be rebased when that lands. Hopefully the PR should have zero semantic changes... r? @alexcrichton
…alexcrichton Refactor tools/build-mainfest I saw some duplication in rust-lang#58990 and got an itch... Will likely need to be rebased when that lands. Hopefully the PR should have zero semantic changes... r? @alexcrichton
⌛ Testing commit 8353487 with merge 6e2e6db90f96f3c26acff95670af391eda5be10d... |
💔 Test failed - status-appveyor |
@bors retry |
Refactor tools/build-mainfest I saw some duplication in #58990 and got an itch... Will likely need to be rebased when that lands. Hopefully the PR should have zero semantic changes... r? @alexcrichton
☀️ Test successful - checks-travis, status-appveyor |
I saw some duplication in #58990 and got an itch... Will likely need to be rebased when that lands. Hopefully the PR should have zero semantic changes...
r? @alexcrichton