Revert "updates (#1386)" (#1471)
## Buildtools PR checklist
- [x] The code in this PR is covered by unit/integration tests.
- [x] I have tested these changes and provide testing instructions
below.
- [x] I have either responded to, or resolved all Gemini comments on the
PR.
- [x] I have read Google Eng Practices on [Small
Changes](https://google.github.io/eng-practices/review/developer/small-cls.html),
this PR either follows these guidelines or the description provides
reasoning for why they can not be followed.
## Description
This reverts commit 4006b543a694f6cf77c2d8cc188c5f53f3bac1d9. As
reported in
https://github.com/bazelbuild/buildtools/pull/1386#issuecomment-3838906721
and below, this has numerous false positives and causes a lot of churn.
The checks could potentially still be useful if scoped far more tightly,
but determining this scope isn't trivial, hence a revert.
diff --git a/WARNINGS.md b/WARNINGS.md
index 3e3dab2..0dfb4da 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -13,7 +13,6 @@
* [`attr-single-file`](#attr-single-file)
* [`build-args-kwargs`](#build-args-kwargs)
* [`bzl-visibility`](#bzl-visibility)
- * [`canonical-repository`](#canonical-repository)
* [`confusing-name`](#confusing-name)
* [`constant-glob`](#constant-glob)
* [`ctx-actions`](#ctx-actions)
@@ -25,7 +24,6 @@
* [`dict-concatenation`](#dict-concatenation)
* [`dict-method-named-arg`](#dict-method-named-arg)
* [`duplicated-name`](#duplicated-name)
- * [`external-path`](#external-path)
* [`filetype`](#filetype)
* [`function-docstring`](#function-docstring)
* [`function-docstring-args`](#function-docstring-args)
@@ -284,35 +282,6 @@
--------------------------------------------------------------------------------
-## <a name="canonical-repository"></a>String contains `@@` which indicates a canonical repository name reference that should be avoided
-
- * Category name: `canonical-repository`
- * Automatic fix: no
- * [Suppress the warning](#suppress): `# buildifier: disable=canonical-repository`
-
-Using canonical repository names (with `@@` prefix) makes BUILD files fragile
-to repository mapping changes and external dependency updates. Canonical names
-are internal implementation details that can change between Bazel versions
-or when external dependencies are updated.
-
-Instead of using canonical names like:
-
-```python
-load("@@rules_go//go:def.bzl", "go_library")
-deps = ["@@protobuf~5.27.0//src:message"]
-```
-
-Use apparent names with single `@`:
-
-```python
-load("@rules_go//go:def.bzl", "go_library")
-deps = ["@protobuf//src:message"]
-```
-
-This makes your BUILD files more maintainable and resilient to changes.
-
---------------------------------------------------------------------------------
-
## <a name="confusing-name"></a>Never use `l`, `I`, or `O` as names
* Category name: `confusing-name`
@@ -550,40 +519,6 @@
--------------------------------------------------------------------------------
-## <a name="external-path"></a>String contains `/external/` which may indicate a dependency on external repositories that could be fragile
-
- * Category name: `external-path`
- * Automatic fix: no
- * [Suppress the warning](#suppress): `# buildifier: disable=external-path`
-
-Using `/external/` paths may indicate a hard-coded dependency on external
-repository locations, which can be fragile and break when external dependencies
-are updated or reorganized.
-
-Paths containing `/external/` (without a leading `//`) may indicate:
-- Direct file system paths to external repositories
-- Dependencies that bypass Bazel's dependency management
-- Code that relies on Bazel's internal directory structure
-
-Examples that trigger this warning:
-
-```python
-srcs = ["/external/some_repo/file.h"]
-data = ["path/external/repo/data.txt"]
-```
-
-Instead, use proper Bazel labels:
-
-```python
-srcs = ["@some_repo//file.h"]
-data = ["@repo//path:data.txt"]
-```
-
-Note: This warning does not apply to main repository paths like `//external/...`
-which are legitimate Bazel labels.
-
---------------------------------------------------------------------------------
-
## <a name="filetype"></a>The `FileType` function is deprecated
* Category name: `filetype`
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
index 9b5264d..a9b0877 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -53,7 +53,6 @@
// "attr-single-file",
// "build-args-kwargs",
// "bzl-visibility",
- // "canonical-repository",
// "confusing-name",
// "constant-glob",
// "ctx-actions",
@@ -65,7 +64,6 @@
// "dict-concatenation",
// "dict-method-named-arg",
// "duplicated-name",
- // "external-path",
// "filetype",
// "function-docstring",
// "function-docstring-args",
@@ -280,7 +278,6 @@
"attr-single-file",
"build-args-kwargs",
"bzl-visibility",
- "canonical-repository",
"confusing-name",
"constant-glob",
"ctx-actions",
@@ -292,7 +289,6 @@
"dict-concatenation",
"dict-method-named-arg",
"duplicated-name",
- "external-path",
"filetype",
"function-docstring",
"function-docstring-args",
@@ -383,7 +379,6 @@
"attr-single-file",
"build-args-kwargs",
"bzl-visibility",
- "canonical-repository",
"confusing-name",
"constant-glob",
"ctx-actions",
@@ -395,7 +390,6 @@
"dict-concatenation",
"dict-method-named-arg",
"duplicated-name",
- "external-path",
"filetype",
"function-docstring",
"function-docstring-args",
@@ -486,7 +480,6 @@
"attr-single-file",
"build-args-kwargs",
"bzl-visibility",
- "canonical-repository",
"confusing-name",
"constant-glob",
"ctx-actions",
@@ -498,7 +491,6 @@
"dict-concatenation",
"dict-method-named-arg",
"duplicated-name",
- "external-path",
"filetype",
"function-docstring",
"function-docstring-args",
@@ -589,7 +581,6 @@
"attr-single-file",
"build-args-kwargs",
"bzl-visibility",
- "canonical-repository",
"confusing-name",
"constant-glob",
"ctx-actions",
@@ -601,7 +592,6 @@
"dict-concatenation",
"dict-method-named-arg",
"duplicated-name",
- "external-path",
"filetype",
"function-docstring",
"function-docstring-args",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index e0d2976..d6da2aa 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -287,7 +287,6 @@
"attr-single-file",
"build-args-kwargs",
"bzl-visibility",
- "canonical-repository",
"confusing-name",
"constant-glob",
"ctx-actions",
@@ -299,7 +298,6 @@
"dict-concatenation",
"dict-method-named-arg",
"duplicated-name",
- "external-path",
"filetype",
"function-docstring",
"function-docstring-args",
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index ab26893..e24c95f 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -115,28 +115,6 @@
}
warnings: {
- name: "canonical-repository"
- header: "String contains `@@` which indicates a canonical repository name reference that should be avoided"
- description:
- "Using canonical repository names (with `@@` prefix) makes BUILD files fragile\n"
- "to repository mapping changes and external dependency updates. Canonical names\n"
- "are internal implementation details that can change between Bazel versions\n"
- "or when external dependencies are updated.\n\n"
- "Instead of using canonical names like:\n\n"
- "```python\n"
- "load(\"@@rules_go//go:def.bzl\", \"go_library\")\n"
- "deps = [\"@@protobuf~5.27.0//src:message\"]\n"
- "```\n\n"
- "Use apparent names with single `@`:\n\n"
- "```python\n"
- "load(\"@rules_go//go:def.bzl\", \"go_library\")\n"
- "deps = [\"@protobuf//src:message\"]\n"
- "```\n\n"
- "This makes your BUILD files more maintainable and resilient to changes."
- autofix: false
-}
-
-warnings: {
name: "confusing-name"
header: "Never use `l`, `I`, or `O` as names"
description: "The names `l`, `I`, or `O` can be easily confused with `I`, `l`, or `0` correspondingly."
@@ -325,32 +303,6 @@
}
warnings: {
- name: "external-path"
- header: "String contains `/external/` which may indicate a dependency on external repositories that could be fragile"
- description:
- "Using `/external/` paths may indicate a hard-coded dependency on external\n"
- "repository locations, which can be fragile and break when external dependencies\n"
- "are updated or reorganized.\n\n"
- "Paths containing `/external/` (without a leading `//`) may indicate:\n"
- "- Direct file system paths to external repositories\n"
- "- Dependencies that bypass Bazel's dependency management\n"
- "- Code that relies on Bazel's internal directory structure\n\n"
- "Examples that trigger this warning:\n\n"
- "```python\n"
- "srcs = [\"/external/some_repo/file.h\"]\n"
- "data = [\"path/external/repo/data.txt\"]\n"
- "```\n\n"
- "Instead, use proper Bazel labels:\n\n"
- "```python\n"
- "srcs = [\"@some_repo//file.h\"]\n"
- "data = [\"@repo//path:data.txt\"]\n"
- "```\n\n"
- "Note: This warning does not apply to main repository paths like `//external/...`\n"
- "which are legitimate Bazel labels."
- autofix: false
-}
-
-warnings: {
name: "filetype"
header: "The `FileType` function is deprecated"
description:
diff --git a/warn/warn.go b/warn/warn.go
index 2ec3cdb..7c9dd70 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -125,7 +125,6 @@
"attr-output-default": attrOutputDefaultWarning,
"attr-single-file": attrSingleFileWarning,
"build-args-kwargs": argsKwargsInBuildFilesWarning,
- "canonical-repository": canonicalRepositoryWarning,
"confusing-name": confusingNameWarning,
"constant-glob": constantGlobWarning,
"ctx-actions": ctxActionsWarning,
@@ -136,7 +135,6 @@
"dict-method-named-arg": dictMethodNamedArgWarning,
"dict-concatenation": dictionaryConcatenationWarning,
"duplicated-name": duplicatedNameWarning,
- "external-path": externalPathWarning,
"filetype": fileTypeWarning,
"function-docstring": functionDocstringWarning,
"function-docstring-header": functionDocstringHeaderWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 555e4aa..51c2add 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -279,46 +279,3 @@
})
return findings
}
-
-func externalPathWarning(f *build.File) []*LinterFinding {
- if f.Type == build.TypeDefault {
- // Only applicable to Bazel files
- return nil
- }
-
- findings := []*LinterFinding{}
- build.Walk(f, func(expr build.Expr, stack []build.Expr) {
- // Look for any string expression that contains "/external/"
- stringExpr, ok := expr.(*build.StringExpr)
- if !ok {
- return
- }
- // Warn for "/external/" but not if "//" appears in the string (main repository paths)
- if strings.Contains(stringExpr.Value, "/external/") && !strings.Contains(stringExpr.Value, "//") {
- findings = append(findings,
- makeLinterFinding(stringExpr, `String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`))
- }
- })
- return findings
-}
-
-func canonicalRepositoryWarning(f *build.File) []*LinterFinding {
- if f.Type == build.TypeDefault {
- // Only applicable to Bazel files
- return nil
- }
-
- findings := []*LinterFinding{}
- build.Walk(f, func(expr build.Expr, stack []build.Expr) {
- // Look for any string expression that contains "@@"
- stringExpr, ok := expr.(*build.StringExpr)
- if !ok {
- return
- }
- if strings.Contains(stringExpr.Value, "@@") {
- findings = append(findings,
- makeLinterFinding(stringExpr, `String contains "@@" which indicates a canonical repository name reference that should be avoided.`))
- }
- })
- return findings
-}
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index d6e495c..329ace8 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -208,88 +208,6 @@
scopeBazel)
}
-func TestExternalPathWarning(t *testing.T) {
- checkFindings(t, "external-path", `
-cc_library(
- name = "foo",
- srcs = ["//external/com_google_protobuf:src/google/protobuf/message.h"],
-)
-
-py_binary(
- name = "tool",
- srcs = ["tool.py"],
- data = ["/external/some_repo/data.txt"],
-)
-
-java_library(
- name = "lib",
- srcs = glob(["*.java"]),
- deps = ["@maven//:org_apache_commons_commons_lang3"],
-)
-
-filegroup(
- name = "configs",
- srcs = ["config.txt"],
-)
-
-some_rule(
- arg1 = "normal/path/file.txt",
- arg2 = "/external/repo/file.py",
- arg3 = ["file1.txt", "/external/another/file.cc"],
-)`,
- []string{
- `:9: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`,
- `:25: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`,
- `:26: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`,
- },
- scopeBazel)
-
- // Test cases that should NOT warn (main repository paths with // prefix)
- checkFindings(t, "external-path", `
-cc_library(
- name = "foo",
- srcs = ["//external/repo/file.h"],
- hdrs = ["//external/another_repo/header.h"],
-)
-
-py_binary(
- name = "tool",
- srcs = ["//external/tools/tool.py"],
- data = ["//some/path/external/nested/file.txt"],
- args = ["//different/external/location/config.json"],
-)`,
- []string{},
- scopeBazel)
-}
-
-func TestCanonicalRepositoryWarning(t *testing.T) {
- checkFindings(t, "canonical-repository", `
-load("@@rules_go//go:def.bzl", "go_library")
-load("@repo//file.bzl", "symbol") # Should NOT warn (single @)
-load("@@protobuf~5.27.0//src:defs.bzl", "proto_library")
-
-cc_library(
- name = "test",
- deps = ["@@rules_go//cc:toolchain"],
- srcs = ["@@protobuf~5.27.0//src/google/protobuf:message_lite_h"],
-)
-
-py_binary(
- name = "tool",
- srcs = ["tool.py"],
- data = ["@repo//file.txt"], # Should NOT warn (single @)
- args = ["@@some_canonical_repo//path:target"],
-)`,
- []string{
- `:1: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
- `:3: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
- `:7: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
- `:8: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
- `:15: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
- },
- scopeBazel)
-}
-
func TestMakeLocationVariable(t *testing.T) {
checkFindings(t, "make-location", `
genrule(
