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.

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: provide a way to ignore external cgo source #3619

Closed
sluongng opened this issue Jul 7, 2023 · 3 comments · Fixed by #3995
Closed

Nogo: provide a way to ignore external cgo source #3619

sluongng opened this issue Jul 7, 2023 · 3 comments · Fixed by #3995

Comments

@sluongng
Copy link
Contributor

sluongng commented Jul 7, 2023

What version of rules_go are you using?

v0.40.1

What version of gazelle are you using?

Irrelevant

What version of Bazel are you using?

6.2.1

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

Darwin / ARM64

Any other potentially useful information about your toolchain?

N/A

What did you do?

We have a dependency over github.com/shirou/gopsutil/v3/process package.

When enable copylocks analyzer by including "@org_golang_x_tools//go/analysis/passes/copylock:go_default_library" in nogo config, the code fail to compile.

We have already set nogo_config.json with

    "copylocks": {
        "exclude_files": {
            "external[\\\\,\\/]": "third_party"
        }
    },

However, the cgo source path does not include external and therefore, escaped the current regex.

We also want to avoid specific regex like this

    "copylocks": {
        "exclude_files": {
            "cgo[\\\\,\\/]github.com[\\\\,\\/]shirou[\\\\,\\/]gopsutil[\\\\,\\/]": "third_party cgo",
            "external[\\\\,\\/]": "third_party"
        }
    },

Even though it works, it might be harder to maintain.

What did you expect to see?

If possible, external cgo sources should be included into nogo with external inside the source path.
This would allow us to ignore those files using existing regex.

What did you see instead?

ERROR: /private/var/tmp/_bazel_sluongng/06e573a93bc2d6a9cad4ad41f00b4310/external/com_github_shirou_gopsutil_v3/process/BUILD.bazel:3:11: GoCompilePkg external/com_github_shirou_gopsutil_v3/process/process.a failed: (Exit 1): builder failed: error executing command (from target @com_github_shirou_gopsutil_v3//process:process) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/go_sdk_darwin_arm64/builder_reset/builder compilepkg -sdk external/go_sdk_darwin_arm64 -installsuffix darwin_arm64 -src ... (remaining 95 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
compilepkg: nogo: errors found by nogo during build-time code analysis:
/var/folders/ly/gjsqx9wd0cv1ck7xdslrt6t00000gn/T/rules_go_work-1366086039/cgo/github.com/shirou/gopsutil/v3/process/process.go:144:9: String passes lock by value: github.com/shirou/gopsutil/v3/process.Process contains sync.RWMutex (copylocks)
/var/folders/ly/gjsqx9wd0cv1ck7xdslrt6t00000gn/T/rules_go_work-1366086039/cgo/github.com/shirou/gopsutil/v3/process/process.go:145:23: call of json.Marshal copies lock value: github.com/shirou/gopsutil/v3/process.Process contains sync.RWMutex (copylocks)

Note how rules_go_work-1366086039/cgo/github.com/shirou/gopsutil/v3/process/process.go does not contain external inside.

@sluongng
Copy link
Contributor Author

sluongng commented Jan 8, 2024

@emmaxy do you know if the change in #3770 will exclude out cgo sources like rules_go_work-1366086039/cgo/github.com/shirou/gopsutil/v3/process/process.go from being fed into nogo?

If yes, we should be able to close this issue.

@emmaxy
Copy link
Contributor

emmaxy commented Jan 8, 2024

@emmaxy do you know if the change in #3770 will exclude out cgo sources like rules_go_work-1366086039/cgo/github.com/shirou/gopsutil/v3/process/process.go from being fed into nogo?

If yes, we should be able to close this issue.

#3770 does not exclude any cgo sources from being fed into nogo, I don't think it would fix this issue.

@fmeum
Copy link
Member

fmeum commented Aug 2, 2024

With #3995, the output becomes:

nogo: errors found by nogo during build-time code analysis:
external/gazelle~~go_deps~com_github_shirou_gopsutil_v3/process/process.go:144:9: String passes lock by value: github.com/shirou/gopsutil/v3/process.Process contains sync.RWMutex (copylocks)
external/gazelle~~go_deps~com_github_shirou_gopsutil_v3/process/process.go:145:23: call of json.Marshal copies lock value: github.com/shirou/gopsutil/v3/process.Process contains sync.RWMutex (copylocks)

Based on that, I would assume that the PR fixes this issue. @emmaxy Could you confirm that?

fmeum added a commit that referenced this issue Aug 6, 2024
**What type of PR is this?**

Feature

**What does this PR do? Why is it needed?**

`nogo` is run in a separate action that can be toggled with
`--run_validations`. This avoids rerunning compilation if the `nogo`
tool changes and also allows to collect `nogo` findings of targets that
depend on other targets that themselves have `nogo` findings (with
`--keep_going`).

**Which issues(s) does this PR fix?**

Fixes #3619
Fixes #3695 
Fixes #3846 
Closes #3707

**Other notes for review**

---------

Co-authored-by: Zhongpeng Lin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants