Skip to content

Commit 077f15f

Browse files
authored
Fail when expected files are not produced by protoc (#4287)
**What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** This PR fail the action if a plugin does not produce a valid Go file that it is supposed to produce. This disallows people to add gRPC plugin to a `go_proto_library` target that has no service definition, but avoids the cache poisoning due to bugs in proto plugins. Because Gazelle is able to switch between proto and grpc plugins depending on the existence of service definition, existing repos managed by Gazelle will not be affected. **Which issues(s) does this PR fix?** Fixes #3949 **Other notes for review** Credits to @r-hang for coming up with the patch
1 parent 10b45b2 commit 077f15f

File tree

1 file changed

+17
-12
lines changed

1 file changed

+17
-12
lines changed

go/tools/builders/protoc.go

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020
"errors"
2121
"flag"
2222
"fmt"
23+
"go/parser"
24+
"go/token"
2325
"io/ioutil"
2426
"log"
2527
"os"
@@ -36,7 +38,7 @@ type genFileInfo struct {
3638
created bool // Whether the file was created by protoc
3739
from *genFileInfo // The actual file protoc produced if not Path
3840
unique bool // True if this base name is unique in expected results
39-
ambiguious bool // True if there were more than one possible outputs that matched this file
41+
ambiguous bool // True if there were more than one possible outputs that matched this file
4042
}
4143

4244
func run(args []string) error {
@@ -170,8 +172,8 @@ func run(args []string) error {
170172
case !copyTo.unique:
171173
// not unique, no copy allowed
172174
case copyTo.from != nil:
173-
copyTo.ambiguious = true
174-
info.ambiguious = true
175+
copyTo.ambiguous = true
176+
info.ambiguous = true
175177
default:
176178
copyTo.from = info
177179
copyTo.created = true
@@ -183,15 +185,9 @@ func run(args []string) error {
183185
for _, f := range files {
184186
switch {
185187
case f.expected && !f.created:
186-
// Some plugins only create output files if the proto source files have
187-
// have relevant definitions (e.g., services for grpc_gateway). Create
188-
// trivial files that the compiler will ignore for missing outputs.
189-
data := []byte("// +build ignore\n\npackage ignore")
190-
if err := ioutil.WriteFile(abs(f.path), data, 0644); err != nil {
191-
return err
192-
}
193-
case f.expected && f.ambiguious:
194-
fmt.Fprintf(buf, "Ambiguious output %v.\n", f.path)
188+
return fmt.Errorf("file %q expected from plugin %q but not created", f.path, *plugin)
189+
case f.expected && f.ambiguous:
190+
fmt.Fprintf(buf, "Ambiguous output %v.\n", f.path)
195191
case f.from != nil:
196192
data, err := ioutil.ReadFile(f.from.path)
197193
if err != nil {
@@ -207,6 +203,15 @@ func run(args []string) error {
207203
fmt.Fprintf(buf, "Check that the go_package option is %q.", *importpath)
208204
return errors.New(buf.String())
209205
}
206+
207+
if filepath.Ext(f.path) != ".go" {
208+
continue
209+
}
210+
// Additionally check that created files are valid, because invalid Go file causes cache poisoning.
211+
_, err := parser.ParseFile(token.NewFileSet(), f.path, nil, parser.PackageClauseOnly)
212+
if err != nil {
213+
return fmt.Errorf("plugin %q created an invalid Go file %q: %v", *plugin, f.path, err)
214+
}
210215
}
211216

212217
return nil

0 commit comments

Comments
 (0)