New buildifier versions need to be told what type of file they're formatting

[chiphogg]

Apparently, buildifier is for more than just BUILD files these days. See here: https://github.com/bazelbuild/buildtools/pull/431/files

The upshot is that, in order to format BUILD files, buildifier needs to know what type of file it's formatting. This is done with the -type flag, as in:

buildifier -type=build

There are probably several different ways we could go here. We could introduce buildifier_options, but that conflicts with some of the desires expressed in #66. Probably the best way would be for codefmt to decide what flags to pass based on the filename, so we would automatically do the right thing for both .bzl and BUILD files... although, this might not work well for older versions of buildifier!

[fowles]

In the past we have done this by having a small invoke to test the version

[fowles]

https://github.com/google/vim-codefmt/blob/master/vroom/clangformat.vroom#L31-L34

[malcolmr]

Note that #100 previously fixed this because buildifier used -path to determine the type. Has it changed the CLI again?

[malcolmr]

$ echo 'cc_library(name="foo", srcs=["foo.cc"], deps=[":foo", ":bar"])' \
    | buildifier -path BUILD
cc_library(
    name = "foo",
    srcs = ["foo.cc"],
    deps = [
        ":bar",
        ":foo",
    ],
)
$ echo 'cc_library(name="foo", srcs=["foo.cc"], deps=[":foo", ":bar"])' \
    | buildifier -path foo.bzl
cc_library(name = "foo", srcs = ["foo.cc"], deps = [":foo", ":bar"])
$

Seems like it is using the path even if that's not what's documented?

[chiphogg]

Sorry, I should have mentioned the version I'm on.

buildifier --version
buildifier version: 0.25.1 
buildifier scm revision: f42fd8fb92a30a55f5e53681975a3d773df8ec7b 

Here are my results:

echo 'cc_library(name="foo", srcs=["foo.cc"], deps=[":foo", ":bar"])'     | buildifier -path BUILD
cc_library(name = "foo", srcs = ["foo.cc"], deps = [":foo", ":bar"])

Here's what I get when I modify that to specify the --type:

echo 'cc_library(name="foo", srcs=["foo.cc"], deps=[":foo", ":bar"])'     | buildifier --type=build
cc_library(
    name = "foo",
    srcs = ["foo.cc"],
    deps = [
        ":bar",
        ":foo",
    ],
)

[malcolmr]

Interesting. What does buildifier --help show you for -type?

I get:

  -type="auto": Input file type: build (for BUILD files), bzl (for .bzl files), default (for generic Starlark files) or auto (default, based on the filename)

[chiphogg]

  -type string
    	Input file type: build (for BUILD files), bzl (for .bzl files), workspace (for WORKSPACE files), default (for generic Starlark files) or auto (default, based on the filename) (default "auto")

What version are you on?

Also, here's where the --type flag appears to have been introduced: bazelbuild/buildtools#232. I mention it because any --type based solution would likely break for versions that are older than this.

[malcolmr]

I was just trying to figure out the version. We seem to be using some internally-built shim, but I think the core code corresponds to https://github.com/bazelbuild/buildtools/commits/89ce10de6b39666331d0d91c0d7e5a146a920cef.

[chiphogg]

OK. Well, I switched to my fork as a temporary workaround. Not ready to make a PR yet, since I think it would break old users!

It looks like your version is much newer than mine, as it's from earlier this month. So maybe this was a bug that got fixed?

[malcolmr]

Ah. Yes, it was fixed recently: bazelbuild/buildtools#681, which looks like it made it into 0.28.0.

(I assume it was broken at some point prior to that, since we know it worked last year.)

[dbarnett]

Okay, I'll call this resolved, but let's reopen if more people run into this and we need some kind of version guard.