search

bazelbuild / buildtools

A bazel BUILD file formatter and editor
Apache License 2.0
1.01k stars 415 forks source link

buildifier_test is not a suitable replacement for `mode = "check"` #1005

Open djmarcin opened 3 years ago

djmarcin commented 3 years ago

mode = "check" emits a deprecation warning. However I don't think that this should be deprecated because the replacement is not nearly as useful.

Since buildifier_test requires a dependency on every starlark file in the workspace (which you can't easily glob due to glob not crossing package boundaries) it is very easy to forget to add to the central list when you add new BUILD files and is therefore error prone / not very user friendly.

Contrast with the buildifier rule with mode = "check" which can be run by CI and will always find every file in the workspace without any additional work once initially configured. The only thing it doesn't have going for it is that it doesn't automatically run when you run bazel test //....

Please don't remove mode = "check" without an equally user-friendly replacement.

sudoforge commented 1 year ago

hey @djmarcin, checking in here to see if you still think this is relevant after #1092 -- you can set the buildifier_test rule up so that it escapes the sandbox and operates on all files without having to supply srcs. as an example:

buildifier_test(
    name = "example",
    size = "small",
    timeout = "short",
    lint_mode = "warn",
    mode = "diff",
    no_sandbox = True,
    workspace = "//:WORKSPACE",
)
djmarcin commented 1 year ago

I’m not actively using bazel at the moment so I can’t verify, but from the description it sounds like what I was after. Thanks!

limdor commented 8 months ago

hey @djmarcin, checking in here to see if you still think this is relevant after #1092 -- you can set the buildifier_test rule up so that it escapes the sandbox and operates on all files without having to supply srcs. as an example:

buildifier_test(
    name = "example",
    size = "small",
    timeout = "short",
    lint_mode = "warn",
    mode = "diff",
    no_sandbox = True,
    workspace = "//:WORKSPACE",
)

I wanted to replace buildifier for buildifier_test and when doing so I just tried out and I can confirm that it works. What is a bit hard to find is the documentation for it, I created https://github.com/bazelbuild/buildtools/issues/1225