Skip to content

Commit

Permalink
nogo: ignore generated source files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sluongng committed Jun 28, 2022
1 parent 9fdf306 commit 9e4f252
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 47 deletions.
1 change: 1 addition & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
- "-//tests/core/race:race_test" # fails on Windows due to upstream bug, see issue #2911
- "-//tests/core/stdlib:buildid_test"
- "-//tests/examples/executable_name:executable_name"
Expand Down
47 changes: 40 additions & 7 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,15 @@ func compileArchive(
}
defer cleanup()

// As part of compilation process, rules_go does generate and/or rewrite code
// based on the original source files. We should only run static analysis
// over original source files and not the generated source as end users have
// little control over the generated source.
//
// nogoSrcsOrigin maps generated/rewritten source files back to original source.
// If the original source path is an empty string, exclude generated source from nogo run.
nogoSrcsOrigin := make(map[string]string)

if len(srcs.goSrcs) == 0 {
// We need to run the compiler to create a valid archive, even if there's
// nothing in it. GoPack will complain if we try to add assembly or cgo
Expand All @@ -194,13 +203,13 @@ func compileArchive(
// otherwise platforms without sandbox support may race to create/remove
// the file during parallel compilation.
emptyDir := filepath.Join(filepath.Dir(outPath), sanitizePathForIdentifier(importPath))
if err := os.Mkdir(emptyDir, 0700); err != nil {
if err := os.Mkdir(emptyDir, 0o700); err != nil {
return fmt.Errorf("could not create directory for _empty.go: %v", err)
}
defer os.RemoveAll(emptyDir)

emptyPath := filepath.Join(emptyDir, "_empty.go")
if err := os.WriteFile(emptyPath, []byte("package empty\n"), 0666); err != nil {
if err := os.WriteFile(emptyPath, []byte("package empty\n"), 0o666); err != nil {
return err
}

Expand All @@ -210,6 +219,8 @@ func compileArchive(
matched: true,
pkg: "empty",
})

nogoSrcsOrigin[emptyPath] = ""
}
packageName := srcs.goSrcs[0].pkg
var goSrcs, cgoSrcs []string
Expand Down Expand Up @@ -290,9 +301,11 @@ func compileArchive(

if i < len(goSrcs) {
goSrcs[i] = coverSrc
} else {
cgoSrcs[i-len(goSrcs)] = coverSrc
nogoSrcsOrigin[coverSrc] = origSrc
continue
}

cgoSrcs[i-len(goSrcs)] = coverSrc
}
}

Expand All @@ -312,7 +325,7 @@ func compileArchive(
gcFlags = append(gcFlags, createTrimPath(gcFlags, srcDir))
} else {
if cgoExportHPath != "" {
if err := ioutil.WriteFile(cgoExportHPath, nil, 0666); err != nil {
if err := ioutil.WriteFile(cgoExportHPath, nil, 0o666); err != nil {
return err
}
}
Expand Down Expand Up @@ -390,11 +403,31 @@ func compileArchive(
// Run nogo concurrently.
var nogoChan chan error
outFactsPath := filepath.Join(workDir, nogoFact)
if nogoPath != "" {
nogoSrcs := make([]string, 0, len(goSrcs))
for _, goSrc := range goSrcs {
// If source is found in the origin map, that means it's likely to be generated source file
// so feed the original source file to static analyzers instead of the generated one.
//
// If origin being empty, that means the generated source file has no origin (genrated by rules_go)
// thus ignore that entry entirely.
if originSrc, ok := nogoSrcsOrigin[goSrc]; ok {
if originSrc != "" {
nogoSrcs = append(nogoSrcs, originSrc)
}
continue
}

// TODO(sluongng): most likely what remains here are CGO-generated source files as the result of calling cgo2()
// Need to determine whether we want to feed these CGO-generated files into static analyzers.
//
// Add unknown origin source files into the mix.
nogoSrcs = append(nogoSrcs, goSrc)
}
if nogoPath != "" && len(nogoSrcs) > 0 {
ctx, cancel := context.WithCancel(context.Background())
nogoChan = make(chan error)
go func() {
nogoChan <- runNogo(ctx, workDir, nogoPath, goSrcs, deps, packagePath, importcfgPath, outFactsPath)
nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath)
}()
defer func() {
if nogoChan != nil {
Expand Down
9 changes: 2 additions & 7 deletions tests/core/nogo/coverage/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,5 @@ Verifies `#2146`_.

gen_code_test
-------------
Checks how `nogo`_ would interact with source code that was generated as part of
rules_go's coverage implementation. Currently `nogo`_ would be run over these
generated source code, which is not desirable as end-user have very little control
over how these code were generated.

In a future version, we shall flip this behavior so that `nogo`_ would not run
over source files that users have no control over.
Checks how `nogo`_ shoudl not interact with source code that was generated as part of
rules_go's coverage implementation.
18 changes: 3 additions & 15 deletions tests/core/nogo/coverage/gen_code_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@
package gen_code_test

import (
"errors"
"os/exec"
"strings"
"testing"

"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
Expand Down Expand Up @@ -126,17 +123,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
}

func TestNogoCoverGenCode(t *testing.T) {
out, err := bazel_testing.BazelOutput("coverage", "//:simple_test")
if err == nil {
t.Fatal("test should fail")
}

var eErr *exec.ExitError
if errors.As(err, &eErr) && strings.Contains(string(eErr.Stderr), "was generated by rules_go (nocover)") {
// Expected failure
return
if out, err := bazel_testing.BazelOutput("coverage", "//:simple_test"); err != nil {
println(string(out))
t.Fatal(err)
}

println(string(out))
t.Fatal(err)
}
2 changes: 1 addition & 1 deletion tests/core/nogo/generate/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ Tests to ensure `nogo`_ interaction with generated code.

empty_test
-------------
Checks that `nogo`_ is running over the `_empty.go` file that was
Checks that `nogo`_ is not running over the `_empty.go` file that was
generated as part of GoCompilePkg.

20 changes: 3 additions & 17 deletions tests/core/nogo/generate/empty_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@
package empty_test

import (
"errors"
"os/exec"
"strings"
"testing"

"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
Expand Down Expand Up @@ -98,19 +95,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
}

func TestNogoGenEmptyCode(t *testing.T) {
out, err := bazel_testing.BazelOutput("build", "-k", "//:simple_test")
if err == nil {
t.Fatal("test should fail")
}

var eErr *exec.ExitError
if errors.As(err, &eErr) &&
strings.Contains(string(eErr.Stderr), "Detected generated source code from rules_go") &&
strings.Contains(string(eErr.Stderr), "(noempty)") {
// Expected failure
return
if out, err := bazel_testing.BazelOutput("build", "-k", "//:simple_test"); err != nil {
println(string(out))
t.Fatal(err)
}

println(string(out))
t.Fatal(err)
}

0 comments on commit 9e4f252

Please sign in to comment.