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.

Sign up for GitHub

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

Jump to bottom

Corrected the CONTRIBUTING.md "External Dependencies" section #44664

Merged
merged 1 commit into from
Oct 5, 2017
Merged

Corrected the CONTRIBUTING.md "External Dependencies" section #44664

merged 1 commit into from
Oct 5, 2017

Conversation

budziq
Copy link
Contributor

@budziq budziq commented Sep 17, 2017 •
edited
Loading

The "External Dependencies" section is a little outdated.
Please see following comments #44567 (comment) #44567 (comment) for rationale.

@carols10cents
Copy link
Member

r? @steveklabnik

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2017
@alexcrichton
Copy link
Member

I think this may conflict with #44679, which is also changing this file

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2017

Fine by me, if clippy isn't replaced, but instead rustfmt is added to the list.

It might be easier for now to suggest opening a PR and marking the tool as broken as the primary way to solve this, since in curtains no waiting time ( with the solution in this PR you need to wait on the tool Devs)

@budziq
Copy link
Contributor Author

budziq commented Sep 19, 2017

It might be easier for now to suggest opening a PR and marking the tool as broken as the primary way to solve this,

As long as this approach is the one actually agreed upon by the maintainers I'm game!
Should I close this PR?

@bors
Copy link
Contributor

bors commented Sep 21, 2017

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

@carols10cents
Copy link
Member

@budziq it sounds like @oli-obk was suggesting some modifications to this PR, not necessarily closing it-- do I have that right? It also looks like there were indeed some merge conflicts introduced by #44679 that you'll need to resolve :)

@carols10cents carols10cents 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2017
@budziq
Copy link
Contributor Author

budziq commented Sep 25, 2017

Thanks @carols10cents!
I've updated the PR with best I could infer, but honestly I'm still unclear about the expected workflow in case of broken submodules.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 26, 2017

Since pull requests are rather volatile, we are supposed to only point to branches, not to pull requests. This means that if the tool authors accept your pull request into a branch, you may point your PR's submodule to the tool-branch, but you may not point it to the tool-PR

@budziq
Copy link
Contributor Author

budziq commented Sep 30, 2017

Sorry, I've been super busy with rust-fest preparations. I'll push an update within next 2 days!

@budziq
Copy link
Contributor Author

budziq commented Sep 30, 2017

@oli-obk Updated!

oli-obk
oli-obk reviewed Oct 1, 2017
View reviewed changes
CONTRIBUTING.md Outdated
you can disable the tool via `src/tools/toolstate.toml`.
a pull request against the broken project asking to put the fix on a branch.
When you have opened a pull request, you can disable the tool building via
`src/tools/toolstate.toml`. Once the pull request is likely to be accepted,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still mentioning pull requests instead of long lived branches

@budziq
Copy link
Contributor Author

budziq commented Oct 1, 2017

Sry hope it's ok now

@steveklabnik
Copy link
Member

@bors: r+ rollup

thanks!

@bors
Copy link
Contributor

bors commented Oct 5, 2017

📌 Commit e6c3c7f has been approved by steveklabnik

kennytm added a commit to kennytm/rust that referenced this pull request Oct 5, 2017
@kennytm
Rollup merge of rust-lang#44664 - budziq:contributing_external, r=ste…
b34ade0 
…veklabnik

Corrected the CONTRIBUTING.md "External Dependencies" section

The "External Dependencies" section is a little outdated.
Please see following comments rust-lang#44567 (comment) rust-lang#44567 (comment) for rationale.
@kennytm kennytm mentioned this pull request Oct 5, 2017
Merged
bors added a commit that referenced this pull request Oct 5, 2017
@bors
Auto merge of #45046 - kennytm:rollup, r=kennytm
1db1144 
Rollup of 9 pull requests

- Successful merges: #44664, #44935, #44972, #44980, #44987, #44997, #45006, #45017, #45024
- Failed merges:
@bors bors merged commit e6c3c7f into rust-lang:master Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@steveklabnik steveklabnik

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@budziq @carols10cents @alexcrichton @oli-obk @bors @steveklabnik