buildtools icon indicating copy to clipboard operation
buildtools copied to clipboard

buildifier examines .git

Open jeffalder opened this issue 1 year ago • 4 comments

Buildifier attempts to scan the entire working directory. If you have a git repo, it looks in .git as well. That means that if your remote has a branch called foobar.bzl, buildifier fix will see that it is an invalid value.

Note that you can use exclude_patterns BUT then:

Error in fail: Cannot use 'exclude_patterns' in a test rule without 'no_sandbox'

And then if you set no_sandbox:

Error in fail: Cannot use 'no_sandbox' without a 'workspace'

And this is where we are stuck. We have tried multiple formats for this parameter: workspace = "WORKSPACE", workspace = "/home/jeff/mycheckout/WORKSPACE", workspace = "//WORKSPACE" and more. There is no clear documentation for this and we have been unable to move beyond this. If someone can clue me into the correct syntax I'll happily update and PR the docs. (Of course, I don't quite understand why exclude_patterns requires no_sandbox, or why the workspace can't default to the current workspace.)

jeffalder avatar Oct 02 '24 19:10 jeffalder

workspace should be the label to a WORKSPACE or MODULE file. In your case you likely want: //:WORKSPACE if you have a non-bzlmod project, otherwise: workspace = //:MODULE.bazel.

Here is what our buildifier_test looks like:

buildifier_test(
    name = "buildifier-check",
    exclude_patterns = ["./.git/*"],
    lint_mode = "warn",
    lint_warnings = _BUILDIFIER_WARNINGS,
    mode = "check",
    no_sandbox = True,
    workspace = "//:MODULE.bazel",
)

luispadron avatar Apr 04 '25 23:04 luispadron

@luispadron Thanks! I'll give that a shot. I didn't realize that -- which means there are some gaps in tooling and documentation, along with sensible defaults like "exclude .git and other SCM directories".

jeffalder avatar Apr 05 '25 01:04 jeffalder

@luispadron that did it with one extra change:

diff --git a/BUILD b/BUILD
index 366b64145..c73f341a5 100644
--- a/BUILD
+++ b/BUILD
@@ -169,4 +169,7 @@ filegroup(
 # gazelle:exclude src/test/java
 # gazelle:exclude src/test/util

-exports_files(["BUILD"])
+exports_files([
+    "BUILD",
+    "WORKSPACE",
+])
diff --git a/bazel/BUILD b/bazel/BUILD
index b8da0a596..29146319a 100644
--- a/bazel/BUILD
+++ b/bazel/BUILD
@@ -11,6 +11,12 @@ buildifier(

 buildifier_test(
     name = "buildifier-check",
+    exclude_patterns = [
+        "./.git/*",
+        "./.ijwb/*",
+    ],
     mode = "check",
+    no_sandbox = True,
     tags = ["lint"],
+    workspace = "//:WORKSPACE",
 )

jeffalder avatar Apr 05 '25 01:04 jeffalder

Nice! Yeah I think a bit of the confusion was using the wrong label: //WORKSPACE in this case you needed //:WORKSPACE as the former means a directory (package) named WORKSPACE.

Agree though buildifier docs need some love in general

luispadron avatar Apr 05 '25 01:04 luispadron