diff --git a/go/nogo.rst b/go/nogo.rst index f6d2baf0c0..73baf907b5 100644 --- a/go/nogo.rst +++ b/go/nogo.rst @@ -77,14 +77,20 @@ instead. go_register_toolchains(version = "1.23.1") go_register_nogo( nogo = "@//:my_nogo" # my_nogo is in the top-level BUILD file of this workspace - includes = ["@//:__subpackages__"], # Labels to lint. By default only lints code in workspace. - excludes = ["@//generated:__subpackages__"], # Labels to exclude. + excludes = ["@//generated:__subpackages__"], # Labels to exclude from nogo facts collection. This is unsafe. See notes below. ) **NOTE**: You must include ``"@//"`` prefix when referring to targets in the local workspace. Also note that you cannot use this to refer to bzlmod repos, as the labels don't go though repo mapping. +Although nogo only reports errors in those files in `only_files` is specified for analyzers, +it still collects facts from all targets in the transitive dependency closure, because it needs +those facts to accurately reports the errors. However, this can be slow, especially in large Go packages, +which could be generated. For performance reasons, users can exclude them with `excludes`. +This is unsafe, and should only be used to exclude packages that don't provide +useful nogo facts for the configured set of analyzers. + The `nogo`_ rule will generate a program that executes all the supplied analyzers at build-time. The generated ``nogo`` program will run alongside the compiler when building any Go target (e.g. `go_library`_) within your workspace, @@ -278,6 +284,9 @@ contain the following key-value pairs: | Keys in ``exclude_files`` override keys in ``only_files``. If a .go file matches a key present | | in both ``only_files`` and ``exclude_files``, the analyzer will not emit diagnostics for that | | file. | +| The key difference between ``excludes_files`` and ``excludes`` from ``go_register_nogo`` is that | +| the former is used to exclude files from reporting errors, while the latter is to prevent | +| nogo from collecting facts from certain packages. | +----------------------------+---------------------------------------------------------------------+ | ``"analyzer_flags"`` | :type:`dictionary, string to string` | +----------------------------+---------------------------------------------------------------------+ diff --git a/go/private/actions/archive.bzl b/go/private/actions/archive.bzl index ab56991c0a..f2b909f12e 100644 --- a/go/private/actions/archive.bzl +++ b/go/private/actions/archive.bzl @@ -14,7 +14,7 @@ load( "//go/private:context.bzl", - "validate_nogo", + "get_nogo", ) load( "//go/private:mode.bzl", @@ -59,16 +59,13 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d out_export = go.declare_file(go, name = source.name, ext = pre_ext + ".x") out_cgo_export_h = None # set if cgo used in c-shared or c-archive mode - nogo = go.nogo + nogo = get_nogo(go) # nogo is a FilesToRunProvider and some targets don't have it, some have it but no executable. if nogo != None and nogo.executable != None: out_facts = go.declare_file(go, name = source.name, ext = pre_ext + ".facts") out_diagnostics = go.declare_directory(go, name = source.name, ext = pre_ext + "_nogo") - if validate_nogo(go): - out_nogo_validation = go.declare_file(go, name = source.name, ext = pre_ext + ".nogo") - else: - out_nogo_validation = None + out_nogo_validation = go.declare_file(go, name = source.name, ext = pre_ext + ".nogo") else: out_facts = None out_diagnostics = None diff --git a/go/private/context.bzl b/go/private/context.bzl index f5cd735d38..6d00fb625e 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -38,7 +38,6 @@ load("@io_bazel_rules_go_bazel_features//:features.bzl", "bazel_features") load( "@io_bazel_rules_nogo//:scope.bzl", NOGO_EXCLUDES = "EXCLUDES", - NOGO_INCLUDES = "INCLUDES", ) load( "//go/platform:apple.bzl", @@ -429,11 +428,13 @@ def _matches_scopes(label, scopes): return True return False -def validate_nogo(go): - """Whether nogo should be run as a validation action rather than just to generate fact files for the current - target.""" +def get_nogo(go): + """Returns the nogo file for this target, if enabled and in scope.""" label = go.label - return _matches_scopes(label, NOGO_INCLUDES) and not _matches_scopes(label, NOGO_EXCLUDES) + if not _matches_scopes(label, NOGO_EXCLUDES): + return go.nogo + else: + return None default_go_config_info = GoConfigInfo( static = False, diff --git a/go/private/extensions.bzl b/go/private/extensions.bzl index a53dc6e37d..fc415ca054 100644 --- a/go/private/extensions.bzl +++ b/go/private/extensions.bzl @@ -83,17 +83,17 @@ _nogo_tag = tag_class( # dependency to a Go module can cause a build failure if the new dependency has lint # issues. doc = """ -A Go target is checked with nogo if its package matches at least one of the entries in 'includes' -and none of the entries in 'excludes'. By default, nogo is applied to all targets in the main -repository. - -Uses the same format as 'visibility', i.e., every entry must be a label that ends with ':__pkg__' or -':__subpackages__'. +Deprecated. Use the JSON config file to specify the include list. """, ), "excludes": attr.label_list( default = NOGO_DEFAULT_EXCLUDES, - doc = "See 'includes'.", + doc = """ +Nogo will generate facts from all Go targets, unless excluded here. + +Uses the same format as 'visibility', i.e., every entry must be a label that ends with ':__pkg__' or +':__subpackages__'. + """, ), }, ) @@ -159,7 +159,7 @@ def _go_sdk_impl(ctx): *[t for p in zip(module.tags.nogo, len(module.tags.nogo) * ["\n"]) for t in p] ) nogo_tag = module.tags.nogo[0] - for scope in nogo_tag.includes + nogo_tag.excludes: + for scope in nogo_tag.excludes: # Validate that the scope references a valid, visible repository. # buildifier: disable=no-effect scope.repo_name @@ -168,6 +168,7 @@ def _go_sdk_impl(ctx): "go_sdk.nogo: all entries in includes and excludes must end with ':__pkg__' or ':__subpackages__', got '{}' in".format(scope.name), nogo_tag, ) + go_register_nogo( name = "io_bazel_rules_nogo", nogo = str(nogo_tag.nogo), diff --git a/go/private/nogo.bzl b/go/private/nogo.bzl index 3baf1174a5..56b6f490a7 100644 --- a/go/private/nogo.bzl +++ b/go/private/nogo.bzl @@ -13,7 +13,7 @@ # limitations under the License. DEFAULT_NOGO = "@io_bazel_rules_go//:default_nogo" -NOGO_DEFAULT_INCLUDES = ["@@//:__subpackages__"] +NOGO_DEFAULT_INCLUDES = ["all"] NOGO_DEFAULT_EXCLUDES = [] # repr(Label(...)) does not emit a canonical label literal. @@ -34,13 +34,14 @@ def _go_register_nogo_impl(ctx): }, executable = False, ) + if ctx.attr.includes != NOGO_DEFAULT_INCLUDES: + print("go_register_nogo's include attribute is no-op. Nogo now collect facts from all targets by default. To include files in nogo validation, please use only_files in the JSON: https://github.com/bazel-contrib/rules_go/blob/master/go/nogo.rst#example") + ctx.file( "scope.bzl", """ -INCLUDES = {includes} EXCLUDES = {excludes} """.format( - includes = _scope_list_repr(ctx.attr.includes), excludes = _scope_list_repr(ctx.attr.excludes), ), executable = False, @@ -55,8 +56,6 @@ go_register_nogo = repository_rule( _go_register_nogo_impl, attrs = { "nogo": attr.string(mandatory = True), - # Special sentinel value used to let nogo run on all targets when using - # WORKSPACE, for backwards compatibility. "includes": attr.string_list(default = ["all"]), "excludes": attr.string_list(), }, diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index 1ea336d21d..b5602654b6 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -512,6 +512,8 @@ func (g *goPackage) String() string { return g.types.Path() } +var _externalFilePattern = regexp.MustCompile("^external/") + // checkAnalysisResults checks the analysis diagnostics in the given actions // and returns a string containing all the diagnostics that should be printed // to the build log. @@ -542,6 +544,10 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) ([]diagnosticEntry, if baseConfig, ok := configs[nogoBaseConfigName]; ok { currentConfig = baseConfig } + if currentConfig.excludeFiles == nil { + // TODO(linzhp): add a test to verify the case of []. + currentConfig.excludeFiles = []*regexp.Regexp{_externalFilePattern} + } // Overwrite the config with the desired config. Any unset fields // in the config will default to the base config. if actionConfig, ok := configs[act.a.Name]; ok { diff --git a/tests/core/nogo/bzlmod/includes_excludes_test.go b/tests/core/nogo/bzlmod/includes_excludes_test.go index 0c04c56e80..ebfba97737 100644 --- a/tests/core/nogo/bzlmod/includes_excludes_test.go +++ b/tests/core/nogo/bzlmod/includes_excludes_test.go @@ -30,6 +30,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "nogo", "TOOLS_NOGO") nogo( name = "my_nogo", + config = ":nogo.json", visibility = ["//visibility:public"], deps = TOOLS_NOGO, ) @@ -93,13 +94,23 @@ func useless() string { } return foo } + +-- nogo.json -- +{ + "_base": { + "only_files": { + "^go/": "go files" + }, + "exclude_files": { + "^go/third_party/": "vendored" + } + } +} `, ModuleFileSuffix: ` go_sdk = use_extension("@io_bazel_rules_go//go:extensions.bzl", "go_sdk") go_sdk.nogo( nogo = "//:my_nogo", - includes = ["//go:__subpackages__"], - excludes = ["//go/third_party:__subpackages__"], ) `, }) diff --git a/tests/core/nogo/includes_excludes/includes_excludes_test.go b/tests/core/nogo/includes_excludes/includes_excludes_test.go index 63a116a667..95fd173c50 100644 --- a/tests/core/nogo/includes_excludes/includes_excludes_test.go +++ b/tests/core/nogo/includes_excludes/includes_excludes_test.go @@ -23,15 +23,14 @@ import ( func TestMain(m *testing.M) { bazel_testing.TestMain(m, bazel_testing.Args{ - Nogo: "@//:my_nogo", - NogoIncludes: []string{"@//go:__subpackages__"}, - NogoExcludes: []string{"@//go/third_party:__subpackages__"}, + Nogo: "@io_bazel_rules_go//:tools_nogo", Main: ` -- BUILD.bazel -- load("@io_bazel_rules_go//go:def.bzl", "go_library", "nogo", "TOOLS_NOGO") nogo( name = "my_nogo", + config = ":nogo.json", visibility = ["//visibility:public"], deps = TOOLS_NOGO, ) @@ -95,6 +94,24 @@ func useless() string { } return foo } + +-- nogo.json -- +{ + "_base": { + "only_files": { + "^go/": "go files" + }, + "exclude_files": { + "^go/third_party/": "vendored" + } + } +} +`, + ModuleFileSuffix: ` +go_sdk = use_extension("@io_bazel_rules_go//go:extensions.bzl", "go_sdk") +go_sdk.nogo( + nogo = "//:my_nogo", +) `, }) }