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

Run buildifer on Bazel files #797

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Conversation

drigz
Copy link
Contributor

@drigz drigz commented Nov 7, 2018

buildifier autoformats the files to give a consistent style. This makes
edits easier for contributors that have their editor set to
autoformat-on-save.

@drigz drigz force-pushed the buildify branch 2 times, most recently from 78765a7 to 9c3660c Compare November 7, 2018 11:43
@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #797 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #797   +/-   ##
=======================================
  Coverage   53.27%   53.27%           
=======================================
  Files          30       30           
  Lines        3369     3369           
=======================================
  Hits         1795     1795           
  Misses       1399     1399           
  Partials      175      175

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d429ea...564438b. Read the comment docs.

@johanbrandhorst
Copy link
Collaborator

If we're doing this, I'd also be tempted to add a formatter step to CI. Would you be interested in adding a CI step to this PR that runs the buildifier and reports transgressions?

@drigz
Copy link
Contributor Author

drigz commented Nov 7, 2018

@johanbrandhorst I'm happy to try. Would you like me to do it in this or another PR?

@johanbrandhorst
Copy link
Collaborator

This PR please.

@achew22
Copy link
Collaborator

achew22 commented Nov 7, 2018

Gazelle is a good solution to this problem in the long run. It's a simple binary that is able to regenerate all the BUILD files as they should be written including doing these kinds of formattings.

The maintainer of Gazelle, is presently working on expanding his binary so that you can add language plugins. Once this happens I am going to write a language plugin for grpc-gateway that auto-generates these BUILD files. When that happens I will make it a part of the presubmit process.

I'm going to merge these because my hopes/dreams shouldn't slow other ppl down.

@drigz, thanks so much for contributing! If you know about Bazel and would be interested in contributing to the project, there is a lot more neat stuff we could do!

@drigz
Copy link
Contributor Author

drigz commented Nov 7, 2018

@achew22 I've compared gazelle v0.10.1 vs buildifier (build from oct 24) and found the following:

  • buildifier can reformat WORKSPACE but gazelle doesn't (by default at least)
  • buildifier can reformat *.bzl but gazelle doesn't (by default at least)
  • gazelle (unsurprisingly) wants to make a few changes to the go rules (see :gazelle_diff output from last run)
  • neither sets their exit code based on differences (gazelle issue)

Do you think the CI check should fail when Gazelle (/buildifier) would make a change?

@drigz drigz force-pushed the buildify branch 2 times, most recently from 077fb9d to e397e5d Compare November 7, 2018 17:41
buildifier autoformats the files to give a consistent style. This makes
edits easier for contributors that have their editor set to
autoformat-on-save.

buildifier has been added to the Bazel workspace so that the same
version can be used by contributors and CI. You can run `bazel run
:buildifier_check` to check for necessary changes and `bazel run
:buildifier` to apply them.
@drigz
Copy link
Contributor Author

drigz commented Nov 7, 2018

I'm assuming that, for now, running both buildifier and gazelle is useful, so I've added it to the CI.

@achew22 I followed your lead in preserving --batch, although we could probably make the presubmit faster by dropping --batch and combining :bazel_build and :bazel_test (since test //... implies build //...).

@johanbrandhorst
Copy link
Collaborator

Please revise the CI tests to make sense - I only reproduced the existing travis tests to the best of my ability when creating the circle CI configuration.

@johanbrandhorst
Copy link
Collaborator

LGTM, but I'll let @achew22 have the final say.

`bazel test //...` also builds all targets. These changes should make
the CI steps run a little faster.
@drigz
Copy link
Contributor Author

drigz commented Nov 7, 2018

I tried combining them into one step, which is more efficient but means that if there is a lint error, devs won't see build/test failures. I split them up into separate lint and test steps, but if you want to save ~2 minutes of CI time, you could consider combining them.

@achew22 achew22 merged commit 3ff87af into grpc-ecosystem:master Nov 8, 2018
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
buildifier autoformats the files to give a consistent style. This makes
edits easier for contributors that have their editor set to
autoformat-on-save.

buildifier has been added to the Bazel workspace so that the same
version can be used by contributors and CI. You can run `bazel run
:buildifier_check` to check for necessary changes and `bazel run
:buildifier` to apply them.

`bazel test //...` also builds all targets. These changes should make
the CI steps run a little faster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants