Add make-location lint for deprecated $(location) make variables (#1484)
## Summary
- Add a new buildifier lint category, `make-location`, that flags
deprecated `$(location)` and `$(locations)` make variables in BUILD
files.
- Dogfood the rule in this repo by replacing existing uses with
`$(execpath ...)` and enforcing the lint via `//:make_location_lint`.
## Background / deprecation context
Bazel's [`$(location)`](https://bazel.build/reference/be/make-variables)
and [`$(locations)`](https://bazel.build/reference/be/make-variables)
make variables are legacy pre-Starlark synonyms for
[`$(execpath)`](https://bazel.build/reference/be/make-variables) and
[`$(rootpath)`](https://bazel.build/reference/be/make-variables). Which
path they expand to depends on the attribute being expanded, which makes
behavior hard to predict and easy to get wrong.
From the [Make
Variables](https://bazel.build/reference/be/make-variables) reference:
> **`location`**: A synonym for either `execpath` or `rootpath`,
depending on the attribute being expanded. This is legacy pre-Starlark
behavior and **not recommended** unless you really know what it does for
a particular rule. See
[#2475](https://github.com/bazelbuild/bazel/issues/2475) for details.
The underlying inconsistency is discussed in
[bazelbuild/bazel#2475](https://github.com/bazelbuild/bazel/issues/2475)
(e.g. `$(location)` expanding to an exec path in some attributes but a
runfiles path in others). Bazel's docs now steer users toward explicit
variables:
- [`$(execpath ...)`](https://bazel.build/reference/be/make-variables) —
path under the execroot where build actions run
- [`$(rootpath ...)`](https://bazel.build/reference/be/make-variables) —
runfiles-relative path for runtime lookup (prefer [`$(rlocationpath
...)`](https://bazel.build/reference/be/make-variables) for
cross-platform runfiles)
This lint nudges BUILD authors toward those explicit forms instead of
the ambiguous legacy alias.
## Changes
- New `make-location` warning in `warn/warn_bazel.go` (BUILD files only;
does not flag `load("location", ...)`).
- Tests, `WARNINGS.md` / `warnings.textproto` docs, and warning-list
updates.
- Repo fixes: `buildifier/BUILD.bazel`, `buildozer/BUILD.bazel`,
`build/build_defs.bzl`.
- New `buildifier_test` target `//:make_location_lint` added to
`//:tests`.
## Test plan
- [x] `bazel test //warn:warn_test
--test_filter=TestMakeLocationVariable`
- [x] `bazel test //warn/docs:docs_test`
- [x] `bazel test //buildifier/config:config_test`
- [x] `bazel test //:make_location_lint`diff --git a/BUILD.bazel b/BUILD.bazel
index bc2fc5a..e58944a 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -1,9 +1,10 @@
load("@bazel_gazelle//:def.bzl", "gazelle")
-load("//buildifier:def.bzl", "buildifier")
+load("//buildifier:def.bzl", "buildifier", "buildifier_test")
exports_files([
"LICENSE",
"launcher.js",
+ "WORKSPACE",
])
config_setting(
@@ -20,6 +21,7 @@
test_suite(
name = "tests",
tests = [
+ ":make_location_lint",
"//api_proto:api.gen.pb.go_checkshtest",
"//build:build_test",
"//build_proto:build.gen.pb.go_checkshtest",
@@ -45,3 +47,11 @@
buildifier(
name = "buildifier",
)
+
+buildifier_test(
+ name = "make_location_lint",
+ lint_mode = "warn",
+ lint_warnings = ["make-location"],
+ no_sandbox = True,
+ workspace = "//:WORKSPACE",
+)
diff --git a/WARNINGS.md b/WARNINGS.md
index fe55dd0..3e3dab2 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -38,6 +38,7 @@
* [`list-append`](#list-append)
* [`load`](#load)
* [`load-on-top`](#load-on-top)
+ * [`make-location`](#make-location)
* [`module-docstring`](#module-docstring)
* [`name-conventions`](#name-conventions)
* [`native-android`](#native-android)
@@ -761,6 +762,36 @@
--------------------------------------------------------------------------------
+## <a name="make-location"></a>The `$(location)` make variable is deprecated
+
+ * Category name: `make-location`
+ * Automatic fix: no
+ * [Suppress the warning](#suppress): `# buildifier: disable=make-location`
+
+The `$(location)` and `$(locations)` make variables are legacy synonyms for
+`$(execpath)` and `$(rootpath)` whose behavior depends on the attribute being
+expanded. Use `$(execpath ...)` when you need the execution path, or
+`$(rootpath ...)` when you need the runfiles path.
+
+Examples that trigger this warning:
+
+```python
+genrule(
+ name = "example",
+ srcs = [":input"],
+ outs = ["output"],
+ cmd = "cp $(location :input) $@",
+)
+```
+
+Instead, use an explicit make variable:
+
+```python
+cmd = "cp $(execpath :input) $@",
+```
+
+--------------------------------------------------------------------------------
+
## <a name="module-docstring"></a>The file has no module docstring
* Category name: `module-docstring`
diff --git a/build/build_defs.bzl b/build/build_defs.bzl
index 2a2c489..900f8b8 100644
--- a/build/build_defs.bzl
+++ b/build/build_defs.bzl
@@ -144,7 +144,7 @@
srcs = [src + "_check.sh"],
deps = ["@bazel_tools//tools/bash/runfiles"],
data = [src, gen],
- args = ["$(location " + src + ")", "$(location " + gen + ")"],
+ args = ["$(rootpath " + src + ")", "$(rootpath " + gen + ")"],
)
# magic copy rule used to update the checked-in version
@@ -152,7 +152,7 @@
name = src + "_copysh",
srcs = [gen],
outs = [src + "copy.sh"],
- cmd = "echo 'cp $${BUILD_WORKSPACE_DIRECTORY}/$(location " + gen +
+ cmd = "echo 'cp $${BUILD_WORKSPACE_DIRECTORY}/$(execpath " + gen +
") $${BUILD_WORKSPACE_DIRECTORY}/" + native.package_name() + "/" + src + "' > $@",
)
sh_binary(
diff --git a/buildifier/BUILD.bazel b/buildifier/BUILD.bazel
index 4b27215..b248f58 100644
--- a/buildifier/BUILD.bazel
+++ b/buildifier/BUILD.bazel
@@ -93,7 +93,7 @@
size = "small",
srcs = ["integration_test.sh"],
args = [
- "$(location :buildifier)",
+ "$(rootpath :buildifier)",
],
data = [
":buildifier",
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
index ff614fb..ca31dd6 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -77,6 +77,7 @@
// "keyword-positional-params",
// "list-append",
// "load",
+ // "make-location",
// "module-docstring",
// "name-conventions",
// "native-android",
@@ -301,6 +302,7 @@
"keyword-positional-params",
"list-append",
"load",
+ "make-location",
"module-docstring",
"name-conventions",
"native-android",
@@ -403,6 +405,7 @@
"keyword-positional-params",
"list-append",
"load",
+ "make-location",
"module-docstring",
"name-conventions",
"native-android",
@@ -505,6 +508,7 @@
"keyword-positional-params",
"list-append",
"load",
+ "make-location",
"module-docstring",
"name-conventions",
"native-android",
@@ -607,6 +611,7 @@
"keyword-positional-params",
"list-append",
"load",
+ "make-location",
"module-docstring",
"name-conventions",
"native-android",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 2bf906c..fb7f323 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -311,6 +311,7 @@
"keyword-positional-params",
"list-append",
"load",
+ "make-location",
"module-docstring",
"name-conventions",
"native-android",
diff --git a/buildozer/BUILD.bazel b/buildozer/BUILD.bazel
index 7815553..667b178 100644
--- a/buildozer/BUILD.bazel
+++ b/buildozer/BUILD.bazel
@@ -28,7 +28,7 @@
size = "small",
srcs = ["buildozer_test.sh"],
args = [
- "$(location :buildozer)",
+ "$(rootpath :buildozer)",
],
data = [
"test_common.sh",
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index 8be362e..ab26893 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -502,6 +502,30 @@
}
warnings: {
+ name: "make-location"
+ header: "The `$(location)` make variable is deprecated"
+ description:
+ "The `$(location)` and `$(locations)` make variables are legacy synonyms for\n"
+ "`$(execpath)` and `$(rootpath)` whose behavior depends on the attribute being\n"
+ "expanded. Use `$(execpath ...)` when you need the execution path, or\n"
+ "`$(rootpath ...)` when you need the runfiles path.\n\n"
+ "Examples that trigger this warning:\n\n"
+ "```python\n"
+ "genrule(\n"
+ " name = \"example\",\n"
+ " srcs = [\":input\"],\n"
+ " outs = [\"output\"],\n"
+ " cmd = \"cp $(location :input) $@\",\n"
+ ")\n"
+ "```\n\n"
+ "Instead, use an explicit make variable:\n\n"
+ "```python\n"
+ "cmd = \"cp $(execpath :input) $@\",\n"
+ "```"
+ autofix: false
+}
+
+warnings: {
name: "module-docstring"
header: "The file has no module docstring"
description:
diff --git a/warn/warn.go b/warn/warn.go
index 82d5a42..a221ff4 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -146,6 +146,7 @@
"keyword-positional-params": keywordPositionalParametersWarning,
"list-append": listAppendWarning,
"load": unusedLoadWarning,
+ "make-location": makeLocationVariableWarning,
"module-docstring": moduleDocstringWarning,
"name-conventions": nameConventionsWarning,
"native-build": nativeInBuildFilesWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index be6000c..555e4aa 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -20,11 +20,15 @@
import (
"fmt"
+ "regexp"
"strings"
"github.com/bazelbuild/buildtools/build"
)
+// locationMakeVariableRe matches deprecated $(location) and $(locations) make variables.
+var locationMakeVariableRe = regexp.MustCompile(`\$\(locations?(?:\s[^)]*)?\)`)
+
func constantGlobPatternWarning(patterns *build.ListExpr) []*LinterFinding {
findings := []*LinterFinding{}
for _, expr := range patterns.List {
@@ -256,6 +260,26 @@
return findings
}
+func makeLocationVariableWarning(f *build.File) []*LinterFinding {
+ if f.Type != build.TypeBuild {
+ return nil
+ }
+
+ findings := []*LinterFinding{}
+ build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+ stringExpr, ok := expr.(*build.StringExpr)
+ if !ok {
+ return
+ }
+ if locationMakeVariableRe.MatchString(stringExpr.Value) {
+ findings = append(findings,
+ makeLinterFinding(stringExpr,
+ `The "$(location)" and "$(locations)" make variables are deprecated. Use "$(execpath ...)" or "$(rootpath ...)" instead.`))
+ }
+ })
+ return findings
+}
+
func externalPathWarning(f *build.File) []*LinterFinding {
if f.Type == build.TypeDefault {
// Only applicable to Bazel files
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index 9201cea..d6e495c 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -289,3 +289,41 @@
},
scopeBazel)
}
+
+func TestMakeLocationVariable(t *testing.T) {
+ checkFindings(t, "make-location", `
+genrule(
+ name = "a",
+ srcs = [":foo"],
+ outs = ["out"],
+ cmd = "cp $(location :foo) $@",
+)
+
+genrule(
+ name = "b",
+ srcs = [":a", ":b"],
+ outs = ["out"],
+ cmd = "cat $(locations :a :b) > $@",
+)
+
+genrule(
+ name = "c",
+ srcs = [":foo"],
+ outs = ["out"],
+ cmd = "cp $$(location :foo) $$@",
+)
+
+cc_test(
+ name = "d",
+ args = ["--config=$(execpath :cfg)"],
+)
+
+load("location", "symbol")
+`,
+ []string{
+ `:5: The "$(location)" and "$(locations)" make variables are deprecated. Use "$(execpath ...)" or "$(rootpath ...)" instead.`,
+ `:12: The "$(location)" and "$(locations)" make variables are deprecated. Use "$(execpath ...)" or "$(rootpath ...)" instead.`,
+ `:19: The "$(location)" and "$(locations)" make variables are deprecated. Use "$(execpath ...)" or "$(rootpath ...)" instead.`,
+ },
+ scopeBuild)
+}
