Buildifier reformats assignment ignoring "# buildifier: leave-alone"

[artem-zinnatullin]

Hi, I have a .bzl file that I need to keep compatible with both Starlark and Bash

The original contents of the file:

# buildifier: leave-alone
MY_VAR="value"

However after running buildifier -v -lint fix -mode fix against this file (we do that as pre-commit hook) it still reformats the assignment by adding spaces aroud =:

# buildifier: leave-alone
MY_VAR = "value"

= is invalid in Bash :(

As mentioned here https://github.com/bazelbuild/buildtools/issues/806#issuecomment-600644003 https://github.com/bazelbuild/buildtools/blob/195b83e94906df245fdc44da27b1be5489087025/build/rewrite.go#L128-L129, I used buildifier: leave-alone comment, but it still reformats it.

Looks like a bug, wdyt?

---

Notes:

• I've tried to find a category that reformatting falls in, but couldn't figure out after reading https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#buildifier-warnings
• I've tried to make buildifier print JSON to figure out the category, but that doesn't seem to work either:

  uildifier --format=json --type=check myfile.bzl                                                                                      
  uildifier: unrecognized input type check; valid types are build, bzl, workspace, default, auto
  

  I'll submit a PR for this, should be --mode=check it seems (submitted PR https://github.com/bazelbuild/buildtools/pull/891)

Running on macOS

buildifier version: 3.4.0
buildifier scm revision: b1667ff58f714d13c2bba6823d6c52214705508f

[pmbethe09]

leave-alone is a directive to stop sorting or label-shortening, but as mentioned in the linked comment, "It's not possible to disable formatting completely because formatting itself is not supposed to change the semantics"

So at the moment, this is working-as-intended.

I don't believe this case has ever been explored -- a .bzl file that also needs to be a bash file. Why is that required?

On Tue, Aug 25, 2020 at 9:36 PM Artem Zinnatullin :slowpoke: < [email protected]> wrote:

Hi, I have a .bzl file that I need to keep compatible with both Starlark and Bash

The original contents of the file:

buildifier: leave-alone

MY_VAR="value"

However after running buildifier -v -lint fix -mode fix against this file (we do that as pre-commit hook) it still reformats the assignment by adding spaces aroud =:

buildifier: leave-alone

MY_VAR = "value"

= is invalid in Bash :(

As mentioned here #806 (comment) https://github.com/bazelbuild/buildtools/issues/806#issuecomment-600644003 https://github.com/bazelbuild/buildtools/blob/195b83e94906df245fdc44da27b1be5489087025/build/rewrite.go#L128-L129, I used buildifier: leave-alone comment, but it still reformats it.

Looks like a bug, wdyt?

Notes:

• I've tried to find a category that reformatting falls in, but couldn't figure out after reading https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#buildifier-warnings
• I've tried to make buildifier print JSON to figure out the category, but that doesn't seem to work either:

buildifier --format=json --type=check myfile.bzl                                                                                       buildifier: unrecognized input type check; valid types are build, bzl, workspace, default, auto

I'll submit a PR for this, should be --mode=check it seems

Running on macOS

buildifier version: 3.4.0 buildifier scm revision: b1667ff58f714d13c2bba6823d6c52214705508f

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/buildtools/issues/890, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACYX5UVQUZVZEZYKCKBLFO3SCRRIHANCNFSM4QLIT3JA .

[artem-zinnatullin]

Ah, that's unfortunate, since Buildifier doesn't support file exclusion either https://github.com/bazelbuild/buildtools/issues/801 I'll have to figure out another way..

I don't believe this case has ever been explored -- a .bzl file that also needs to be a bash file. Why is that required?

It's not required in general of course, but in our case it's a build config constant that we need to read both in Bash and Starlark to properly configure the build.

To be more precise, we have a giant set of dependencies that is another Bazel project that we were consuming as set of Maven libraries (which is not very efficient with https://github.com/bazelbuild/rules_jvm_external), so I'm now migrating it to be a dependency built from sources — as a composite build.

The way I'm doing it is that I have this .bzl file:

# Valid values are "maven" or "source"
THAT_ONE_GIANT_DEP_CONSUMPTION_MODE="source"

And then depending on it in our bazelw (wrapper) I either add maven_install or composite build declarations and also do some rewiring in our Bazel macros.

That's why I need that .bzl file to be compatible with both Bash and Starlark.

Hope this explains the use case :)

[laurentlb]

That's working as intended.

If you like hacks, you can have a shell script that inserts the spaces around = before executing the file.

[mzeren-vmw]

Another motivating case: We have tables (arrays of tuples) in starlark. For review-ability we require that the columns align.

[mzeren-vmw]

Currently we cannot enforce "buildifier-clean" changes in our internal review tool unless we rename files to something like my_table.no-buildifier.bzl and filter out *.no-buildifier.bzl in our review automation.

It would be much better to have an annotation in a file to disable buildifier. Similar to // clang-format off.

[laurentlb]

Trying to store all the whitespace information would add a lot of complexity. The goal of the tool is to make everything consistent, which means it discards personal preferences such as alignment.

[mzeren-vmw]

From our point of view when looking at O(100) lines of tabular data (file paths and file attributes in this case) vertical alignment is critical for reviewing a change in the context of the other lines. An alternative would be to be able to load() a file with a different extension?

[aiuto]

I think the kind of ignore you want could be done relatively easily with a # buildifier:ignore tag that essentially turned the comment it is in until the next blank line into a comment that was not touched in any way. E.g.

# buildifier:ignore
my_list=[
  ["//abc:def",     "foo",          1],
  ["xyz",           "barbaz",       2],
]

... 

[joshburkart]

Every other formatter/linter I'm aware of (clang-format, yapf, autopep8, pylint, clang-tidy) provides a mechanism to ignore certain lines... I ended up on this ticket because I encountered a case where buildifier made an incorrect change that invalidated a file, so I think this is a desirable feature.

FWIW the line it changed went from this

    kwargs["deps"] += ["//some/special:target"]

to this

    kwargs["deps"].append("//some/special:target")

which was invalid since kwargs["deps"] was a select(...) which doesn't support appending:

ERROR: package contains errors: [redacted]
ERROR: /home/jburkart/[redacted]: Traceback (most recent call last):
        File "/home/jburkart/[redacted]",
                [redacted]
        File "/home/jburkart/[redacted]", in [redacted]
                kwargs["deps"].append
'select' value has no field or method 'append'

Because there's no way to disable formatting, I made this convoluted replacement:

    kwargs["deps"] = kwargs["deps"] + ["//some/special:target"]

[laurentlb]

The .append replacement is part of the linter, not the formatter.

I think it happens when you use --lint=fix. You should be able to disable list-append. See https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md#linter

The formatter should be very safe and never break an existing file. The linter uses heuristics and can make semantic changes.

[joshburkart]

Thanks for the workaround! Seems preferable to not need to disable this particular fix for an entire project, though, so I still advocate for line- or file-specific disabling...

[vladmos]

You can disable individual findings by # buildifier: disable=list-append (that will suppress both warnings and fixes).

[joshburkart]

Awesome, totally worked. (Documented somewhere?) Guess I was misunderstanding the scope of this bug report. Thank you!

[aiuto]

Just for fun, I decided to spend a few hours tinkering with a buildifier:noformat option. This would apply to the next expression. The basic idea is

• we use the Positions in nearly every expression and subpart of more complex elements
• instead of printing the canonical white space we advance from the logical end position of the previous element up to the logical start of the next element.

By "logical" position, I mean that if the previous expression ended on line 4, rune 5, and if the next one was at line 5, rune 10, then we need to emit a newllne because the logical line changed. We can't use physical lines because refomatted code above would throw everything off.

Problem points:

• I am counting bytes instead of runes. This is easy enough to correct because it is all in the advance() method
• Presence of trailing commas on lists and dicts is lost.
• Probably every major type of expression needs modification.
• I have not yet specified the expected behavior.

Without too much work I got it to handle this pretty well

    print_test.go:213: formatted testdata/067.golden with parser build incorrectly: diff shows -067.golden, +ours
    diff.go:62: --- /tmp/testdiff308894500	2020-10-19 20:36:14.852600029 +0000
        +++ /tmp/testdiff420145203	2020-10-19 20:36:14.852600029 +0000
        @@ -1,7 +1,7 @@
         # buildifier:noformat
         L1  =  [
             ["c",       "file_cc",     {11111:  2}],
        -    ["aaaa",    "file_aaaa",   {33:    44}],
        +    ["aaaa",    "file_aaaa",   {33:    44}]
         ]

         f = 1

Notice the extra spacing around the equals. We already have the position of the operator in binary expressions, so advance()-ing to it and then to the head of the list was trivial.

It is not ready for a PR, but someone might want to build on it. https://github.com/aiuto/buildtools/tree/noformat

[achew22]

Another option for handling this would be to create the common variable in a .bzl file and use a sh_binary that depends on a genrule that puts the constant into a file. Then you don't need to have multi language compatibility in the formatter.

[vladmos]

@joshburkart It's documented at the same page where all warnings are documented: https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#how-to-disable-warnings

This section is easy to miss because you need to scroll from a warning description. We may need to restructure the page somehow.

[joshburkart]

Ahh got it. I think info on how to disable a warning from within a BUILD file would be most appropriate to add to this section: https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md#linter

And then make https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#how-to-disable-warnings just info about what all the warnings are. Seem reasonable?