Skip to content

/rust-bindgen

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

Enable --rustfmt-bindings by default #1022

Merged
merged 1 commit into from Sep 25, 2017

Conversation

Reviewers

@fitzgen fitzgen

Assignees

@fitzgen fitzgen

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@harlanhaskins @fitzgen @bors-servo @highfive
@harlanhaskins
Copy link
Contributor

harlanhaskins commented Sep 22, 2017

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.

@highfive highfive added the S-awaiting-review label Sep 22, 2017

@harlanhaskins

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

harlanhaskins commented Sep 22, 2017 •
edited

This still has 357 failing tests because the existing expected output is unformatted. For the purpose of this patch, should I update all the test cases or pass --no-rustfmt-bindings instead?

Never mind, looks like I misinterpreted things.

@harlanhaskins harlanhaskins force-pushed the harlanhaskins:rustfmt-by-default branch from e5c73a2 to 0e07f9f Sep 22, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 22, 2017

This looks good to me -- thanks! Is there still anything outstanding that I'm not seeing that is keeping this PR "WIP"?

@harlanhaskins harlanhaskins changed the title [WIP] Enable --rustfmt-bindings by default Enable --rustfmt-bindings by default Sep 22, 2017

@harlanhaskins

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

harlanhaskins commented Sep 22, 2017

Not really, no! Unless -- should I add the flag back in and mark it deprecated?
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 22, 2017

Not really, no! Unless -- should I add the flag back in and mark it deprecated?

Yeah, let's do that, good idea.
@harlanhaskins
Enable --rustfmt-bindings by default 
This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.
Loading status checks…
89b4dbc

@harlanhaskins harlanhaskins force-pushed the harlanhaskins:rustfmt-by-default branch from 0e07f9f to 89b4dbc Sep 22, 2017

@harlanhaskins

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

harlanhaskins commented Sep 22, 2017

@fitzgen Good to merge?
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

@fitzgen Good to merge?

Sorry, didn't see that you pushed new commits -- sometimes I don't get those emails for some reason...
@fitzgen

fitzgen approved these changes Sep 25, 2017
View changes

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

@bors-servo r+

Thanks @harlanhaskins !
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 25, 2017

📌 Commit 89b4dbc has been approved by fitzgen

@highfive highfive assigned fitzgen Sep 25, 2017

@highfive highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 25, 2017

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 25, 2017

⌛️ Testing commit 89b4dbc with merge 81c634b...

bors-servo added a commit that referenced this pull request Sep 25, 2017

@bors-servo
Auto merge of #1022 - harlanhaskins:rustfmt-by-default, r=fitzgen 
Enable --rustfmt-bindings by default

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.
Loading status checks…
81c634b
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 25, 2017

💔 Test failed - status-travis

@highfive highfive added S-tests-failed and removed S-awaiting-merge labels Sep 25, 2017

@harlanhaskins

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

harlanhaskins commented Sep 25, 2017

Looks like it failed downloading components:
https://travis-ci.org/rust-lang-nursery/rust-bindgen/jobs/279586683

Can we re-run the tests?
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

@harlanhaskins

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

harlanhaskins commented Sep 25, 2017

I haven’t deleted the branch...
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

Huh. Retriggered once again. Was seeing

$ cd rust-lang-nursery/rust-bindgen
$ git checkout -qf 81c634b8628b0bf2d34f521a7824d68058a53c8f
fatal: reference is not a tree: 81c634b8628b0bf2d34f521a7824d68058a53c8f
The command "git checkout -qf 81c634b8628b0bf2d34f521a7824d68058a53c8f" failed and exited with 128 during .
Your build has been stopped.

Which is usually what happens when the branch gets deleted before CI finishes.
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 25, 2017

@bors-servo retry
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 25, 2017

⌛️ Testing commit 89b4dbc with merge 9113b7b...

bors-servo added a commit that referenced this pull request Sep 25, 2017

@bors-servo
Auto merge of #1022 - harlanhaskins:rustfmt-by-default, r=fitzgen 
Enable --rustfmt-bindings by default

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.
Loading status checks…
9113b7b

@highfive highfive added S-awaiting-merge and removed S-tests-failed labels Sep 25, 2017

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 25, 2017

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 9113b7b to master...

@bors-servo bors-servo merged commit 89b4dbc into rust-lang:master Sep 25, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@highfive highfive removed the S-awaiting-merge label Sep 25, 2017

@fitzgen fitzgen referenced this pull request Sep 25, 2017

Closed

Make `--rustfmt-bindings`/`Builder::rustfmt_bindings` on by default #977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.