Skip to content

Commit

Permalink
Add rules_shell fixing (#1303)
Browse files Browse the repository at this point in the history
Currently these are split into 3 rules because 1 rule cannot add 3 load
statements and this repo doesn't have a defs.bzl that exposes them all
  • Loading branch information
keith authored Jan 17, 2025
1 parent 3a48437 commit 3648bec
Show file tree
Hide file tree
Showing 8 changed files with 159 additions and 0 deletions.
33 changes: 33 additions & 0 deletions WARNINGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ Warning categories supported by buildifier's linter:
* [`native-proto-lang-toolchain`](#native-proto-lang-toolchain)
* [`native-proto-lang-toolchain-info`](#native-proto-lang-toolchain-info)
* [`native-py`](#native-py)
* [`native-sh-binary`](#native-sh-binary)
* [`native-sh-library`](#native-sh-library)
* [`native-sh-test`](#native-sh-test)
* [`no-effect`](#no-effect)
* [`out-of-order-load`](#out-of-order-load)
* [`output-group`](#output-group)
Expand Down Expand Up @@ -945,6 +948,36 @@ at the moment it's not required to load Starlark rules.

--------------------------------------------------------------------------------

## <a name="native-sh-binary"></a>sh_binary build rules should be loaded from Starlark

* Category name: `native-sh-binary`
* Automatic fix: yes
* [Suppress the warning](#suppress): `# buildifier: disable=native-sh-binary`

The sh_binary build rules should be loaded from Starlark.

--------------------------------------------------------------------------------

## <a name="native-sh-library"></a>sh_library build rules should be loaded from Starlark

* Category name: `native-sh-library`
* Automatic fix: yes
* [Suppress the warning](#suppress): `# buildifier: disable=native-sh-library`

The sh_library build rules should be loaded from Starlark.

--------------------------------------------------------------------------------

## <a name="native-sh-test"></a>sh_test build rules should be loaded from Starlark

* Category name: `native-sh-test`
* Automatic fix: yes
* [Suppress the warning](#suppress): `# buildifier: disable=native-sh-test`

The sh_test build rules should be loaded from Starlark.

--------------------------------------------------------------------------------

## <a name="no-effect"></a>Expression result is not used

* Category name: `no-effect`
Expand Down
12 changes: 12 additions & 0 deletions buildifier/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ func ExampleExample() {
// "native-proto-lang-toolchain",
// "native-proto-lang-toolchain-info",
// "native-py",
// "native-sh-binary",
// "native-sh-library",
// "native-sh-test",
// "no-effect",
// "output-group",
// "overly-nested-depset",
Expand Down Expand Up @@ -298,6 +301,9 @@ func TestValidate(t *testing.T) {
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
"native-py",
"native-sh-binary",
"native-sh-library",
"native-sh-test",
"no-effect",
"output-group",
"overly-nested-depset",
Expand Down Expand Up @@ -377,6 +383,9 @@ func TestValidate(t *testing.T) {
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
"native-py",
"native-sh-binary",
"native-sh-library",
"native-sh-test",
"no-effect",
"output-group",
"overly-nested-depset",
Expand Down Expand Up @@ -456,6 +465,9 @@ func TestValidate(t *testing.T) {
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
"native-py",
"native-sh-binary",
"native-sh-library",
"native-sh-test",
"no-effect",
"output-group",
"overly-nested-depset",
Expand Down
3 changes: 3 additions & 0 deletions buildifier/integration_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ cat > golden/.buildifier.example.json <<EOF
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
"native-py",
"native-sh-binary",
"native-sh-library",
"native-sh-test",
"no-effect",
"output-group",
"overly-nested-depset",
Expand Down
3 changes: 3 additions & 0 deletions tables/tables.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ var CcLoadPath = "@rules_cc//cc:defs.bzl"
// JavaLoadPathPrefix is the load package for the Starlark Java Rules.
var JavaLoadPathPrefix = "@rules_java//java"

// ShellLoadPathPrefix is the load package for the Starlark Shell Rules.
var ShellLoadPathPrefix = "@rules_shell//shell"

// PyNativeRules lists all Python rules that are being migrated from Native to Starlark.
var PyNativeRules = []string{
"py_library",
Expand Down
21 changes: 21 additions & 0 deletions warn/docs/warnings.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,27 @@ warnings: {
autofix: true
}

warnings: {
name: "native-sh-binary"
header: "sh_binary build rules should be loaded from Starlark"
description: "The sh_binary build rules should be loaded from Starlark."
autofix: true
}

warnings: {
name: "native-sh-library"
header: "sh_library build rules should be loaded from Starlark"
description: "The sh_library build rules should be loaded from Starlark."
autofix: true
}

warnings: {
name: "native-sh-test"
header: "sh_test build rules should be loaded from Starlark"
description: "The sh_test build rules should be loaded from Starlark."
autofix: true
}

warnings: {
name: "no-effect"
header: "Expression result is not used"
Expand Down
3 changes: 3 additions & 0 deletions warn/warn.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,9 @@ var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader)
"native-proto-common": nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"),
"native-proto-lang-toolchain-info": nativeProtoSymbolsWarning("ProtoLangToolchainInfo", "proto_lang_toolchain_info.bzl"),
"native-py": nativePyRulesWarning,
"native-sh-binary": NativeShellRulesWarning("sh_binary"),
"native-sh-library": NativeShellRulesWarning("sh_library"),
"native-sh-test": NativeShellRulesWarning("sh_test"),
"unnamed-macro": unnamedMacroWarning,
}

Expand Down
9 changes: 9 additions & 0 deletions warn/warn_bazel_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,15 @@ func nativeProtoSymbolsWarning(symbol string, bzlfile string) func(f *build.File
}
}

func NativeShellRulesWarning(rule string) func(f *build.File, fileReader *FileReader) []*LinterFinding {
return func(f *build.File, fileReader *FileReader) []*LinterFinding {
if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
return nil
}
return NotLoadedFunctionUsageCheck(f, fileReader, []string{rule}, tables.ShellLoadPathPrefix+":"+rule+".bzl")
}
}

func contextArgsAPIWarning(f *build.File) []*LinterFinding {
if f.Type != build.TypeBzl {
return nil
Expand Down
75 changes: 75 additions & 0 deletions warn/warn_bazel_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,81 @@ def macro():
scopeBzl|scopeBuild)
}

func TestNativeShBinaryWarning(t *testing.T) {
checkFindingsAndFix(t, "native-sh-binary", `
"""My file"""
def macro():
native.sh_binary()
sh_binary()
`, `
"""My file"""
load("@rules_shell//shell:sh_binary.bzl", "sh_binary")
def macro():
sh_binary()
sh_binary()
`,
[]string{
fmt.Sprintf(`:4: Function "sh_binary" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_binary.bzl".`),
fmt.Sprintf(`:6: Function "sh_binary" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_binary.bzl".`),
},
scopeBzl|scopeBuild)
}

func TestNativeShLibraryWarning(t *testing.T) {
checkFindingsAndFix(t, "native-sh-library", `
"""My file"""
def macro():
native.sh_library()
sh_library()
`, `
"""My file"""
load("@rules_shell//shell:sh_library.bzl", "sh_library")
def macro():
sh_library()
sh_library()
`,
[]string{
fmt.Sprintf(`:4: Function "sh_library" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_library.bzl".`),
fmt.Sprintf(`:6: Function "sh_library" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_library.bzl".`),
},
scopeBzl|scopeBuild)
}

func TestNativeShTestWarning(t *testing.T) {
checkFindingsAndFix(t, "native-sh-test", `
"""My file"""
def macro():
native.sh_test()
sh_test()
`, `
"""My file"""
load("@rules_shell//shell:sh_test.bzl", "sh_test")
def macro():
sh_test()
sh_test()
`,
[]string{
fmt.Sprintf(`:4: Function "sh_test" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_test.bzl".`),
fmt.Sprintf(`:6: Function "sh_test" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_test.bzl".`),
},
scopeBzl|scopeBuild)
}

func TestKeywordParameters(t *testing.T) {
checkFindingsAndFix(t, "keyword-positional-params", `
foo(key = value)
Expand Down

0 comments on commit 3648bec

Please sign in to comment.