Revert changes migrating nogo to configuration transitions (bazel-con…

…trib#2481) This rolls back bazel-contrib#2473 and bazel-contrib#2474 due to bazel-contrib#2479, which is caused by bazelbuild/bazel#11291. There doesn't seem to be a way to work around that, and I don't want to hold the release back any longer. Reopens bazel-contrib#2374 Reopens bazel-contrib#2470
muratekici · May 8, 2020 · c0696e4 · c0696e4
1 parent 0a93be4
commit c0696e4
Show file tree

Hide file tree

Showing 24 changed files with 854 additions and 515 deletions.
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml
@@ -316,9 +316,7 @@ platforms:
     - "-//tests/core/go_proto_library:transitive_test"
     - "-//tests/core/go_test:data_test"
     - "-//tests/core/go_test:pwd_test"
-    - "-//tests/core/nogo/config:config_test"
     - "-//tests/core/nogo/coverage:coverage_test"
-    - "-//tests/core/nogo/flag:flag_test"
     - "-//tests/core/nogo/vet:vet_test"
     - "-//tests/core/stdlib:buildid_test"
     - "-//tests/examples/executable_name:executable_name"

diff --git a/BUILD.bazel b/BUILD.bazel
@@ -4,7 +4,6 @@ load(
 )
 load(
     "@io_bazel_rules_go//go/private:rules/nogo.bzl",
-    "default_nogo",
     "nogo",
 )
 load(
@@ -39,7 +38,7 @@ stdlib(
 # default_nogo is the nogo target that nogo references by default. It
 # does not analyze anything, which means no binary is built or run
 # at compile time.
-default_nogo(
+nogo(
     name = "default_nogo",
     visibility = ["//visibility:public"],
 )
@@ -54,18 +53,6 @@ nogo(
     deps = TOOLS_NOGO,
 )
 
-# nogo_alias points to the nogo flag which points to the active nogo binary
-# when nogo is enabled.
-# TODO(bazelbuild/bazel#11291): This exists to avoid a dependency cycle, but
-# the cycle shouldn't exist when transitions are taken into account.
-alias(
-    name = "nogo_alias",
-    actual = select({
-        "//go/private:need_nogo": "//go/config:nogo",
-        "//conditions:default": "//:default_nogo",
-    }),
-)
-
 # go_context_data collects build options and is depended on by all Go targets.
 # It may depend on cgo_context_data if CGo isn't disabled.
 go_context_data(
@@ -76,7 +63,7 @@ go_context_data(
     }),
     coverdata = "//go/tools/coverdata",
     go_config = ":go_config",
-    nogo = ":nogo_alias",
+    nogo = "@io_bazel_rules_nogo//:nogo",
     stdlib = ":stdlib",
     visibility = ["//visibility:public"],
 )

diff --git a/go/config/BUILD.bazel b/go/config/BUILD.bazel
@@ -58,12 +58,6 @@ string_list_flag(
     visibility = ["//visibility:public"],
 )
 
-label_flag(
-    name = "nogo",
-    build_setting_default = "@io_bazel_rules_nogo//:nogo",
-    visibility = ["//visibility:public"],
-)
-
 filegroup(
     name = "all_files",
     testonly = True,

diff --git a/go/core.rst b/go/core.rst
@@ -24,7 +24,6 @@ Core Go rules
 .. _test_arg: https://docs.bazel.build/versions/master/user-manual.html#flag--test_arg
 .. _test_filter: https://docs.bazel.build/versions/master/user-manual.html#flag--test_filter
 .. _write a CROSSTOOL file: https://github.com/bazelbuild/bazel/wiki/Yet-Another-CROSSTOOL-Writing-Tutorial
-.. _Deprecation schedule: https://github.com/bazelbuild/rules_go/wiki/Deprecation-schedule
 
 .. role:: param(kbd)
 .. role:: type(emphasis)
@@ -386,11 +385,9 @@ the same package.
 
 This rule is a limited variant of ``go_library`` which may be used to
 bootstrap tools used by rules_go. This avoids a circular dependency.
-
-**DEPRECATED:** This rule should no longer be used. ``go_library`` should be
-used instead. This was previously needed for ``nogo`` to avoid a circular
-dependency, but it's no longer necessary. See `Deprecation schedule`_ for more
-information.
+If you are building analyzers to be linked into a `nogo`_ binary, you'll
+need to use ``go_tool_library`` since ``go_library`` depends on `nogo`_
+implicitly.
 
 Providers
 ^^^^^^^^^

diff --git a/go/def.bzl b/go/def.bzl
@@ -80,41 +80,41 @@ load(
 # This is not backward compatible, so use caution when depending on this --
 # new analyses may discover issues in existing builds.
 TOOLS_NOGO = [
-    "@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/assign:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/atomic:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/atomicalign:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/bools:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/buildssa:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/buildtag:go_default_library",
+    "@org_golang_x_tools//go/analysis/passes/asmdecl:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/assign:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/atomic:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/atomicalign:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/bools:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/buildssa:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/buildtag:go_tool_library",
     # TODO(#2396): pass raw cgo sources to cgocall and re-enable.
-    # "@org_golang_x_tools//go/analysis/passes/cgocall:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/composite:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/copylock:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/ctrlflow:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/deepequalerrors:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/errorsas:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/findcall:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/httpresponse:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/loopclosure:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/lostcancel:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/nilfunc:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/nilness:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/pkgfact:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/printf:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/shadow:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/shift:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/sortslice:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/stdmethods:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/stringintconv:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/structtag:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/testinggoroutine:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/tests:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/unmarshal:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/unreachable:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_default_library",
-    "@org_golang_x_tools//go/analysis/passes/unusedresult:go_default_library",
+    # "@org_golang_x_tools//go/analysis/passes/cgocall:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/composite:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/copylock:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/ctrlflow:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/deepequalerrors:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/errorsas:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/findcall:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/httpresponse:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/loopclosure:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/lostcancel:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/nilfunc:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/nilness:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/pkgfact:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/printf:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/shadow:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/shift:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/sortslice:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/stdmethods:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/stringintconv:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/structtag:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/testinggoroutine:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/tests:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/unmarshal:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/unreachable:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_tool_library",
+    "@org_golang_x_tools//go/analysis/passes/unusedresult:go_tool_library",
 ]
 
 # Current version or next version to be tagged. Gazelle and other tools may

diff --git a/go/nogo.rst b/go/nogo.rst
@@ -3,6 +3,7 @@
 
 .. _nogo: nogo.rst#nogo
 .. _go_library: core.rst#go_library
+.. _go_tool_library: core.rst#go_tool_library
 .. _analysis: https://godoc.org/golang.org/x/tools/go/analysis
 .. _Analyzer: https://godoc.org/golang.org/x/tools/go/analysis#Analyzer
 .. _GoLibrary: providers.rst#GoLibrary
@@ -18,6 +19,10 @@
 .. footer:: The ``nogo`` logo was derived from the Go gopher, which was designed by Renee French. (http://reneefrench.blogspot.com/) The design is licensed under the Creative Commons 3.0 Attributions license. Read this article for more details: http://blog.golang.org/gopher
 
 
+**WARNING**: This functionality is experimental, so its API might change.
+Please do not rely on it for production use, but feel free to use it and file
+issues.
+
 ``nogo`` is a tool that analyzes the source code of Go programs. It runs
 alongside the Go compiler in the Bazel Go rules and rejects programs that
 contain disallowed coding patterns. In addition, ``nogo`` may report
@@ -49,32 +54,30 @@ want to run.
             # analyzer from the local repository
             ":importunsafe",
             # analyzer from a remote repository
-            "@org_golang_x_tools//go/analysis/passes/printf:go_default_library",
+            "@org_golang_x_tools//go/analysis/passes/printf:go_tool_library",
         ],
         visibility = ["//visibility:public"], # must have public visibility
     )
 
-    go_library(
+    go_tool_library(
         name = "importunsafe",
         srcs = ["importunsafe.go"],
         importpath = "importunsafe",
-        deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
+        deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
         visibility = ["//visibility:public"],
     )
 
-To build with this ``nogo`` target, use the
-``--@io_bazel_rules_go//go/config:nogo`` command line flag.
-
-.. code::
+Pass a label for your `nogo`_ target to ``go_register_toolchains`` in your
+``WORKSPACE`` file.
 
-    bazel build --@io_bazel_rules_go//go/config:nogo=//:my_nogo //:my_binary
-
-For convenience, you can add this to your workspace's ``.bazelrc`` file to
-avoid having to type this out for every command.
+.. code:: bzl
 
-.. code::
+    load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains")
+    go_rules_dependencies()
+    go_register_toolchains(nogo = "@//:my_nogo") # my_nogo is in the top-level BUILD file of this workspace
 
-    build --@io_bazel_rules_go//go/config:nogo=//:my_nogo
+**NOTE**: You must include ``"@//"`` prefix when referring to targets in the local
+workspace.
 
 The `nogo`_ rule will generate a program that executes all the supplied
 analyzers at build-time. The generated ``nogo`` program will run alongside the
@@ -83,20 +86,20 @@ even if the target is imported from an external repository. However, ``nogo``
 will not run when targets from the current repository are imported into other
 workspaces and built there.
 
-The target ``@io_bazel_rules_go//:tools_nogo`` contains the analyzers from
-``golang.org/x/tools``. This is the same set of analyzers that ``go vet`` uses.
-You can use this instead of declaring your own ``nogo`` target.
+To run all the ``golang.org/x/tools`` analyzers, use ``@io_bazel_rules_go//:tools_nogo``.
 
-.. code::
+.. code:: bzl
 
-    build --@io_bazel_rules_go//go/config:nogo=@io_bazel_rules_go//:tools_nogo
+    load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains")
+    go_rules_dependencies()
+    go_register_toolchains(nogo = "@io_bazel_rules_go//:tools_nogo")
 
 To run the analyzers from ``tools_nogo`` together with your own analyzers, use
 the ``TOOLS_NOGO`` list of dependencies.
 
 .. code:: bzl
 
-    load("@io_bazel_rules_go//go:def.bzl", "go_library", "nogo", "TOOLS_NOGO")
+    load("@io_bazel_rules_go//go:def.bzl", "nogo", "TOOLS_NOGO")
 
     nogo(
         name = "my_nogo",
@@ -107,11 +110,11 @@ the ``TOOLS_NOGO`` list of dependencies.
         visibility = ["//visibility:public"], # must have public visibility
     )
 
-    go_library(
+    go_tool_library(
         name = "importunsafe",
         srcs = ["importunsafe.go"],
         importpath = "importunsafe",
-        deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
+        deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
         visibility = ["//visibility:public"],
     )
 
@@ -156,35 +159,40 @@ already been run. For example:
 Any diagnostics reported by the analyzer will stop the build. Do not emit
 diagnostics unless they are severe enough to warrant stopping the build.
 
-Each analyzer must be written as a `go_library`_ rule and should import
-`@org_golang_x_tools//go/analysis:go_default_library`, the package anaysis
-framework.
+Each analyzer must be written as a `go_tool_library`_ rule and must import
+`@org_golang_x_tools//go/analysis:go_tool_library`, the `go_tool_library`_
+version of the package `analysis`_ target.
 
 For example:
 
 .. code:: bzl
 
-    load("@io_bazel_rules_go//go:def.bzl", "go_library")
+    load("@io_bazel_rules_go//go:def.bzl", "go_tool_library")
 
-    go_library(
+    go_tool_library(
         name = "importunsafe",
         srcs = ["importunsafe.go"],
         importpath = "importunsafe",
-        deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
+        deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
         visibility = ["//visibility:public"],
     )
 
-    go_library(
+    go_tool_library(
         name = "unsafedom",
         srcs = [
             "check_dom.go",
             "dom_utils.go",
         ],
         importpath = "unsafedom",
-        deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
+        deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
         visibility = ["//visibility:public"],
     )
 
+**NOTE**: `go_tool_library`_ is a limited variant of ``go_library`` which avoids
+a circular dependency: `go_library`_ implicitly depends on `nogo`_, which
+depends on analyzer libraries, which must not depend on `nogo`_.
+`go_tool_library`_ does not have the same implicit dependency.
+
 Pass labels for these targets to the ``deps`` attribute of your `nogo`_ target,
 as described in the `Setup`_ section.
 
@@ -295,7 +303,7 @@ See the full list of available nogo checks:
 
 .. code:: shell
 
-    bazel query 'kind(go_library, @org_golang_x_tools//go/analysis/passes/...)'
+    bazel query 'kind(go_tool_library, @org_golang_x_tools//go/analysis/passes/...)'
 
 
 API
@@ -324,6 +332,10 @@ Attributes
 |                                                                                                  |
 | These libraries must declare an ``analysis.Analyzer`` variable named `Analyzer` to ensure that   |
 | the analyzers they implement are called by nogo.                                                 |
+|                                                                                                  |
+| To avoid bootstrapping problems, these libraries must be `go_tool_library`_ targets, and must    |
+| import `@org_golang_x_tools//go/analysis:go_tool_library`, the `go_tool_library`_ version of     |
+| the package `analysis`_ target.                                                                  |
 +----------------------------+-----------------------------+---------------------------------------+
 | :param:`config`            | :type:`label`               | :value:`None`                         |
 +----------------------------+-----------------------------+---------------------------------------+

diff --git a/go/private/BUILD.bazel b/go/private/BUILD.bazel
@@ -1,5 +1,3 @@
-load("@bazel_skylib//rules:common_settings.bzl", "bool_setting")
-
 filegroup(
     name = "all_rules",
     srcs = glob(["**/*.bzl"]),
@@ -32,21 +30,3 @@ config_setting(
     name = "stamp",
     values = {"stamp": "true"},
 )
-
-# need_nogo_flag is set to false by nogo's incoming edge transition. It's used
-# to control whether //:nogo_alias depends on //go/config:nogo to break
-# a dependency cycle.
-# TODO(bazelbuild/bazel#11291): the cycle shouldn't exist if transitions
-# are taken into account, so this should be removed at some point.
-bool_setting(
-    name = "need_nogo_flag",
-    build_setting_default = True,
-)
-
-config_setting(
-    name = "need_nogo",
-    flag_values = {
-        ":need_nogo_flag": "True",
-    },
-    visibility = ["//:__pkg__"],
-)
diff --git a/go/private/context.bzl b/go/private/context.bzl
@@ -493,12 +493,7 @@ def go_context(ctx, attr = None):
 
 def _go_context_data_impl(ctx):
     coverdata = ctx.attr.coverdata[GoArchive]
-    nogo = None
-    if ctx.attr.nogo and DefaultInfo in ctx.attr.nogo:
-        # TODO: may need multiple files and runfiles.
-        nogo_files = ctx.attr.nogo[DefaultInfo].files.to_list()
-        if nogo_files:
-            nogo = nogo_files[0]
+    nogo = ctx.files.nogo[0] if ctx.files.nogo else None
     providers = [
         GoContextInfo(
             coverdata = ctx.attr.coverdata[GoArchive],
@@ -524,6 +519,7 @@ go_context_data = rule(
             providers = [GoConfigInfo],
         ),
         "nogo": attr.label(
+            mandatory = True,
             cfg = "exec",
         ),
         "stdlib": attr.label(