protoc-gen-go: add paths=source_relative option #533

neild · 2018-02-26T20:52:51Z

When --go_out=paths=source_relative:., output filenames are always
derived from the input filenames. For example, the output file
for a/b/c.proto will always be a/b/c.pb.go, regardless of the
presence or absence of a go_package option in the input file.

Fixes #515.

Change-Id: I7483449e68f65757bc459233b1644636b98885b9

When --go_out=paths=source_relative:., output filenames are always derived from the input filenames. For example, the output file for a/b/c.proto will always be a/b/c.pb.go, regardless of the presence or absence of a go_package option in the input file. Fixes golang#515. Change-Id: I7483449e68f65757bc459233b1644636b98885b9

Add a "// import <path>" comment to the package line when the source file includes a `go_package` option containing an import path. Change-Id: I84a397c91e84b10339e7131592b75bd1d6ded432

No particular reason for this to be a ./ import, and it confuses vgo.

As part of the recent protobuf optimization work, we started generating Unmarshal and Marshal methods on generated messages. These were done to support the table-driven implementation within the proto package. We are renaming Marshal as XXX_Marshal and Unmarshal as XXX_Unmarshal to make it clear that users are not supposed to call these directly. There are several reasons for the rename: * The Marshal method assumes that deterministic is the only option we would ever support for protos, and is not very forward compatible. * The presence of a deterministic boolean is confusing for users, where many set it without considering whether it is necessary. * The Unmarshal method has a slightly different semantic than the previously documented proto.Unmarshaler interface. The documented Unmarshal specifies that Reset is called, while the new method does no such thing. The semantic difference warrants a rename of the method. * Some users in the Go community depend on these methods not being generated by default. Fixes golang#530

Test importing in a variety of configurations: - Two files in the same Go and proto package. - Two files in the same Go package, but different proto packages. - Two files in different Go packages, but the same proto package. - Public imports. - Two files in the same package which each import a different Go package with the same name. (i.e., demonstrate whether rewrites of package names cross file boundaries.) - Imports which conflict with standard imports (e.g., "fmt").

New tests weren't updated for renamed Marshal/Unmarshal methods.

We're only testing with 1.6+, so claiming support for earlier versions is dubious.

Remove generate code dependencies on the order in which source files were provided to the compiler. In other words, "protoc a.proto b.proto" produces the same output as "protoc b.proto a.proto". - Include the proto package version assertion in all files. - Use the source file name and contents to generate unique var names for file descriptors, rather than the file index. In other words, "fileDescriptor_imp_81275c260ac30f8b" rather than "fileDescriptor0". Makes the generated code more stable in the face of unrelated changes (i.e., adding a new file to a package won't cause generated code for other files in irrelevant ways), as well as trivial changes in the protoc command line. Removes the requirement that all files in a package be compiled at the same time. Compiling each file individually will produce the same results as compiling all at once.

protoc-gen-go: add test for various generation params Add tests for import_prefix and Ma.proto=package.

dsnet · 2018-03-02T22:53:21Z

protoc-gen-go/golden_test.go

@@ -56,7 +56,7 @@ func TestGolden(t *testing.T) {
 			"protoc",
 			"--plugin=protoc-gen-go="+os.Args[0],
 			"-Itestdata",
-			"--go_out=plugins=grpc:"+workdir,
+			"--go_out=paths=source_relative,plugins=grpc:"+workdir,


I don't think this test really exercises the feature you're adding.

Added a couple test cases to the parameters test.

dsnet · 2018-03-02T22:55:41Z

README.md

+
+The protocol buffer language has a concept of "packages" which does not
+correspond well to the Go notion of packages. In generated Go code,
+each source `.proto` file is associated with a single Go package. The


"is associated with" => "can only be associated with"

The previous phrase seems to suggest one-to-one relationship.
The later allows for a many-to-one relationship.

I don't think "can only be associated with" is any less 1-1. Reworked this section a bit; PTAL.

dsnet · 2018-03-02T22:56:16Z

README.md

+	option go_package = "github.com/golang/protobuf/ptypes/any";
+
+The protocol buffer compiler will attempt to derive a package name and
+import path if when a `go_package` option is not present, but it is


"if when" => "only if"

dsnet · 2018-03-02T23:00:36Z

README.md

+import path if when a `go_package` option is not present, but it is
+best to always specify one explicitly.
+
+The filename of generated files may be produced in one of two ways.


Perhaps:

The output name of the generated file is always produced the same way where the .proto suffix is replaced with .pb.go (e.g., "foo.proto" is named "foo.pb.go").
However, the output directory is selected in one of two ways.

Phrased a bit differently; PTAL.

dsnet · 2018-03-02T23:01:23Z

README.md

+- Relative to the input file:
+
+	protoc --go_out=paths=source_relative:. inputs/x.proto
+	# generate ./inputs/x.proto


Don't you mean ./inputs/x.pb.go?

dsnet · 2018-03-02T23:08:02Z

README.md

 - `import_prefix=xxx` - a prefix that is added onto the beginning of
-  all imports. Useful for things like generating protos in a
-  subdirectory, or regenerating vendored protobufs in-place.
+  all imports. Supported for backwards compatibility, but rarely useful.


Perhaps use the word "deprecated" somewhere in here?

Moved to a "deprecated" section.

dsnet · 2018-03-02T23:12:23Z

protoc-gen-go/generator/generator.go

@@ -625,6 +637,15 @@ func (g *Generator) CommandLineParameters(parameter string) {
 			g.ImportPrefix = v
 		case "import_path":
 			g.PackageImportPath = v
+		case "paths":
+			switch v {
+			case "import":


Why is this undocumented?

When --go_out=paths=source_relative:., output filenames are always derived from the input filenames. For example, the output file for a/b/c.proto will always be a/b/c.pb.go, regardless of the presence or absence of a go_package option in the input file. Fixes golang#515. Change-Id: I7483449e68f65757bc459233b1644636b98885b9

googlebot · 2018-03-02T23:46:45Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-03-02T23:46:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dsnet · 2018-03-02T23:48:18Z

Yay... git pull does a merge instead of rebase.

neild · 2018-03-02T23:50:43Z

I have absolutely no idea what's gone wrong here. Is this fixable, or should I just start a new PR?

dsnet · 2018-03-03T00:12:32Z

You could do some "git rebase" magic and then do a "git push -f"...

I would just create a new PR.

neild · 2018-03-03T00:20:01Z

#544

neild requested a review from dsnet February 26, 2018 20:52

neild and others added 4 commits February 26, 2018 14:25

protoc-gen-go: include canonical import comment (golang#534)

1021ee9

Add a "// import <path>" comment to the package line when the source file includes a `go_package` option containing an import path. Change-Id: I84a397c91e84b10339e7131592b75bd1d6ded432

protoc-gen-go: remove relative import in main_test.go (golang#535)

2ef4baf

No particular reason for this to be a ./ import, and it confuses vgo.

neild changed the title ~~[dev] protoc-gen-go: add paths=source_relative option~~ protoc-gen-go: add paths=source_relative option Mar 1, 2018

neild added 4 commits March 1, 2018 10:42

protoc-gen-go: regenerate golden tests (golang#540)

3194ec9

New tests weren't updated for renamed Marshal/Unmarshal methods.

README: change minimum required Go version Go 1.6 (golang#541)

1c9f675

We're only testing with 1.6+, so claiming support for earlier versions is dubious.

protoc-gen-go: add test for various generation params (golang#539)

a198716

protoc-gen-go: add test for various generation params Add tests for import_prefix and Ma.proto=package.

dsnet reviewed Mar 2, 2018

View reviewed changes

neild added 3 commits March 2, 2018 15:27

Merge branch 'dev.output' of github.com:neild/protobuf into dev.output

56f520e

review updates

e8aa935

neild closed this Mar 3, 2018

golang locked and limited conversation to collaborators Jun 26, 2020

protoc-gen-go: add paths=source_relative option #533

protoc-gen-go: add paths=source_relative option #533

Uh oh!

Conversation

neild commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 2, 2018

Uh oh!

googlebot commented Mar 2, 2018

Uh oh!

dsnet commented Mar 2, 2018

Uh oh!

neild commented Mar 2, 2018

Uh oh!

dsnet commented Mar 3, 2018

Uh oh!

neild commented Mar 3, 2018

Uh oh!

Uh oh!