bazelbuild/rules_scala

Use consistent formatting for our BUILD files

ittaiz opened this issue · 9 comments

ittaiz commented

@pauldraper opened this IntelliJ issue bazelbuild/intellij#283 which made me wonder why we don’t do one auto format PR and then try to uphold it.
I seem to remember someone @bazelcon showed a bot to run buildifier on PRs (@mattmoor was it you maybe?)

@johnynek wdyt?

jjudd commented

I'm in favor of using buildifier. We've experimented with it locally, and it looks nice. It's great to not have to think about formatting things and just let buildifier do it.

promiseofcake commented

We use it as well, it's a CI check that will fail-fast if a PR attempts to push with incompatible changes.

buildifier -showlog -mode=check `find command to get the files you'd like in case of excludes`.

I haven't bought into to formatting my 3rdparty directory yet though or linking in via the bazel-deps options.

johnynek commented

we use buildifier internally with a way to build and run it from the current bazel build. I would like something like that. I don't want contributors to have to go install that right away.

sh_binary(
    name = "format_build_files",
    srcs = ["format_build_files.sh"],
    data = [
        "run_buildifier.sh",
        "//:WORKSPACE",
        "@com_github_bazelbuild_buildtools//buildifier",
    ],
    tags = [
        "external",
        "local",
    ],
)

then:

#!/bin/bash
if [ "$(uname -s)" == "Linux" ]; then
   PLATFORM_NAME='linux'
elif [ "$(uname -s)" == "Darwin" ]; then
  PLATFORM_NAME='darwin'
else
  "Your platform $(uname -s) is unsupported, sorry"
  exit 1
fi

# check or fix
mode="$1"

workspace_root="$(dirname $(readlink WORKSPACE))"
buildifier="${PWD}/external/com_github_bazelbuild_buildtools/buildifier/${PLATFORM_NAME}_amd64_stripped/buildifier"
cd "${workspace_root}"
test_exit=0
"${buildifier}" --mode=$mode WORKSPACE || test_exit=1
"${buildifier}" --mode=$mode $(find . -type f -name BUILD) || test_exit=1
"${buildifier}" --mode=$mode $(find . -type f -name BUILD.bazel) || test_exit=1
# https://github.com/bazelbuild/buildtools/issues/206
# "${buildifier}" --mode=check --format_bzl $(find . -type f -name '*.bzl') || test_exit=1
exit "${test_exit}"
promiseofcake commented

I don't want contributors to have to go install that right away.

Nice thanks @johnynek, that will make things a bit cleaner for me.

ittaiz commented
johnynek commented
mattmoor commented

My demo was a /buildify command in Prow (see github.com/kubernetes/test-infra).

It doesn't format for you, it just warns you when things aren't formatted.

It could conceivably send a PR against your fork/branch with the formatting changes.

Alternately you could set up a pre-commit hook in Git to format all files (but a lot of people dislike such magic, and it is no substitute for validation on PRs).

ittaiz commented

I think we can close this thanks to @andyscott :)

  • 🎉1