Skip to content

rustbuild: distribute cargo-fmt alongside rustfmt - #46031

Merged
bors merged 4 commits into
rust-lang:masterfrom
Keruspe:cargofmt
Nov 21, 2017
Merged

rustbuild: distribute cargo-fmt alongside rustfmt#46031
bors merged 4 commits into
rust-lang:masterfrom
Keruspe:cargofmt

Conversation

@Keruspe

@KeruspeKeruspe commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

Not sure whether we want that nor if it's the right way to do so, but it feels quite weird to have rustfmt without cargo-fmt. Or are there other plans wrt that?

What do you think @nrc ?

@Keruspe
rustbuild: use a macro to define "extended" tools
8cd5735
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
rustbuild: dist cargo-fmt as part of rustfmt
8f91a45
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
rustbuild: make tidy happy
9817589
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@kennytmkennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2017
@kennytm

kennytm commented Nov 18, 2017

Copy link
Copy Markdown
Member

r? @nrc

@nrc

nrc commented Nov 20, 2017

Copy link
Copy Markdown
Member

Thanks! Seems good to me, but r? @Mark-Simulacrum for the build system changes.

Mark-Simulacrum
Mark-Simulacrum requested changes Nov 20, 2017
View reviewed changes

@Mark-SimulacrumMark-Simulacrum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty good. I'm not too happy with the macro, but I think it's okay for now at least. We do similar things elsewhere, and until we come up with a better solution this should work.

Please fix the one comment and r=me afterwards.

Comment threadsrc/bootstrap/dist.rs Outdated
let cargofmt = builder.ensure(tool::Cargofmt {
compiler: builder.compiler(stage, build.build),
target
}).expect("Rustfmt to build: toolstate is testing");

@Mark-SimulacrumMark-SimulacrumNov 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please change the expect statement here to "cargofmt" or something like that.

@kennytmkennytm 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 Nov 20, 2017
@Keruspe
rustbuild: fix expectation message
b29a61e
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe

Keruspe commented Nov 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@bors: r=Mark-Simulacrum

@bors

bors commented Nov 20, 2017

Copy link
Copy Markdown
Collaborator

@Keruspe: 🔑 Insufficient privileges: Not in reviewers

@Keruspe

Keruspe commented Nov 20, 2017

Copy link
Copy Markdown
ContributorAuthor

(I kind of expected that indeed, would have been surprised otherwise)

@Keruspe

Keruspe commented Nov 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@kennytm not waiting on bors yet, fwiw (wrt tags)

@Mark-Simulacrum

Mark-Simulacrum commented Nov 20, 2017

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Nov 20, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit b29a61e has been approved by Mark-Simulacrum

@oli-obk

oli-obk commented Nov 20, 2017

Copy link
Copy Markdown
Contributor

cargo fmt could be implemented as a stable cargo installable tool, which would not require distributing it, since it would never break.

@nrcnrc mentioned this pull request Nov 21, 2017
Merged
@kennytmkennytm mentioned this pull request Nov 21, 2017
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Nov 21, 2017
@kennytm
Rollup merge ofrust-lang#46031- Keruspe:cargofmt, r=Mark-Simulacrum
f8b3e7c
rustbuild: distribute cargo-fmt alongside rustfmt
Not sure whether we want that nor if it's the right way to do so, but it feels quite weird to have rustfmt without cargo-fmt. Or are there other plans wrt that?
What do you think @nrc ?
@kennytmkennytm mentioned this pull request Nov 21, 2017
Merged
bors added a commit that referenced this pull request Nov 21, 2017
@bors
Auto merge of#46166- kennytm:rollup, r=kennytm
63739ab
Rollup of 11 pull requests
- Successful merges: #45987, #46031, #46050, #46052, #46103, #46120, #46134, #46141, #46148, #46155, #46157
- Failed merges:
@bors
bors merged commit b29a61e into rust-lang:masterNov 21, 2017
bors added a commit to rust-lang/rustup that referenced this pull request Dec 6, 2017
@bors
Auto merge of#1294- rust-lang-nursery:rustfmt, r=alexcrichton
4c23be5
Add rustfmt to the tools list
r? @alexcrichton
We should probably add something for cargo-fmt too once rust-lang/rust#46031 lands, but I'm not sure if we can just add it to the list or not, seeing as it is not its own component.
bors added a commit to rust-lang/rustup that referenced this pull request Dec 6, 2017
@bors
Auto merge of#1294- rust-lang-nursery:rustfmt, r=alexcrichton
9f013e8
Add rustfmt to the tools list
r? @alexcrichton
We should probably add something for cargo-fmt too once rust-lang/rust#46031 lands, but I'm not sure if we can just add it to the list or not, seeing as it is not its own component.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-SimulacrumMark-SimulacrumMark-Simulacrum requested changes

Assignees

@Mark-SimulacrumMark-Simulacrum

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Keruspe@kennytm@nrc@bors@Mark-Simulacrum@oli-obk