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

Jump to bottom

nogo: ignore generated source files #3216

Merged
merged 1 commit into from
Jul 6, 2022
Merged

nogo: ignore generated source files #3216

merged 1 commit into from
Jul 6, 2022

Conversation

sluongng
Copy link
Contributor

Under GoCompilePkg actions, there are several ways that source files
could have been modified / generated that is out of end users
control:

  • go_test would generate an '_empty.go' file for the missing
    internal/external package to compile it.

  • coverage instrumentation would rewrite test source files with a
    wrapper source file that collects coverage data into a determined
    variable.

  • CGO usage would transform the original source files into several
    output Go and C source files with go tool cgo ...

Implement a mechanism to ignore the generated source files for the first
2 cases and instruct static analyzers to run over the original source
files wherever applicable. This provides a better default for rules_go
users as static analysis errors over these generated files are outside
of their control and thus, could not be fixed.

Add a note regarding the CGO case for future investigations.

Replacement of #2996
Fixes #2984

@google-cla
Copy link

google-cla bot commented Jun 28, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sluongng sluongng force-pushed the sluongng/nogo-ignore-gen branch 2 times, most recently from 1012079 to 9e4f252 Compare June 28, 2022 08:39
sluongng
sluongng commented Jun 28, 2022
View reviewed changes
.bazelci/presubmit.yml
@@ -295,6 +295,7 @@ tasks:
- "-//tests/core/go_test:data_test"
- "-//tests/core/go_test:pwd_test"
- "-//tests/core/nogo/coverage:coverage_test"
- "-//tests/core/nogo/coverage:gen_code_test"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2518 for details

@sluongng
Copy link
Contributor Author

The reason why I skip on doing CGO is:

  • It's a bit more complex to dive into how cgo2() work
  • I have yet to encounter a CGO generated code conflicting with static analyzers myself.

Would love if somebody could address that in the future as I believe it would make the code a lot 'cleaner'.

fmeum
fmeum reviewed Jun 28, 2022
View reviewed changes
go/tools/builders/compilepkg.go Show resolved Hide resolved
go/tools/builders/compilepkg.go Outdated Show resolved Hide resolved
go/tools/builders/compilepkg.go Outdated Show resolved Hide resolved
tests/core/nogo/coverage/README.rst Outdated Show resolved Hide resolved
@fmeum
Copy link
Collaborator

fmeum commented Jun 28, 2022

Not dealing with CGo code until there is a clear need to do so seems fine to me.

@sluongng
Copy link
Contributor Author

@fmeum I applied your comment suggestion but Google's CLA does not seems to have the email that is used in the commit trailer:

The following contributors were found for this pull request:

✅ https://github.com/bazelbuild/rules_go/commit/de2e09007e5d3e354ae12d8b042ca273edd790db Author: @sluongng <sl****ng​@gmail.com>
❌ https://github.com/bazelbuild/rules_go/commit/de2e09007e5d3e354ae12d8b042ca273edd790db Co-Author: <fa***n​@meumertzhe.im>

If you don't want to sign the CLA w. that email, I can either do an amend to change to an email of your choice or to remove the Co-Author trailer entirely. Let me know your preference.

@fmeum
Copy link
Collaborator

fmeum commented Jun 28, 2022

@aiuto Sorry to bother you with this, but is there a way to sign the Google CLA for a single GitHub account with two Google accounts?

My last name changed and I now maintain two distinct Google accounts linked to my old and new name. I have signed the CLA with the old one and now can't add my new email to it because myaccount.google.com/alternateemail doesn't allow entering the address of another Google account. Do you see a way to unblock my commits under the new name?

@fmeum
Copy link
Collaborator

fmeum commented Jun 28, 2022

@sluongng Seems rather difficult to do, so please go ahead and squash away my author info.

@sluongng
nogo: ignore generated source files
1ce1832 
Under GoCompilePkg actions, there are several ways that source files
could have been modified / generated that is out of end users
control:

- go_test would generate an '_empty.go' file for the missing
  internal/external package to compile it.

- coverage instrumentation would rewrite test source files with a
  wrapper source file that collects coverage data into a determined
  variable.

- CGO usage would transform the original source files into several
  output Go and C source files with `go tool cgo ...`

Implement a mechanism to ignore the generated source files for the first
2 cases and instruct static analyzers to run over the original source
files wherever applicable.  This provides a better default for rules_go
users as static analysis errors over these generated files are outside
of their control and thus, could not be fixed.

Add a note regarding the CGO case for future investigations.
@fmeum fmeum merged commit df02d01 into bazelbuild:master Jul 6, 2022
gcf-merge-on-green bot pushed a commit to googleapis/gapic-generator-go that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#1076)
22aecf2 
[![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.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, 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, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-go).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
gcf-merge-on-green bot pushed a commit to googleapis/gapic-config-validator that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#255)
b92b5b7 
[![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.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, 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, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-config-validator).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
@emmaxy emmaxy mentioned this pull request Nov 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sluongng @fmeum