Fix to avoid printing import aliases when not needed #341

meling · 2017-10-05T01:49:41Z

It required a minor change in the grpc package, but otherwise the tests passed (surprisingly). This PR solves my own use case in issue #338.

PS: I realize this may potentially be an invasive change, and so should be tested widely on use cases outside the main repo.

It required a minor change in the grpc package, but otherwise the tests passed (surprisingly).

awalterschulze · 2017-10-05T08:46:58Z

protoc-gen-gogo/generator/generator.go


 func (g *Generator) PrintImport(alias, pkg string) {
 	statement := "import " + alias + " " + strconv.Quote(pkg)
+	if alias == pkg[strings.LastIndex(pkg, "/")+1:] {


I suspect this won't always work, since the packageName can differ from the packagePath.

awalterschulze · 2017-10-05T08:47:43Z

protoc-gen-gogo/generator/generator.go

-	// Convert dots to underscores before finding a unique alias.
-	pkg = strings.Map(badToUnderscore, pkg)
+	// Only make pkg names unique if they are non-standard lib
+	if f != nil {


Nice trick here :)

The comment looks incorrect though, judging by the method comment which says:

If f is nil, it's a builtin package like "proto" and has no file descriptor.

So it's not that the package is in the stdlib, it's that it is "a builtin package", whatever that means.

This comment is still wrong (this code only makes the names unique if they are not builtin).

However, this makes me think that this comment is just not useful. Perhaps instead of explaining what this code is doing (which is pretty clear just from reading the code and the method comment), explain why it is doing it?

How about this:

// Ensure that package names across multiple imports are unique, unless they // are builtin or standard lib packages, in which case they should not conflict.

awalterschulze · 2017-10-05T08:50:00Z

I also wonder about a case where someone is importing github.com/gogo/protobuf/proto and github.com/golang/protobuf/proto
Which is what happens when someone uses the goproto_registration plugin.

awalterschulze · 2017-10-05T08:50:45Z

When we have thought about these cases, then hopefully we are at a place where we can ask a gogoprotobuf user to test this branch their system.

meling · 2017-10-05T22:43:21Z

I ran the tests in github.com/gogo/protobuf/test/registration locally, and the following import statements are produced:

import proto "github.com/gogo/protobuf/proto"
import golang_proto "github.com/golang/protobuf/proto"

Of course, there could be other issues that I've overlooked. Let me know if there is any specific tests beyond this I should run.

I've also checked a few other places and it seems that some duplicate imports of the same package under different aliases are eliminated, e.g. in vanity/test/faster/vanity.pb.go.

About the Travis CI build failing, I assume it is caused by some diff check of the current output against previous output. I was unable to find a specific test failure or error message in the log. (Perhaps if I committed also the regenerated .pb.go files etc. this problem would go away?)

(I'm sorry if my unfamiliarity with the dev process is causing this. Please advise on how to do things properly, if I'm doing it wrong; maybe there is a contributor guide that I should read?)

awalterschulze · 2017-10-06T11:05:46Z

Those proto imports are beautiful :)

On Travis you should be able to click on details and see the printout.
But it is as you suspect, it is simply a diff that is failing, that wants the regenerated code to be commited. Simply calling make in the project root should fix this.

Your doing perfectly great :)
But adding a contributing guidelines doc is maybe not a bad idea. I just haven't found one in the wild which I thought was a good read yet, but I am open to suggestions and would like to know where you are struggling.
From my end though, you are doing great.

I think this is ready for a first outside tester.

After that I think it would be great to add a little test to gogoprotobuf, where multiple imports end in the same suffix, just like proto, but without requiring more dependencies.

awalterschulze · 2017-10-06T11:07:28Z

@tamird , can I ask you a favor. Can you test this branch on your codebase?
The tests that are failing is simply generated code that is not committed yet.

tamird

Tested against CockroachDB, looks good (with the changes I suggested). Here's the diff: https://gist.github.com/bb8f751131a1b7c4c43ab48b5065ce5f.

tamird · 2017-10-06T19:13:21Z

protoc-gen-gogo/generator/generator.go

-	// Convert dots to underscores before finding a unique alias.
-	pkg = strings.Map(badToUnderscore, pkg)
+	// Only make pkg names unique if they are non-standard lib
+	if f != nil {


The comment looks incorrect though, judging by the method comment which says:

If f is nil, it's a builtin package like "proto" and has no file descriptor.

So it's not that the package is in the stdlib, it's that it is "a builtin package", whatever that means.

tamird · 2017-10-06T20:52:35Z

protoc-gen-gogo/grpc/grpc.go

-	g.P(")")
-	g.P()
+	grpcImports := []string{
+		path.Join(g.gen.ImportPrefix, contextPkgPath),


this bit is incorrect - there is no need for this path.Join - the imports object already takes care of the import prefix. In CockroachDB, this causes the import prefix to appear twice.

awalterschulze · 2017-10-07T12:43:57Z

I meant to put this comment in the main thread

I wonder if a package like sortkeys is a f!=nil package and if so I wonder what happens when I also try to import a proto file with package name sortkeys.

Maybe it is time to start adding a test like this one?

tamird

9 commits seems excessive, though.

meling · 2017-11-08T00:30:26Z

Yeah, I guess they should be squashed into a single commit at some point when it is ready to be merged, but I'm not sure how legible the commits will be anyway with all the generated noise. Anyway, I planning to take a stab at adding the tests that @awalterschulze requested; these last few commits are just to bring my fork up to dated with the master branch.

The importduplicate test tests if the fix to avoid long import names is still able to detect duplicate import names.

meling · 2017-11-08T03:23:16Z

@awalterschulze I've prepared a new test to check that my proposed patch still works when a proto files uses a generated package (from another proto file) that conflicts with the name of an internal package (in this case, sortkeys as suggested earlier). Let me know if you want something more in addition to this.

meling · 2017-11-08T03:43:13Z

I discovered a problem, if the proto package name is proto, and there is also an import like this:

import proto "github.com/gogo/protobuf/proto"

My fix won't detect this. I'm thinking about a fix.

awalterschulze · 2017-11-08T08:08:04Z

I am really positive about this change. I can squash the commits when I merge. Also github makes it easy to review the generated code. I don't see it as noise. Good luck with the final issue

…

On Wed, 8 Nov 2017, 04:43 Hein Meling, ***@***.***> wrote: I discovered a problem, if the proto package name is proto, and there is also an import like this: import proto "github.com/gogo/protobuf/proto" My fix won't detect this. I'm thinking about a fix. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#341 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLSWWfaATGLIOoLjXw4clE1aJGSf5ks5s0SNRgaJpZM4PudnO> .

It seems that PR gogo#316 is only sound if you ensure that the package name does not overlap with a builtin type, such as proto. Hence, this commit reverts my previous attempt to incorporate gogo#316.

meling · 2017-11-09T01:37:29Z

I've tried a wide range of things to make the imports more readable, including trying to incorporate #316, but that fails with my new tests in test/importduplicate. Hence, unless you think my tests are unreasonable (importing from a sub package proto/proto.proto), I think you can close #316.

Anyway, I've resolved the issues I discovered yesterday, and so this should be ready to be merged.

PS: If you import a type whose name overlap with the package name of the proto file, then that type gets named type1. For example, in test package types, there will be an import like this:

import types1 "github.com/gogo/protobuf/types"

This is strictly not necessary, as Go allows importing types even though the package is also types. However, fixing that issue is beyond the need for my other project, and it does not appear trivial to fix.

awalterschulze · 2017-11-09T18:15:13Z

I think thats fair.
Really great work :)
Thank you very much. I really appreciate it.

tamird · 2017-11-09T18:16:06Z

Womp womp, no squash

awalterschulze · 2017-11-09T18:16:23Z

My bad.

awalterschulze · 2017-11-09T18:17:14Z

Sorry

meling added 2 commits October 4, 2017 18:25

Fix to avoid long import alias names when not needed.

ca6d352

It required a minor change in the grpc package, but otherwise the tests passed (surprisingly).

Fix to avoid printing import alias when same as import path suffix.

3ae4cfd

awalterschulze reviewed Oct 5, 2017

View reviewed changes

Readded alias name to import statements.

c59a8c7

tamird suggested changes Oct 6, 2017

View reviewed changes

meling added 4 commits October 6, 2017 17:59

Fixed misleading comment.

874a222

Fixed double import prefix issue.

48a47cb

Regenerated pb.go files.

44008aa

Revised comment regarding the uniqueness of package names.

79e6522

meling mentioned this pull request Oct 20, 2017

Manual changes to vendor/ breaks 'dep ensure (-update)' relab/gorums#39

Closed

meling added 2 commits November 7, 2017 15:24

Merge remote-tracking branch 'upstream/master'

b989071

Removed unsafe stuff that got readded; regenerated .pb.go files

b0a8a05

tamird approved these changes Nov 8, 2017

View reviewed changes

Test duplicate import names from proto package and generated code.

685a9b3

The importduplicate test tests if the fix to avoid long import names is still able to detect duplicate import names.

meling added 4 commits November 8, 2017 16:14

Fix duplicate import names when same name used in different contexts

563235a

Fix issue when provided f!=nil

bf3b9f4

It seems that PR gogo#316 is only sound if you ensure that the package name does not overlap with a builtin type, such as proto. Hence, this commit reverts my previous attempt to incorporate gogo#316.

Minor update to the comment.

e683811

Ensure v3 to compile importduplicate.proto

ff2773e

awalterschulze merged commit 616a82e into gogo:master Nov 9, 2017

awalterschulze mentioned this pull request Nov 9, 2017

grouping similar imports in .pb.go file #316

Closed

meling mentioned this pull request Nov 9, 2017

Can I generate imports without the import alias (or without the long underscored import path)? #338

Open

meling mentioned this pull request Apr 10, 2018

protoc-gen-go: handle package import names per-file golang/protobuf#543

Merged

Fix to avoid printing import aliases when not needed #341

Fix to avoid printing import aliases when not needed #341

Uh oh!

Conversation

meling commented Oct 5, 2017

Uh oh!

awalterschulze Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

awalterschulze Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

tamird Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

tamird Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

meling Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Oct 5, 2017

Uh oh!

awalterschulze commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meling commented Oct 5, 2017

Uh oh!

awalterschulze commented Oct 6, 2017

Uh oh!

awalterschulze commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

tamird Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

tamird Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

awalterschulze commented Oct 7, 2017

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

meling commented Nov 8, 2017

Uh oh!

meling commented Nov 8, 2017

Uh oh!

meling commented Nov 8, 2017

Uh oh!

awalterschulze commented Nov 8, 2017 via email

Uh oh!

meling commented Nov 9, 2017

Uh oh!

awalterschulze commented Nov 9, 2017

Uh oh!

tamird commented Nov 9, 2017

Uh oh!

awalterschulze commented Nov 9, 2017

Uh oh!

awalterschulze commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

awalterschulze commented Oct 5, 2017 •

edited

Loading

awalterschulze commented Oct 6, 2017 •

edited

Loading