Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require proto rules to be loaded by buildifier #1310

Merged
merged 6 commits into from
Nov 29, 2024
Merged

Require proto rules to be loaded by buildifier #1310

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f0b36d
Require proto rules to be loaded by buildifier
comius Nov 27, 2024
3c8b65d
Format with gofmt
comius Nov 28, 2024
3721249
Add missing quote
comius Nov 28, 2024
135cfa1
Add and regenerated docs
comius Nov 28, 2024
918b8d1
Fix tests
comius Nov 28, 2024
d4a80df
Remove \n\n
comius Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion buildifier/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,16 @@ func ExampleExample() {
// "native-android",
// "native-build",
// "native-cc",
// "native-cc-proto",
// "native-java",
// "native-java-lite-proto",
// "native-java-proto",
// "native-package",
// "native-proto",
// "native-proto-common",
// "native-proto-info",
// "native-proto-lang-toolchain",
// "native-proto-lang-toolchain-info",
// "native-py",
// "no-effect",
// "output-group",
Expand Down Expand Up @@ -260,9 +267,16 @@ func TestValidate(t *testing.T) {
"native-android",
"native-build",
"native-cc",
"native-cc-proto",
"native-java",
native-java-lite-proto",
comius marked this conversation as resolved.
Show resolved Hide resolved
"native-java-proto",
"native-package",
"native-proto",
"native-proto-common",
"native-proto-info",
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
"native-py",
"no-effect",
"output-group",
Expand Down Expand Up @@ -322,9 +336,16 @@ func TestValidate(t *testing.T) {
// "native-android",
"native-build",
// "native-cc",
"native-cc-proto",
// "native-java",
"native-java-lite-proto",
"native-java-proto",
"native-package",
// "native-proto",
"native-proto",
"native-proto-common",
"native-proto-info",
"native-proto-lang-toolchain",
"native-proto-lang-toolchain-info",
// "native-py",
"no-effect",
"output-group",
Expand Down Expand Up @@ -383,7 +404,10 @@ func TestValidate(t *testing.T) {
"name-conventions",
// "native-android",
"native-build",
"native-cc-proto",
// "native-java",
"native-java-lite-proto",
"native-java-proto",
"native-package",
// "native-proto",
// "native-py",
Expand Down
19 changes: 2 additions & 17 deletions tables/tables.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ var CcNativeRules = []string{
"cc_test",
"cc_library",
"cc_import",
"cc_proto_library",
"fdo_prefetch_hints",
"fdo_profile",
"cc_toolchain",
Expand All @@ -251,8 +250,6 @@ var JavaNativeRules = []string{
"java_binary",
"java_import",
"java_library",
"java_lite_proto_library",
"java_proto_library",
"java_test",
"java_package_configuration",
"java_plugin",
Expand All @@ -274,20 +271,8 @@ var PyNativeRules = []string{
// PyLoadPath is the load path for the Starlark Python Rules.
var PyLoadPath = "@rules_python//python:defs.bzl"

// ProtoNativeRules lists all Proto rules that are being migrated from Native to Starlark.
var ProtoNativeRules = []string{
"proto_lang_toolchain",
"proto_library",
}

// ProtoNativeSymbols lists all Proto symbols that are being migrated from Native to Starlark.
var ProtoNativeSymbols = []string{
"ProtoInfo",
"proto_common",
}

// ProtoLoadPath is the load path for the Starlark Proto Rules.
var ProtoLoadPath = "@rules_proto//proto:defs.bzl"
// ProtoLoadPathPrefix is the load path prefix for the Starlark Proto Rules.
var ProtoLoadPathPrefix = "@com_google_protobuf//bazel"

// IsModuleOverride contains the names of all Bzlmod module overrides available in MODULE.bazel.
var IsModuleOverride = map[string]bool{
Expand Down
7 changes: 2 additions & 5 deletions warn/docs/warnings.textproto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,11 +518,8 @@ warnings: {
header: "All Proto build rules and symbols should be loaded from Starlark"
description:
"The Proto build rules should be loaded from Starlark.\n\n"
"Update: the plans for disabling native rules\n"
"[have been postponed](https://groups.google.com/g/bazel-discuss/c/XNvpWcge4AE/m/aJ-aQzszAwAJ),\n"
"at the moment it's not required to load Starlark rules."
bazel_flag: "--incompatible_load_proto_rules_from_bzl"
bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/8922"
bazel_flag: "--incompatible_autoload_externally"
bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043"
autofix: true
}

Expand Down
122 changes: 64 additions & 58 deletions warn/warn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,63 +118,70 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi

// FileWarningMap lists the warnings that run on the whole file.
var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
"attr-applicable_licenses": attrApplicableLicensesWarning,
"attr-cfg": attrConfigurationWarning,
"attr-license": attrLicenseWarning,
"attr-licenses": attrLicensesWarning,
"attr-non-empty": attrNonEmptyWarning,
"attr-output-default": attrOutputDefaultWarning,
"attr-single-file": attrSingleFileWarning,
"build-args-kwargs": argsKwargsInBuildFilesWarning,
"bzl-visibility": bzlVisibilityWarning,
"confusing-name": confusingNameWarning,
"constant-glob": constantGlobWarning,
"ctx-actions": ctxActionsWarning,
"ctx-args": contextArgsAPIWarning,
"depset-items": depsetItemsWarning,
"depset-iteration": depsetIterationWarning,
"depset-union": depsetUnionWarning,
"dict-method-named-arg": dictMethodNamedArgWarning,
"dict-concatenation": dictionaryConcatenationWarning,
"duplicated-name": duplicatedNameWarning,
"filetype": fileTypeWarning,
"function-docstring": functionDocstringWarning,
"function-docstring-header": functionDocstringHeaderWarning,
"function-docstring-args": functionDocstringArgsWarning,
"function-docstring-return": functionDocstringReturnWarning,
"git-repository": nativeGitRepositoryWarning,
"http-archive": nativeHTTPArchiveWarning,
"integer-division": integerDivisionWarning,
"keyword-positional-params": keywordPositionalParametersWarning,
"list-append": listAppendWarning,
"load": unusedLoadWarning,
"module-docstring": moduleDocstringWarning,
"name-conventions": nameConventionsWarning,
"native-android": nativeAndroidRulesWarning,
"native-build": nativeInBuildFilesWarning,
"native-cc": nativeCcRulesWarning,
"native-java": nativeJavaRulesWarning,
"native-package": nativePackageWarning,
"native-proto": nativeProtoRulesWarning,
"native-py": nativePyRulesWarning,
"no-effect": noEffectWarning,
"output-group": outputGroupWarning,
"overly-nested-depset": overlyNestedDepsetWarning,
"package-name": packageNameWarning,
"package-on-top": packageOnTopWarning,
"print": printWarning,
"provider-params": providerParamsWarning,
"redefined-variable": redefinedVariableWarning,
"repository-name": repositoryNameWarning,
"rule-impl-return": ruleImplReturnWarning,
"return-value": missingReturnValueWarning,
"skylark-comment": skylarkCommentWarning,
"skylark-docstring": skylarkDocstringWarning,
"string-iteration": stringIterationWarning,
"uninitialized": uninitializedVariableWarning,
"unreachable": unreachableStatementWarning,
"unsorted-dict-items": unsortedDictItemsWarning,
"unused-variable": unusedVariableWarning,
"attr-applicable_licenses": attrApplicableLicensesWarning,
"attr-cfg": attrConfigurationWarning,
"attr-license": attrLicenseWarning,
"attr-licenses": attrLicensesWarning,
"attr-non-empty": attrNonEmptyWarning,
"attr-output-default": attrOutputDefaultWarning,
"attr-single-file": attrSingleFileWarning,
"build-args-kwargs": argsKwargsInBuildFilesWarning,
"bzl-visibility": bzlVisibilityWarning,
"confusing-name": confusingNameWarning,
"constant-glob": constantGlobWarning,
"ctx-actions": ctxActionsWarning,
"ctx-args": contextArgsAPIWarning,
"depset-items": depsetItemsWarning,
"depset-iteration": depsetIterationWarning,
"depset-union": depsetUnionWarning,
"dict-method-named-arg": dictMethodNamedArgWarning,
"dict-concatenation": dictionaryConcatenationWarning,
"duplicated-name": duplicatedNameWarning,
"filetype": fileTypeWarning,
"function-docstring": functionDocstringWarning,
"function-docstring-header": functionDocstringHeaderWarning,
"function-docstring-args": functionDocstringArgsWarning,
"function-docstring-return": functionDocstringReturnWarning,
"git-repository": nativeGitRepositoryWarning,
"http-archive": nativeHTTPArchiveWarning,
"integer-division": integerDivisionWarning,
"keyword-positional-params": keywordPositionalParametersWarning,
"list-append": listAppendWarning,
"load": unusedLoadWarning,
"module-docstring": moduleDocstringWarning,
"name-conventions": nameConventionsWarning,
"native-android": nativeAndroidRulesWarning,
"native-build": nativeInBuildFilesWarning,
"native-cc": nativeCcRulesWarning,
"native-java": nativeJavaRulesWarning,
"native-package": nativePackageWarning,
"native-proto": NativeProtoRulesWarning("proto_library"),
"native-java-proto": NativeProtoRulesWarning("java_proto_library"),
"native-java-lite-proto": NativeProtoRulesWarning("java_lite_proto_library"),
"native-cc-proto": NativeProtoRulesWarning("cc_proto_library"),
"native-proto-lang-toolchain": nativeProtoLangToolchainWarning,
"native-proto-info": nativeProtoSymbolsWarning("ProtoInfo", "proto_info.bzl"),
"native-proto-common": nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"),
"native-proto-lang-toolchain-info": nativeProtoSymbolsWarning("ProtoLangToolchainInfo", "proto_lang_toolchain_info.bzl"),
comius marked this conversation as resolved.
Show resolved Hide resolved
"native-py": nativePyRulesWarning,
"no-effect": noEffectWarning,
"output-group": outputGroupWarning,
"overly-nested-depset": overlyNestedDepsetWarning,
"package-name": packageNameWarning,
"package-on-top": packageOnTopWarning,
"print": printWarning,
"provider-params": providerParamsWarning,
"redefined-variable": redefinedVariableWarning,
"repository-name": repositoryNameWarning,
"rule-impl-return": ruleImplReturnWarning,
"return-value": missingReturnValueWarning,
"skylark-comment": skylarkCommentWarning,
"skylark-docstring": skylarkDocstringWarning,
"string-iteration": stringIterationWarning,
"uninitialized": uninitializedVariableWarning,
"unreachable": unreachableStatementWarning,
"unsorted-dict-items": unsortedDictItemsWarning,
"unused-variable": unusedVariableWarning,
}

// MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
Expand All @@ -190,7 +197,6 @@ var nonDefaultWarnings = map[string]bool{
"native-android": true, // disables native android rules
"native-cc": true, // disables native cc rules
"native-java": true, // disables native java rules
"native-proto": true, // disables native proto rules
"native-py": true, // disables native python rules
}

Expand Down
29 changes: 27 additions & 2 deletions warn/warn_bazel_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,12 @@ func NotLoadedFunctionUsageCheck(f *build.File, globals []string, loadFrom strin
return notLoadedUsageCheck(f, globals, []string{}, loadFrom)
}

// NotLoadedSymbolUsageCheck checks whether there's a usage of a given not imported function in the file
// and adds a load statement if necessary.
func NotLoadedSymbolUsageCheck(f *build.File, globals []string, loadFrom string) []*LinterFinding {
return notLoadedUsageCheck(f, []string{}, globals, loadFrom)
}

// makePositional makes the function argument positional (removes the keyword if it exists)
func makePositional(argument build.Expr) build.Expr {
if binary, ok := argument.(*build.AssignExpr); ok {
Expand Down Expand Up @@ -709,11 +715,30 @@ func nativePyRulesWarning(f *build.File) []*LinterFinding {
return NotLoadedFunctionUsageCheck(f, tables.PyNativeRules, tables.PyLoadPath)
}

func nativeProtoRulesWarning(f *build.File) []*LinterFinding {
// NativeProtoRulesWarning produces a warning for missing loads of proto rules
func NativeProtoRulesWarning(rule string) func(f *build.File) []*LinterFinding {
return func(f *build.File) []*LinterFinding {
if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
return nil
}
return NotLoadedFunctionUsageCheck(f, []string{rule}, tables.ProtoLoadPathPrefix + ":" + rule + ".bzl")
}
}

func nativeProtoLangToolchainWarning(f *build.File) []*LinterFinding {
if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
return nil
}
return notLoadedUsageCheck(f, tables.ProtoNativeRules, tables.ProtoNativeSymbols, tables.ProtoLoadPath)
return NotLoadedFunctionUsageCheck(f, []string{"proto_lang_toolchain"}, tables.ProtoLoadPathPrefix + "/toolchains:proto_lang_toolchain.bzl")
}

func nativeProtoSymbolsWarning(symbol string, bzlfile string) func(f *build.File) []*LinterFinding {
return func(f *build.File) []*LinterFinding {
if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
return nil
}
return NotLoadedSymbolUsageCheck(f, []string{symbol}, tables.ProtoLoadPathPrefix + "/common:" + bzlfile)
}
}

func contextArgsAPIWarning(f *build.File) []*LinterFinding {
Expand Down
40 changes: 20 additions & 20 deletions warn/warn_bazel_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,6 @@ def macro():
cc_library()
native.cc_binary()
cc_test()
cc_proto_library()
native.fdo_prefetch_hints()
native.objc_library()
objc_import()
Expand All @@ -564,13 +563,12 @@ cc_import()
`, fmt.Sprintf(`
"""My file"""

load(%q, "cc_binary", "cc_import", "cc_library", "cc_proto_library", "cc_test", "cc_toolchain", "cc_toolchain_suite", "fdo_prefetch_hints", "fdo_profile", "objc_import", "objc_library")
load(%q, "cc_binary", "cc_import", "cc_library", "cc_test", "cc_toolchain", "cc_toolchain_suite", "fdo_prefetch_hints", "fdo_profile", "objc_import", "objc_library")

def macro():
cc_library()
cc_binary()
cc_test()
cc_proto_library()
fdo_prefetch_hints()
objc_library()
objc_import()
Expand All @@ -584,14 +582,13 @@ cc_import()
fmt.Sprintf(`:4: Function "cc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:5: Function "cc_binary" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:6: Function "cc_test" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:7: Function "cc_proto_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:8: Function "fdo_prefetch_hints" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:9: Function "objc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:10: Function "objc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:11: Function "cc_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:12: Function "cc_toolchain_suite" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:14: Function "fdo_profile" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:15: Function "cc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:7: Function "fdo_prefetch_hints" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:8: Function "objc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:9: Function "objc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:10: Function "cc_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:11: Function "cc_toolchain_suite" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:13: Function "fdo_profile" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
fmt.Sprintf(`:14: Function "cc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath),
},
scopeBzl|scopeBuild)
}
Expand Down Expand Up @@ -665,7 +662,7 @@ py_test()
}

func TestNativeProtoWarning(t *testing.T) {
checkFindingsAndFix(t, "native-proto", `
checkFindingsAndFix(t, "native-proto,native-proto-lang-toolchain,native-proto-info,native-proto-common", `
"""My file"""

def macro():
Expand All @@ -679,7 +676,10 @@ def macro():
`, fmt.Sprintf(`
"""My file"""

load(%q, "ProtoInfo", "proto_common", "proto_lang_toolchain", "proto_library")
load("%[1]s:proto_library.bzl", "proto_library")
load("%[1]s/common:proto_common.bzl", "proto_common")
load("%[1]s/common:proto_info.bzl", "ProtoInfo")
load("%[1]s/toolchains:proto_lang_toolchain.bzl", "proto_lang_toolchain")

def macro():
proto_library()
Expand All @@ -689,14 +689,14 @@ def macro():

ProtoInfo
proto_common
`, tables.ProtoLoadPath),
`, tables.ProtoLoadPathPrefix),
[]string{
fmt.Sprintf(`:4: Function "proto_library" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:5: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:6: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:7: Function "proto_library" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:9: Symbol "ProtoInfo" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:10: Symbol "proto_common" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
fmt.Sprintf(`:4: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, tables.ProtoLoadPathPrefix),
fmt.Sprintf(`:5: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, tables.ProtoLoadPathPrefix),
fmt.Sprintf(`:6: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, tables.ProtoLoadPathPrefix),
fmt.Sprintf(`:7: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, tables.ProtoLoadPathPrefix),
fmt.Sprintf(`:9: Symbol "ProtoInfo" is not global anymore and needs to be loaded from "%s/common:proto_info.bzl".`, tables.ProtoLoadPathPrefix),
fmt.Sprintf(`:10: Symbol "proto_common" is not global anymore and needs to be loaded from "%s/common:proto_common.bzl".`, tables.ProtoLoadPathPrefix),
},
scopeBzl|scopeBuild)
}
Expand Down
Loading