-
-
Notifications
You must be signed in to change notification settings - Fork 668
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.
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
nogo: use original source files instead of coverage-instrumented #3770
Conversation
CC @sluongng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hey that’s my TODO 😂
Great job on this PR!
Could you help provide us with a test that demonstrate the broken behavior before this change and it’s now fixed? There has been several discussions about reworking nogo
to enable it with bzlmod and validation action. A test could help us avoid repeating the same mistake.
On second thought, I am not quite sure if this is the right thing to do since we do have analyzers that could operate on cgo code. https://cs.opensource.google/go/x/tools/+/master:go/analysis/passes/cgocall/cgocall.go It would be nice if we have a test to demonstrate how this PR would affect such an analyzer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current mechanism with nogoSrcsOrigin map doesn't work when cgo is enabled, because source files paths change after cgo compilation, resulting in no match to the nogoSrcsOrigin map, hence nogo checks against modified source files, reporting unexpected errors.
Perhaps we could include the original cgoSrcs
path inside nogoSrcsOrigin
for a simpler fix.
If that's not desirable vs the current approach, then I would expect the nogoSrcsOrigin
mechanism cleaned up within this PR as well.
Would love to hear what you think about this.
As explained above, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. The PR looks much cleaner now.
Is it possible to add some tests demonstrating the new behavior?
This goes without saying, but might be worth highlighting in the changelog if we were to merge this in the future: This change will double the cgo/cc part in a |
Could you help provide some example/reference for such tests? I'd really appreciate it. I'm not very familiar with how tests are done in rules_go, as this is my first time working in this repo. |
https://github.com/bazelbuild/rules_go/blob/master/tests/core/nogo/coverage/README.rst should be a good starting point for this case since it tests for a combination of nogo+coverage. |
Unit test added! Sorry for the delay.
This shows that the nogo was running static analysis on coverage instrumented source files, resulting in error non-existing in the original source file. But I'm not sure why the build is failing for windows (OpenJDK 11, VS2022), any suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Having the test would let us prevent future regression.
Could you please add a few lines describing the test in tests/core/nogo/coverage/README.rst
?
Aside from that and a few nits, this PR LGTM.
680bfbb
to
b43bffc
Compare
Took a while before I realized the build failure was because a known issue #2518 that coverage not able to run on windows platform, so I updated the tests accordingly. Also changed it so that tests are skipped with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.0` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@​emmaxy](https://togithub.com/emmaxy) in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - Allow compilers to be overwritten in go_grpc_library() by [@​mering](https://togithub.com/mering) in [https://github.com/bazelbuild/rules_go/pull/3812](https://togithub.com/bazelbuild/rules_go/pull/3812) - update documentation for 0.44 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3813](https://togithub.com/bazelbuild/rules_go/pull/3813) - Temporarily undo deprecation of `//proto:go_grpc` by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3818](https://togithub.com/bazelbuild/rules_go/pull/3818) - Rename bazel_features polyfill to prevent conflicts by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3817](https://togithub.com/bazelbuild/rules_go/pull/3817) - Handle prerelease versions in polyfill by [@​illicitonion](https://togithub.com/illicitonion) in [https://github.com/bazelbuild/rules_go/pull/3821](https://togithub.com/bazelbuild/rules_go/pull/3821) - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@​therve](https://togithub.com/therve) in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3820](https://togithub.com/bazelbuild/rules_go/pull/3820) - improving logging for subcommand failure by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3824](https://togithub.com/bazelbuild/rules_go/pull/3824) - release rules go 0.45.0 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3825](https://togithub.com/bazelbuild/rules_go/pull/3825) #### New Contributors - [@​emmaxy](https://togithub.com/emmaxy) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - [@​therve](https://togithub.com/therve) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) **Full Changelog**: bazel-contrib/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.0` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@​emmaxy](https://togithub.com/emmaxy) in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - Allow compilers to be overwritten in go_grpc_library() by [@​mering](https://togithub.com/mering) in [https://github.com/bazelbuild/rules_go/pull/3812](https://togithub.com/bazelbuild/rules_go/pull/3812) - update documentation for 0.44 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3813](https://togithub.com/bazelbuild/rules_go/pull/3813) - Temporarily undo deprecation of `//proto:go_grpc` by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3818](https://togithub.com/bazelbuild/rules_go/pull/3818) - Rename bazel_features polyfill to prevent conflicts by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3817](https://togithub.com/bazelbuild/rules_go/pull/3817) - Handle prerelease versions in polyfill by [@​illicitonion](https://togithub.com/illicitonion) in [https://github.com/bazelbuild/rules_go/pull/3821](https://togithub.com/bazelbuild/rules_go/pull/3821) - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@​therve](https://togithub.com/therve) in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3820](https://togithub.com/bazelbuild/rules_go/pull/3820) - improving logging for subcommand failure by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3824](https://togithub.com/bazelbuild/rules_go/pull/3824) - release rules go 0.45.0 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3825](https://togithub.com/bazelbuild/rules_go/pull/3825) #### New Contributors - [@​emmaxy](https://togithub.com/emmaxy) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - [@​therve](https://togithub.com/therve) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) **Full Changelog**: bazel-contrib/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/kreempuff/rules_unreal_engine). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyNy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.44.2` -> `v0.45.1` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.45.1`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.1) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.45.0...v0.45.1) #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "6734a719993b1ba4ebe9806e853864395a8d3968ad27f9dd759c196b3eb3abe8", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.1/rules_go-v0.45.1.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.1/rules_go-v0.45.1.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - Disable `layering_check` feature in CGo tests by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3829](https://togithub.com/bazelbuild/rules_go/pull/3829) - Print stack traces without panicking on test timeout by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3833](https://togithub.com/bazelbuild/rules_go/pull/3833) - Fix regression in empty `.go` file filtering for nogo by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3832](https://togithub.com/bazelbuild/rules_go/pull/3832) - release patch version 0.45.1 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3836](https://togithub.com/bazelbuild/rules_go/pull/3836) **Full Changelog**: bazel-contrib/rules_go@v0.45.0...v0.45.1 ### [`v0.45.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.45.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.2...v0.45.0) #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "de7974538c31f76658e0d333086c69efdf6679dbc6a466ac29e65434bf47076d", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.45.0/rules_go-v0.45.0.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.6") #### What's Changed - nogo: use original source files instead of coverage-instrumented by [@​emmaxy](https://togithub.com/emmaxy) in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - Allow compilers to be overwritten in go_grpc_library() by [@​mering](https://togithub.com/mering) in [https://github.com/bazelbuild/rules_go/pull/3812](https://togithub.com/bazelbuild/rules_go/pull/3812) - update documentation for 0.44 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3813](https://togithub.com/bazelbuild/rules_go/pull/3813) - Temporarily undo deprecation of `//proto:go_grpc` by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3818](https://togithub.com/bazelbuild/rules_go/pull/3818) - Rename bazel_features polyfill to prevent conflicts by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3817](https://togithub.com/bazelbuild/rules_go/pull/3817) - Handle prerelease versions in polyfill by [@​illicitonion](https://togithub.com/illicitonion) in [https://github.com/bazelbuild/rules_go/pull/3821](https://togithub.com/bazelbuild/rules_go/pull/3821) - Extend gomock to allow passing an source_importpath instead of library when operating in source mode by [@​therve](https://togithub.com/therve) in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) - Fix coverage collection with modified `COVERAGE_DIR` env variable by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3820](https://togithub.com/bazelbuild/rules_go/pull/3820) - improving logging for subcommand failure by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3824](https://togithub.com/bazelbuild/rules_go/pull/3824) - release rules go 0.45.0 by [@​tyler-french](https://togithub.com/tyler-french) in [https://github.com/bazelbuild/rules_go/pull/3825](https://togithub.com/bazelbuild/rules_go/pull/3825) #### New Contributors - [@​emmaxy](https://togithub.com/emmaxy) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3770](https://togithub.com/bazelbuild/rules_go/pull/3770) - [@​therve](https://togithub.com/therve) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3822](https://togithub.com/bazelbuild/rules_go/pull/3822) **Full Changelog**: bazel-contrib/rules_go@v0.44.2...v0.45.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Nogo static analysis should not run against coverage instrumented source files, but the original ones.
The current mechanism with
nogoSrcsOrigin
map doesn't work when cgo is enabled, because source files paths change after cgo compilation, resulting in no match to thenogoSrcsOrigin
map, hence nogo checks against modified source files, reporting unexpected errors.This PR allows nogo to run static analysis against the original source files, instead of coverage-instrumented ones, by making a hard copy of the source file slice if coverage mode is set and before they are modified.
Which issues(s) does this PR fix?
Fixes #3769
Other notes for review