Support linting all BUILD files with `buildifier_test`

[jfirebaugh]

It seems to me that a common use case for buildifier would be to have a rule that tests that all BUILD files conform to the lint rules. However, I can't seem to figure out how to set this up.

I tried:

load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier_test")

buildifier_test(
    name = "buildifier_test",
)

But this produces:

$ bazel test //bazel/tools:buildifier_test
ERROR: /Users/john/figma/figma/bazel/tools/BUILD.bazel:60:16: in srcs attribute of buildifier_test rule //bazel/tools:buildifier_test: attribute must be non empty
ERROR: /Users/john/figma/figma/bazel/tools/BUILD.bazel:60:16: Analysis of target '//bazel/tools:buildifier_test' failed
ERROR: Analysis of target '//bazel/tools:buildifier_test' failed; build aborted: 

I also tried:

load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier")

buildifier(
    name = "buildifier_test",
    mode = "diff",
)

But this produces a deprecation warning telling me to use the first thing I tried:

$ bazel run //bazel/tools:buildifier_test
DEBUG: /private/var/tmp/_bazel_johnfirebaugh/67beefda950d56283b98d96980e6e332/external/com_github_bazelbuild_buildtools/buildifier/internal/factory.bzl:17:10: DEPRECATION NOTICE: value 'diff' for attribute 'mode' will be removed in the future. Migrate '//bazel/tools:buildifier_test' to buildifier_test.

[sudoforge]

Thanks for opening an issue about this; it's been on my mind for some time. A few comments:

I tried:

load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier_test")

buildifier_test(
    name = "buildifier_test",
)

But this produces:

$ bazel test //bazel/tools:buildifier_test
ERROR: /Users/john/figma/figma/bazel/tools/BUILD.bazel:60:16: in srcs attribute of buildifier_test rule //bazel/tools:buildifier_test: attribute must be non empty
ERROR: /Users/john/figma/figma/bazel/tools/BUILD.bazel:60:16: Analysis of target '//bazel/tools:buildifier_test' failed
ERROR: Analysis of target '//bazel/tools:buildifier_test' failed; build aborted: 

This is expected; the buildifier_test rule requires the srcs attribute be populated with targets comprised of specific file extensions (note that allow_empty is False; this is what ensures that the attribute is not empty).

I also tried:

load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier")

buildifier(
    name = "buildifier_test",
    mode = "diff",
)

But this produces a deprecation warning telling me to use the first thing I tried:

$ bazel run //bazel/tools:buildifier_test
DEBUG: /private/var/tmp/_bazel_johnfirebaugh/67beefda950d56283b98d96980e6e332/external/com_github_bazelbuild_buildtools/buildifier/internal/factory.bzl:17:10: DEPRECATION NOTICE: value 'diff' for attribute 'mode' will be removed in the future. Migrate '//bazel/tools:buildifier_test' to buildifier_test.

You're now using the buildifier rule, which is different from buildifier_test. When buildifier_test was introduced, the decision to add a deprecation notice for specific attribute values was made, however, this does not impact your ability to use the target that way. It has also been requested that the deprecation notice is removed (#1005), and I have submitted a PR doing so (#1072).

---

Given that this issue is requesting that buildifier_test supports running against all files without providing a srcs attribute, I'll address this now:

This is not currently possible. Tests run in a sandbox, and unlike executable rules, cannot break out of the sandbox. I have done a little digging to determine how this is best solved, and believe it should come in the form of an environment variable akin to BUILD_WORKSPACE_DIRECTORY, which is exposed to execution sandboxes, but not test sandboxes. This variable contains the actual path of the workspace directory, and is used within execution sandboxes via cd $BUILD_WORKSPACE_DIRECTORY to break out of the sandbox and execute in the context of the workspace.

[jfirebaugh]

Thank you for the detailed response. I understand that test sandboxing means that buildifier_test cannot currently subsume the complete functionality of buildifier(mode = "diff"). It does make sense then to remove the deprecation warning -- thank you for opening that PR. Until that lands I will just ignore the warning.

[sudoforge]

If you really wanted to avoid the deprecation notice, you can save the patch file from that PR locally, and add it to your repository rule that fetches this workspace via the patches attribute.

The above link and example below assume you are using an http_archive rule to source this repository.

http_archive(
    name = "com_github_bazelbuild_buildtools",
    sha256 = "7cff74ce3150ada3eb95d529cbf94b89a8780214158de3cb5647362642952bb7",
    strip_prefix = "buildtools-a43aed7014c840a4c20c84958f3f15df5da780f5",
    urls = [
        "https://github.com/bazelbuild/buildtools/archive/a43aed7014c840a4c20c84958f3f15df5da780f5.tar.gz",
    ],
    patch_args = [
        "-p1",
    ],
    patches = [
        "//third_party/bazel/buildtools:apply-pr-1072.patch",
    ],
)

... where //third_party/bazel/buildtools:apply-pr-1072.patch is a file that contains the same changes in my PR:

diff --git a/buildifier/internal/factory.bzl b/buildifier/internal/factory.bzl
index 0f60684..f7e166d 100644
--- a/buildifier/internal/factory.bzl
+++ b/buildifier/internal/factory.bzl
@@ -4,18 +4,6 @@ This module contains factory methods for simple rule and implementation generati
 
 load("@bazel_skylib//lib:shell.bzl", "shell")
 
-# buildifier: disable=print
-def _value_deprecation(ctx, attr, value):
-    """
-    Prints a deprecation message related to a specific value for an attr.
-
-    Args:
-      ctx:      The execution context
-      attr:     A String representing the attribute name
-      value:    The deprecated value
-    """
-    print("DEPRECATION NOTICE: value '%s' for attribute '%s' will be removed in the future. Migrate '%s' to buildifier_test." % (value, attr, ctx.label))
-
 # buildifier: disable=print
 def _attr_deprecation(ctx, attr):
     """
@@ -117,9 +105,6 @@ def buildifier_impl_factory(ctx, test_rule = False):
       A DefaultInfo provider
     """
 
-    if not test_rule and ctx.attr.mode in ["check", "diff", "print_if_changed"]:
-        _value_deprecation(ctx, "mode", ctx.attr.mode)
-
     args = [
         "-mode=%s" % ctx.attr.mode,
         "-v=%s" % str(ctx.attr.verbose).lower(),
-- 
2.37.0

A simpler way to do this would be to update the sha256, strip_prefix, and urls attributes to point to my tree, but while my tree will always remain up-to-date, I'd generally recommend against that because you are then relying on a third party identity to both maintain the tree, and to not do anything malicious.

[alexeagle]

I'm studying linters under Bazel right now, and I think hooking into https://github.com/apple/apple_rules_lint is probably the right call here rather than make something specific for buildifier, so that there's a chance of a uniform UI/DX for configuring lints.

[sudoforge]

@alexeagle I've considered that too -- but I'm not sure that makes more sense than exposing the workspace path to the test sandbox, and I've gone back and forth on this.

By hooking into apple_rules_lint, we'd either need to get a buildifier linter accepted, which is likely out of scope for the maintainers, or integrate apple_rules_lint here, which is likely out of scope for the maintainers. This means we'd likely need to maintain this separately, causing users to need com_github_bazelbuild_buildtools, apple_rules_lint, and lint_rules_buildifier, which feels a bit heavy-handed.

I'd like to pick your brain as to your thoughts around exposing the workspace path to the test sandbox and why this would be seen as a buildifier-specific change (yes, it may be originating as a desire for this tool, but I can think of other use cases for it), and why hooking into apple_rules_lint would be preferred. I do understand the desire to have a uniform UX for configuring linters, but I personally haven't seen much adoption of apple_rules_lint and have a hard time calling this the new standard over test rules.

[sudoforge]

this should have been closed by #1092

[alexeagle]

That answer looks wrong to me - you'd change a BUILD file but get a cache hit on the buildifier test because declared inputs are the same?

[sudoforge]

if no_sandbox = True (which is required to operate outside of the sandbox), caching the test is disabled via the no-cache and external tags, which are explicitly included:

def buildifier_test(**kwargs):
    """
    Wrapper for the _buildifier_test rule. Optionally disables sandboxing and caching.

    Args:
      **kwargs: all parameters for _buildifier_test
    """
    if kwargs.get("no_sandbox", False):
        tags = kwargs.get("tags", [])

        # Note: the "external" tag is a workaround for
        # https://github.com/bazelbuild/bazel/issues/15516.
        for t in ["no-sandbox", "no-cache", "external"]:
            if t not in tags:
                tags.append(t)
        kwargs["tags"] = tags
    _buildifier_test(**kwargs)

https://github.com/bazelbuild/buildtools/blob/master/buildifier/buildifier.bzl#L43-L59

[sudoforge]

that said, per our earlier discussion in this thread, i think building explicit lint rules would be beneficial. while i do think the approach @milesdai contributed to buildifier_test improves the ergonomics, it isn't the "final form" i'd want for downstream users of this rule.