compiler/protogen: require that the import path be specified
Since the release of v1.20, we have warned that it be required
that the import path for every generated package be specified.
This CL simplifies the mechanism for specifying such information
to just the "M" command-line flags and the "go_package" options
in the .proto source files, where the former takes precedence
over the latter.
Changes made:
* Remove "import_prefix" and "import_path" flags.
* Make the Go import path and package name derivation logic simpler
where both "M" flags and "go_package" options are parsed the same way
by calling the common splitImportPathAndPackageName function, and
where "M" flags take precedence over "go_package" options.
The exception to this occurs when neither "M" nor "go_package" specify
an explicit Go package name, where the derivation may be non-intuitive.
See the "NOTE" comment for the rationale for this behavior,
which actually matches what was already the case.
* Remove the pathTypeLegacy output mode and make pathTypeImport
the default. The pathTypeLegacy mode becomes even more non-sensible
now that we require that the import path always be provided.
* Remove the baseName function. After deleting unsupported functionality,
this seems to only be used by GeneratedFile.QualifiedGoIdent.
However, that method does not need to create stable package names
since they are only used locally within the generated file.
They only need to guarantee the property of validity and uniqueness.
* Remove the warn function since there are no more warnings.
Change-Id: Ic95fb3cde5ffcb71bbcc829fcff34369758cebef
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/301953
Trust: Joe Tsai <joetsai@digital-static.net>
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/compiler/protogen/protogen_test.go b/compiler/protogen/protogen_test.go
index 36dbf72..613d6b9 100644
--- a/compiler/protogen/protogen_test.go
+++ b/compiler/protogen/protogen_test.go
@@ -18,10 +18,6 @@
"google.golang.org/protobuf/types/pluginpb"
)
-func init() {
- warnings = false // avoid spam in tests
-}
-
func TestPluginParameters(t *testing.T) {
var flags flag.FlagSet
value := flags.Int("integer", 0, "")
@@ -58,32 +54,35 @@
}
func TestNoGoPackage(t *testing.T) {
- gen, err := Options{}.New(&pluginpb.CodeGeneratorRequest{
+ _, err := Options{}.New(&pluginpb.CodeGeneratorRequest{
ProtoFile: []*descriptorpb.FileDescriptorProto{
{
Name: proto.String("testdata/go_package/no_go_package.proto"),
Syntax: proto.String(protoreflect.Proto3.String()),
Package: proto.String("goproto.testdata"),
},
+ },
+ })
+ if err == nil {
+ t.Fatalf("missing go_package option: New(req) = nil, want error")
+ }
+}
+
+func TestInvalidImportPath(t *testing.T) {
+ _, err := Options{}.New(&pluginpb.CodeGeneratorRequest{
+ ProtoFile: []*descriptorpb.FileDescriptorProto{
{
- Name: proto.String("testdata/go_package/no_go_package_import.proto"),
- Syntax: proto.String(protoreflect.Proto3.String()),
- Package: proto.String("goproto.testdata"),
- Dependency: []string{"testdata/go_package/no_go_package.proto"},
+ Name: proto.String("testdata/go_package/no_go_package.proto"),
+ Syntax: proto.String(protoreflect.Proto3.String()),
+ Package: proto.String("goproto.testdata"),
+ Options: &descriptorpb.FileOptions{
+ GoPackage: proto.String("foo"),
+ },
},
},
})
- if err != nil {
- t.Fatal(err)
- }
-
- for i, f := range gen.Files {
- if got, want := string(f.GoPackageName), "goproto_testdata"; got != want {
- t.Errorf("gen.Files[%d].GoPackageName = %v, want %v", i, got, want)
- }
- if got, want := string(f.GoImportPath), "testdata/go_package"; got != want {
- t.Errorf("gen.Files[%d].GoImportPath = %v, want %v", i, got, want)
- }
+ if err == nil {
+ t.Fatalf("missing go_package option: New(req) = nil, want error")
}
}
@@ -102,13 +101,6 @@
wantFilename string
}{
{
- desc: "no parameters, no go_package option",
- generate: true,
- wantPackageName: "proto_package",
- wantImportPath: "dir",
- wantFilename: "dir/filename",
- },
- {
desc: "go_package option sets import path",
goPackageOption: "golang.org/x/foo",
generate: true,
@@ -125,21 +117,13 @@
wantFilename: "golang.org/x/foo/filename",
},
{
- desc: "go_package option sets package",
- goPackageOption: "foo",
- generate: true,
- wantPackageName: "foo",
- wantImportPath: "dir",
- wantFilename: "dir/filename",
- },
- {
desc: "command line sets import path for a file",
parameter: "Mdir/filename.proto=golang.org/x/bar",
goPackageOption: "golang.org/x/foo",
generate: true,
wantPackageName: "foo",
wantImportPath: "golang.org/x/bar",
- wantFilename: "golang.org/x/foo/filename",
+ wantFilename: "golang.org/x/bar/filename",
},
{
desc: "command line sets import path for a file with package name specified",
@@ -148,25 +132,7 @@
generate: true,
wantPackageName: "bar",
wantImportPath: "golang.org/x/bar",
- wantFilename: "golang.org/x/foo/filename",
- },
- {
- desc: "import_path parameter sets import path of generated files",
- parameter: "import_path=golang.org/x/bar",
- goPackageOption: "golang.org/x/foo",
- generate: true,
- wantPackageName: "foo",
- wantImportPath: "golang.org/x/bar",
- wantFilename: "golang.org/x/foo/filename",
- },
- {
- desc: "import_path parameter does not set import path of dependencies",
- parameter: "import_path=golang.org/x/bar",
- goPackageOption: "golang.org/x/foo",
- generate: false,
- wantPackageName: "foo",
- wantImportPath: "golang.org/x/foo",
- wantFilename: "golang.org/x/foo/filename",
+ wantFilename: "golang.org/x/bar/filename",
},
{
desc: "module option set",
@@ -190,7 +156,7 @@
desc: "module option implies paths=import",
parameter: "module=golang.org/x,Mdir/filename.proto=golang.org/x/foo",
generate: false,
- wantPackageName: "proto_package",
+ wantPackageName: "foo",
wantImportPath: "golang.org/x/foo",
wantFilename: "foo/filename",
},
@@ -245,6 +211,7 @@
func TestPackageNameInference(t *testing.T) {
gen, err := Options{}.New(&pluginpb.CodeGeneratorRequest{
+ Parameter: proto.String("Mdir/file1.proto=path/to/file1"),
ProtoFile: []*descriptorpb.FileDescriptorProto{
{
Name: proto.String("dir/file1.proto"),
@@ -254,7 +221,7 @@
Name: proto.String("dir/file2.proto"),
Package: proto.String("proto.package"),
Options: &descriptorpb.FileOptions{
- GoPackage: proto.String("foo"),
+ GoPackage: proto.String("path/to/file2"),
},
},
},
@@ -265,7 +232,7 @@
}
if f1, ok := gen.FilesByPath["dir/file1.proto"]; !ok {
t.Errorf("missing file info for dir/file1.proto")
- } else if f1.GoPackageName != "foo" {
+ } else if f1.GoPackageName != "file1" {
t.Errorf("dir/file1.proto: GoPackageName=%v, want foo; package name should be derived from dir/file2.proto", f1.GoPackageName)
}
}