bazelbuild/buildtools

A bazel BUILD file formatter and editor

buildtools's Issues

Unable to set srcs to glob with buildozer.

Buildozer assumes the value for the srcs attribute is always a list, preventing the use of globs. For example:
buildozer "set srcs glob(["*"])" $target
produces the attribute
srcs = ["glob([\"*\"])"]

allow running buildifier from other directories

I have buildifier as a repo in my WORKSPACE, and was trying to use it with the command

./scripts/bazel-run.sh @io_bazel_buildifier//buildifier -- -mode=fix -path=my/path/to/target

(where bazel-run.sh is the same one as in the bazel repo that uses bazel run --script_path under the hood)

But that fails to fix up the BUILD in my repo because -path is relative the current working directory and, with all bazel run commands, the current working directory in in the bazel sandbox, not directory of the source code repository.

It's kind of a bummer to build and run bazel repo'ed commands, so it might be nice to be able to set the current working directory back to the root of the source code repo for these commands.

Maybe this is a request for bazel run? Y'all would know better than me.

Prefer `diff -u` to `tkdiff` as the default when running buildifier on a single file from the command line

I believe the relevant code is here:

https://github.com/bazelbuild/buildifier/blob/7679b82ab766294a27b3b439160277286d8efa1d/differ/diff.go#L96-L113

I suspect tkdiff is installed by default at Google on Goobuntu, but tkdiff isn't available by default on OS X, and I'm pretty sure it isn't on Windows (or even vanilla Ubuntu, for that matter).

I think that diff -u is a better default because the output from regular diff does not provide enough information when you pass multiple files to buildifier. Currently, I am running buildifier with BUILDIFIER_DIFF="diff -u" to achieve this, but again, I think it should be the default.

For the code that I highlighted, I believe what's happening is:

• knowMultiDiff := false on line 96.
• if !knowMultiDiff { is true on line 109.
• d.MultiDiff = isatty(1) && os.Getenv("DISPLAY") != "" on 110, such that d.MultiDiff is true.
• if d.MultiDiff { is true on line 112, so d.Cmd = "tkdiff" on line 113.

Buildifier can't format a nested tuple expansion inside a list comprehension

PAIRS = [('a', ('b', 'c'))]
[(a, b, c) for (a, (b, c)) in PAIRS]

$ buildifier BUILD
BUILD:2:21: syntax error near (

It seems fine with a single level of tuple expansion, converting it to the non-parenthesized form:

PAIRS = [(
    "a",
    ("b", "c"),
)]

[(a, b) for a, b in PAIRS]

Repo needs a new name

With the addition of buildozer, and soon unused_deps, the repo name should be made more generic.

Our current favorite is buildtools, so that would be
https://github.com/bazelbuild/buildtools

Any objections or better ideas?

certain buildifier runs are not idempotent

after running buildifier, you would expect the 2nd time around to be unchanged/identical, but apparently not for some cases:

example
$ cat > /tmp/dir/BUILD
py_test(
tags = ["manual", "local"], # comment
)

$ cat /tmp/dir/BUILD | buildifier
py_test(
tags = [
"manual",
"local",
], # comment
)

$ cat /tmp/dir/BUILD | buildifier | buildifier
py_test(
tags = [
"manual",
"local", # comment
],
)

Make indent and quote style configurable.

To get this to follow Buck's conventions, I had to make some small changes:

bolinfest@68b6d08

It would be nice if these things were configurable, as we would prefer not to maintain a fork.

Buildifier undoes raw strings

Buildifier turns raw string literals into regular strings, which makes inline shell code and regexps more difficult to read. See for example RobotLocomotion/drake@a4a313e.

Keep sorted not honored

I have the following code:

srcsListExpr := build.ListExpr{
	Comments: build.Comments{
		Before: []build.Comment{
			build.Comment{
				Token: "# keep sorted",
			},
		},
	},
}

which produces

foo(name = "foo_1", srcs =
# keep sorted
[
    "f.bmp",
    "m.bmp",
    "s.bmp",
    "e.bmp",

Buildifier removes some comments

[ # This line will be eaten by buildifier
# but not this line.
cc_binary(
name = "something_%s_useful" % lang,
)
for lang in li]

Make "BUILD" being the name of BUILD files configurable

Buck thinks that its input files should be called BUCK. Since buildozer also doesn't seem to have a way to accept a BUILD file on stdin (and seems to want to know the target to modify, not just the BUILD file path), this makes using buildozer with appropriately-formatted BUCK files difficult.

Return value in check mode is always zero

The point of check mode is to be able to verify the formatting of BUILD files from the CI:

  $ buildifier -showlog -mode=check $(find . -name BUILD -o -name WORKSPACE -type f)
./WORKSPACE # reformat
  $ echo $?
0

I would expect different return code when formatting is wrong. That way I would not need to parse the program output.

Prevent KeyValueExpr in CallExpr

Currently I can place a KeyValueExpr inside a CallExpr which generates invalid code. This should at some point complain.

Buildozer stack-overflows on Windows platforms, when not in WORKSPACE directory

It seems that on Windows platforms, Buildozer will stack-overflow, if no super-directory contains a WORKSPACE file.

Root cause is in the function "func Find(dir string) (string, error) {...}" in the file "buildtools/wspace/workspace.go". The condition:

if dir == "" || dir == "/" || dir == "."

which checks whether the top-level directory has been reached, does not seem to take into account the possibility that the top-level directory is something like "C:". Stack-overflow seems to be the result.

Buildozer doesn't document remove with a single argument correctly

Current documentation:

remove <value(s)>: Removes value(s) from the list attr. The wildcard * matches all attributes. Lists containing none of the value(s) are not modified.

Issues:

1. I would expect this to remove nothing as all lists contain none of the values (vacuously) and thus shouldn't be modified. It instead removes the attribute.
2. I would expect this to not work on non list attributes, but it does and also removes the attribute.

Buildozer needs a conditional operator

Say I need to set testing_only to 1 for all java testing libraries. These libraries are identified by having a //java/junit dependency

buildozer \
'set testing_only 1' \
 $(find -L ~/my_project -name BUILD | sed"s/\/BUILD/:%java_library/" )

I can't identify here any of these testing libraries. It would be great for me if I could add some condition using a 'where' keyword:

buildozer \
'set testing_only 1' \
'where ("//java/junit" in deps and testing_only != 1)'
 $(find -L ~/my_project -name BUILD | sed"s/\/BUILD/:%java_library/" )

The semantics of where is basically to match the entire build rule and, if it doesn't match the condition, then skip it from the atomic mutation.

Multiple 'where' operators would be either 'and'-ed or a parse error.

This would make a ton of refactors much easier instead of doing constant git manipulations to have better specificity especially for BUILD files with many tests.

buildozer: ellipsis doesn't find BUILD.bazel or BUCK files

For example, in the buildtools repo:

$ bazel-bin/buildozer/buildozer '//edit/...:*' "print"
$ cp edit/BUILD.bazel edit/BUILD
$ bazel-bin/buildozer/buildozer '//edit/...:*' "print"
rule "//edit/...:" has no attribute "name"
(missing) load
go_default_library go_library
go_default_test go_test
$

no such package when trying to run `bazel build //buildifier` with a clean repo

I just cloned the repo and ran bazel build //buildifier which gives me the following error

Extracting Bazel installation...
..........
ERROR: /Users/cheister/Development/buildifier/build/BUILD.bazel:4:1: no such package '@org_golang_x_tools//cmd/goyacc': no such package '@io_bazel_rules_go_repository_tools//': Traceback (most recent call last):
	File "/private/var/tmp/_bazel_cheister/bb74d75b8b198c6a8a6a4a726cae2eab/external/io_bazel_rules_go/go/private/go_repositories.bzl", line 85
		_fetch_repository_tools_deps(ctx, goroot, gopath)
	File "/private/var/tmp/_bazel_cheister/bb74d75b8b198c6a8a6a4a726cae2eab/external/io_bazel_rules_go/go/private/go_repositories.bzl", line 54, in _fetch_repository_tools_deps
		ctx.download_and_extract('%s/archive/%s.zip' % (dep.repo,...), <4 more arguments>)
java.io.IOException: Prefix buildifier-81c36a8418cb803d381335c95f8bb419ad1efa27 was given, but not found in the zip and referenced by '//build:parse.y.go_yacc'.
ERROR: Analysis of target '//buildifier:buildifier' failed; build aborted.
INFO: Elapsed time: 48.146s

go version is go version go1.8 darwin/amd64

Buildifier formats inconsistently and contrary to PEP-8

Buildifier apparently wants spaces around named parameters, which is contrary to PEP-8. However, it doesn't enforce them consistently, making it near impossible to maintain code style consistency. It is also lacking vs. PEP-8 in several other ways.

For example:

A = [3 ]
B = [ 42]

def bar(x = None):
    if x > 0:
      bar(x = x - 1)
    print(x)

def foo(
  x=None
        ):
  bar(x=0)

How many style problems/inconsistencies can you spot in the above? There are several, but buildifier sees none.

(If buildifier followed PEP-8, this wouldn't be a problem; pycodestyle could be used to fill in the gaps that buildifier leaves.)

Buildifier should rewrite 0/1 to False/True

I've noticed that Bazel seems to use both 0/1 and False/True as Boolean constants. This is unfortunate. Please pick one (preferably False/True!), and then modify buildifier (which presumably has, or can be readily updated to have, the necessary tooling to know if a 0/1 is an integer or a Boolean) to enforce the decision.

buildozer: moves comment to the wrong place

old BUILD file:
go_library(
name = "loader",
srcs = ["loader.go"],
deps = [":loader-impl"], # keep
)

new BUILD file after
buildozer 'add deps //public' :loader :
go_library(
name = "loader",
srcs = ["loader.go"],
cgo = 1,
deps = [
":loader-impl",
"//public", # keep
],
)

buildozer: 'print label' interacts poorly with ellipsis

For example, in the buildtools repo:

$ bazel-bin/buildozer/buildozer '//edit/...:*' 'print label
//edit/...:go_default_library
//edit/...:go_default_test

The output if you try to use ellipsis with the root directory is even weirder:

bazel-bin/buildozer/buildozer '...:*' "print label"
//...:go_default_library
//...:tests
//...:go_default_library
//...:go_default_library
//...:generatetables
//...:copy_and_fix
//...:go_default_library
//...:go_default_library
//...:go_default_test
//...:go_default_library
//...:go_default_library
//...:buildozer
//...:go_default_library
//...:go_default_test
//...:go_default_library
//...:copy_and_fix
//...:go_default_library
//...:copy_and_fix
//...:go_default_library
//...:go_default_library
//...:unused_deps
//...:generateTablesFile
//...:go_default_library
//...:go_default_library
//...:go_default_test
//...:buildifier
//...:go_default_library
//...:go_default_test

It looks like there is no code to adjust the output label to account for the actual path in use. We just preserve the ellipsis bit in the input label.

(By the way, this doesn't seem to be caused by fe4721c. If you rename BUILD.bazel files to BUILD and check out eaf5ad1 (the preceding commit), this issue still repros.)

Buildifier shouldn't break up blocks of constants with newlines

UBUNTU_MAIN = "http://mirrors.kernel.org/ubuntu/pool/main/"
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"
NODEJS = "http://deb.nodesource.com/node_6.x/pool/main/"

becomes

UBUNTU_MAIN = "http://mirrors.kernel.org/ubuntu/pool/main/"

UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"

NODEJS = "http://deb.nodesource.com/node_6.x/pool/main/"

This is not ideal because a BUILD file with blocks of related constants can become very "fluffy" and difficult to scan over.

Copying the gofmt logic of aligning equals signs in blocks of assignments would be nicer:

UBUNTU_MAIN     = "http://mirrors.kernel.org/ubuntu/pool/main/"
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"
NODEJS          = "http://deb.nodesource.com/node_6.x/pool/main/"

Or, alternatively, just don't add newlines.

Sorting issue.

Hi,

There don't appear to be any flags to control sorting of list items. When I reformat this entry which is requires exactly this order, buildifier sorts it alphabetically and breaks my C compilation due to the wrong ordering of C header files.

deps = [":pub",
        ":dst",
        ":shr",
        ":db",
        ":dc",
        ":gn",
        ":ar",
        ":cs",
        ":cm",
        ":po",
        ":oper",
        ":mp",
        ":ms",
        ":sec",
        "//bb/infgn:pub",
        "//bb/infgn:err",
        "//generated_b:lib",
        "//gdd:lib",
        "@oracle_client//:header_files",
        "@qsel//:header_files",
        ],

Looks great, but breaks my compilation. I also know that my code shouldn't be written with duplicate header file names, but that's 20 years of technical debt for you.

deps = [
    ":ar",
    ":cm",
    ":cs",
    ":db",
    ":dc",
    ":dst",
    ":gn",
    ":mp",
    ":ms",
    ":oper",
    ":po",
    ":pub",
    ":sec",
    ":shr",
    "//bb/infgn:err",
    "//bb/infgn:pub",
    "//gdd:lib",
    "//generated_b:lib",
    "@oracle_client//:header_files",
    "@qsel//:header_files",
],

Syntax error for two-level list comprehension with condition

genrule(
name = "test",
outs = [
"%s/%s.txt" % (dictionary["folder_name"], file_name)
for dictionary in DICTIONARIES if dictionary["enabled"]
for file_name in dictionary["file_names"]
],
cmd = "touch $(OUTS)",
)

Is valid in bazel, but not buildifier's grammar.

Buildozer target syntax explanations are confusing

https://github.com/bazelbuild/buildtools/tree/master/buildozer#targets

• What are the macros described in (but there can be some differences in presence of macros.)? The only thing I can think of is build macros, but I don't see how those matter. An example might be helpful. Unless this means the syntax being described here, but this section already introduces the syntax as Targets.
• Use an asterisk to refer to all rules in a file: //pkg:*. I think this should be package and not file.
• Use ... to refer to all descendant BUILD files in a directory: //pkg/...:*. Do I refer to rules within BUILD files or within packages?
• Document support for relative label syntax support, or at least give an example with one not in absolute form.
• The section explaining the target syntax says that __pkg__ is for referring to the package declaration, but lower down this is contradicted to say that __pkg__ is for referring to all file level concepts (Use //path/to/pkg:__pkg__ as label for file level changes like new_load and new_rule.).

Last release was about 2 months ago

Could you please cut a new release?

Buildozer doesn't have a way to remove an attribute if it has a specific value

I want to clear attributes that are set to a specific value. The use case is that an option was provided that had one default, and the default has switched. I now want to clear out all the settings where the new default is explicitly set.

Improve table overrides

The addition of -tables to buildifier is nice, however there are two issues with #45.

• The ability to override the NamePriority table, even though this can be done internally, was omitted.
• This feature replaces the built-in tables, requiring users to duplicate the build-in tables to the extent they are wanted.

I'm trying to fix buildifier doing some obnoxious reordering of argument names for custom functions (which #34 notes as one of the likely uses of this feature), which is outright prevented by the first point and made more annoying by the second. It seems to me that it would be more useful to merge the user-provided tables, at least for users like myself that want to supplement rather than replace wholesale the built in tables. It seems that disabling any built-in item is possible by setting it to false / 0, so I don't see any reason why there should not be an option to just update the built-in tables with the user-provided entries.

Set //buildifier:buildifier to visibility:public for bazel

In buildifier/BUILD, could we set visibility = ["//visibility:public"] on the go_binary(name = "buildifier", ...) rule?

I would like to bring the buildifier project into my WORKSPACE and refer to that binary as a dependency, but the target defaults to private (at least when using bazel).

Publish gh release binaries

I'd like my team to use this tool without forcing them all to install the go runtime

Buildozer can't print attrs whose values are more complicated expressions

If I have a rule like

test_suite(
    name = "example",
    some_attr = ["foo"] + SOME_CONSTANT,
)

and I run buildozer //:example "print some_attr", it prints (unknown). It seems straightforward to instead print ["foo"] + SOME_CONSTANT using build.FormatString. Is there any reason buildozer shouldn't do this?

Check in generated code

Can you please check in genearted code?

From https://blog.golang.org/generate:

if the containing package is intended for import by go get, once the file is generated (and tested!) it must be checked into the source code repository to be available to clients

Having to generate code does not work well with common dependency management tools in the go ecosystem such as godeps and glide.

Buildozer: Add "key: value" to dict

I couldn't find any way to add a key/value pair to a dictionary attribute. Am I missing something or is this functionality simply not there?

How to pretty-print everything?

Is is possible to pretty-print more aggressively, including python (skylark) blocks? It only seems to cleans up whitelisted content (in tables.go I guess). It does not seem catch syntax errors either. For example:

package main

import (
    "bufio"
    "bytes"
    "fmt"
    bzl "github.com/bazelbuild/buildifier/core"
)

func main() {
        var buf = bytes.Buffer
        out := bufio.NewWriter(&buf)
        bzlIn := "def foo():\n   pass\n"
        nBytes, err := out.WriteString(bzlIn)
        build, err := bzl.Parse("BUILD", out.Bytes())
        bzlOut, err := bzl.Format(build)
}

Is there a recommended approach to pretty-print arbitrary skylark code?

buildozer: WORKSPACE search is surprising for Buck users

Buck doesn't have an equivalent to Bazel's WORKSPACE files. Normally, this isn't a problem -- the search fails and buildozer assumes $PWD is the root directory. Because Mac OS uses a case-insensitive filesystem, this causes surprising behavior for buildozer+Buck users who happen to have a file or directory called workspace in their home directory (e.g., because they've used Eclipse).

Passing -root_dir . (and running from the Buck root) is a workaround, but it's not an obvious one; the same seems true for adding an otherwise-unnecessary WORKSPACE file to the Buck root. Perhaps there is something we can do in Buildozer to make using it easier for Buck users?

def support

I am using build to write .bzl files for our internal generator. I am somewhat puzzled how to handle defs properly. On the one hand there is PythonBlock but on the other hand it does not accept a child Expr.

It seems to me like the only way of doing it is to format Expr and then to manually append it to Token of PythonBlock. Seems a bit strange to me, though.

And of course then in Token value having to replace \n to \n\t.

Buildifier does not sort lists when select is involved

Example:

go_library(
    name = "go_default_library",
    srcs = [
        "z.go",
        "a.go",
    ] + select({
        "//conditions:default": ["conditional.go"],
    }),
)

I think this happens whenever an attribute is an expression involving lists rather than a simple list, but this is mostly how it comes up in Go.

List-editing commands don't work on tuples

If I have

test_suite(
    name = "example",
    other_attr = ("foo", "bar"),
    some_attr = [
        "foo",
        "bar",
    ],
)

then buildozer //:example "remove some_attr bar" does the obvious thing, but buildozer //:example "remove other_attr bar" does nothing.

buildifier -d /tmp/BUILD TkDiff error

After installing tkdiff 4.2 on my kubuntu server I am getting this error.

$ ./buildifier -d -v /tmp/BUILD
Error: you specified 3 file(s) and 0 revision(s)

TkDiff may be started in any of the following ways:

Interactive selection of files to compare:
    tkdiff

Plain files:
    tkdiff FILE1 FILE2

Plain file with conflict markers:
    tkdiff -conflict FILE

Source control (AccuRev, BitKeeper, ClearCase, CVS, Git, Mercurial,  Perforce, PVCS, RCS, SCCS, Subversion)
    tkdiff FILE
    tkdiff -rREV FILE
    tkdiff -rREV1 -rREV2 FILE
    tkdiff OLD-URL[@OLDREV] NEW-URL[@NEWREV] (Subversion)

Additional optional parameters:
    -a ANCESTORFILE 
    -o MERGEOUTPUTFILE 
    -L LEFT_FILE_LABEL [-L RIGHT_FILE_LABEL]
    -d debug

$ ./buildifier -version
buildifier version: redacted
buildifier scm revision: redacted

core build is broken

When installing the gazelle command, these errors occur:

$  go get -u github.com/bazelbuild/rules_go/go/tools/gazelle/gazelle
# github.com/bazelbuild/buildifier/core
../../bazelbuild/buildifier/core/lex.go:142: undefined: yySymType
../../bazelbuild/buildifier/core/lex.go:155: undefined: yySymType
../../bazelbuild/buildifier/core/lex.go:170: undefined: yySymType
../../bazelbuild/buildifier/core/lex.go:433: undefined: _AND
../../bazelbuild/buildifier/core/lex.go:434: undefined: _FOR
../../bazelbuild/buildifier/core/lex.go:435: undefined: _IF
../../bazelbuild/buildifier/core/lex.go:436: undefined: _ELSE
../../bazelbuild/buildifier/core/lex.go:437: undefined: _IN
../../bazelbuild/buildifier/core/lex.go:438: undefined: _IS
../../bazelbuild/buildifier/core/lex.go:439: undefined: _LAMBDA
../../bazelbuild/buildifier/core/lex.go:439: too many errors

I imagine a generated file didn't get checked into the repo.

Go 1.8 removes the go tool yacc command

The bazel build of this repo breaks on Go 1.8 because Go 1.8 removed the go tool yacc command. It's now at https://godoc.org/golang.org/x/tools/cmd/goyacc

Provide releases with debian packages

Would be really nice to be able to install releases instead of having to build.

Specify which version of go is needed

With Debian based systems shipping old versions of go it would be good if you could specify, which minimum or exact version of go is needed for all of this to work.
Especially since there does not seem any distribution by golang itself.

E.g.

go install github.com/bazelbuild/buildifier/buildifier

does not work on my Ubuntu 16.04.1:

can't load package: package github.com/bazelbuild/buildifier/buildifier: cannot find package "github.com/bazelbuild/buildifier/buildifier" in any of:
/usr/lib/go-1.6/src/github.com/bazelbuild/buildifier/buildifier (from $GOROOT)
($GOPATH not set)

Add documentation for Buildozer

In the past 2 days, I heard twice about buildozer. I could only find its source code, I have no idea what this tool is doing.

At least, we should add a comment header in the code or a README in https://github.com/bazelbuild/buildifier/tree/master/buildozer.

Recent commits broke the CI tests

All builds here are yellow now: http://ci.bazel.io/job/buildifier/, test targets //api_proto:api.gen.pb.go_checkshtest and //build_proto:build.gen.pb.go_checkshtest are now broken.

Add a command line option to show the version

When committing a change that reformats files, it is useful to mention which version of buildifier was used.

The version is also useful when reporting issues here.

Add homebrew recipe for buildifier

It would be great to be able to install buildifier via Homebrew.

Support Ranged-Formatting

While working on https://github.com/google/vim-codefmt and adding buildifier support, we discussed adding ranged-format, which is standard for many formatters, and which is useful for formatting diffs.

This isn't a big problem since formatting the whole file works pretty well, but would be nice if it were easy to add.

Provide original path of artifact

Since Bazel can't install things by itself, I have been working on bolting on a kludge to do it. One of the issues I run into is:

install_files(
    ...
    files = [
        "src/foo/example.h",
        "src/bar/example.h",
    ]
)

Now, I need to install these to foo/example.h and bar/example.h. Breaking up the list into multiple install directives would be superfluous repetition at best; in some cases, the headers may be coming from elsewhere (transitive_headers being a worst case).

The problem is, while Bazel knows at the point that it first sees a path that ultimately creates the artifact object that a file is named like e.g. src/foo/example.h, this information is lost to Skylark.

I can generally figure out what this "partial path" should be, but only if the artifact originates in the same context as the rule that needs to know; otherwise, the user has to manually specify a context, which leads to DRY violations.

Please make the original partial path available in Skylark.

See the argument at RobotLocomotion/drake#6347 for additional context.