Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/cmd/goimports: can't find proto packages #16600

Open
danielchatfield opened this issue Aug 4, 2016 · 11 comments
Open

x/tools/cmd/goimports: can't find proto packages #16600

danielchatfield opened this issue Aug 4, 2016 · 11 comments
Milestone

Comments

@danielchatfield
Copy link

@danielchatfield danielchatfield commented Aug 4, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?

go version go1.6.2 darwin/amd64

  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/danielchatfield"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.6.2/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.6.2/libexec/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT="1"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"

Package fooproto in github.com/danielchatfield/repo/foo/proto

Running goimports on the following fails to find the import.

package main

func main() {
    _ := &fooproto.SomeStruct{}
}

The first commit that broke this is https://github.com/golang/tools/commit/e047ae774b027ae958a270598c5ac2591e457afc

@danielchatfield danielchatfield changed the title x/tools/cmd/goimports x/tools/cmd/goimports Can't find proto packages Aug 4, 2016
@quentinmit quentinmit added this to the Unreleased milestone Aug 4, 2016
@josharian josharian changed the title x/tools/cmd/goimports Can't find proto packages x/tools/cmd/goimports: can't find proto packages Aug 4, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 4, 2016

This is a side-effect of the pruning of the search space we now do. We have a function which determines whether a package like "fooproto" might appear in a directory as a function of its directory name, considering mostly its final two path components.

It ignores case, and drops hyphens and periods, and then looks for substrings. But "fooproto" doesn't appear in "foo/proto". I suppose I could also try concatenating the last two path components and searching that too.

But does that solve the general problem?

How do protobuf package names & directory names look like in the wild? I haven't been following what the conventions are outside of Google or at least outside of Google open source projects.

@akalin-keybase

This comment has been minimized.

Copy link

@akalin-keybase akalin-keybase commented Aug 22, 2016

Is this one instance of the general problem that a package whose name differs from its containing directory won't be found by goimports anymore?

i.e. if I have $GOPATH/src/github.com/akalin/mymain/main.go with:

package main

import (
    "github.com/akalin/mypkg"
    "github.com/akalin/mypkg2"
)

func main() {
    mypkg.Foo()
    myotherpkg.Bar()
}

where the mypkg package is in github.com/akalin/mypkg, but the myotherpkg package is in github.com/akalin/mypkg2, then goimports will automatically insert the first import if omitted, but won't insert the second import if omitted? (This is the current behavior, according to my tests.)

Sounds like this is a casualty of the new speed improvements. If so, it's unfortunate but understandable. I didn't run into this with protobuf packages, but with similar auto-generated code, namely https://github.com/keybase/client/tree/master/go/protocol , which has package name keybase1 but directory name protocol.

It's probably impossible to solve this in general without slowing down goimports again, but I wonder if we could have some sort of convention for auto-generated code, such that the directory name is some function of the package name, e.g. pkgName + "Autogen".

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 23, 2016

Sounds like this is a casualty of the new speed improvements.

Yup.

If so, it's unfortunate but understandable.

It's at least fortunate in one way: it gives yet another reason to name your packages the conventional way, making it a bit tedious if you want to violate the norms.

I definitely wouldn't penalize people like that on purpose, but if it's necessary to make goimports fast, I think it's quite fine.

I wouldn't support any proposal (like pkgName + "Autogen") to make violating the norm easier. There's no reason for autogen packages to declare that they're auto-generated in their package name. If it really needs to be in the import path, it can be in the second to last path component or something.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 25, 2016

@dsymonds, can you answer my question in #16600 (comment)? Are there conventions for how people name their proto packages vs. how they end up on disk in their import path?

@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Aug 25, 2016

I'm not sure there's a well-established convention. We encourage people to always rename proto imports for this kind of reason.

If you're going to apply any hack, I'd suggest bypassing the pruning if the import name ends in "pb", which should rarely happen except for protos.

@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Aug 25, 2016

At $COMPANY our protobuf build scripts move our generated code to a directory that matches the package name specifically to avoid violating the Go convention and to make sure that tools like goimports work.

(As far as the special names thing goes, we also name our packages specially using option go_package = "foopb"; to make it easier to identify which packages are generated code; this seems orthogonal to the package name != path issue, though.)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 25, 2016

If you're going to apply any hack, I'd suggest bypassing the pruning if the import name ends in "pb", which should rarely happen except for protos.

Oh, that's an interesting idea, to selectively prune. Or I can at least filter off "pb", and then prune.

Does some tool default to the "pb" suffix, or is that just convention?

@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Aug 25, 2016

It's a strong convention. The part before the "pb" (and sometimes there's nothing before the pb) is human chosen, and isn't always consistent for a given imported package. From memory, golint and other similar tools know the "pb" suffix convention and modify their behaviour a bit.

@bradfitz bradfitz removed their assignment May 29, 2019
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 29, 2019

@bufdev

This comment has been minimized.

Copy link

@bufdev bufdev commented Nov 28, 2019

Just a note - if there was some convention here that could be added, it would be great. This is a constant pain point of mine. I think that there is a solution where you either special case for Protobuf, or provide some sort of flag to goimports that checks a certain package suffix. Protobuf usage with Golang is common enough that it might be worth handling this - this is such a common pain point for me that I'm just going to have a fork on my personal account for now that adds the 5 LOC necessary to handle this (it would be more LOC to handle the general case obviously though).

There's two cases for Protobuf that are important:

  1. Non-versioned Protobuf packages, such as foo.bar.baz. The general convention is that the Golang package in this case should be bazpb, as mentioned above. This can be handled by adding a check for the suffix pb from the last component (not the last two), which shouldn't slow down goimports. Even better would be to add a flag to goimports that takes a list of such suffixes to check, and then this isn't even special-casing for Protobuf.

  2. Versioned Protobuf packages, such as foo.bar.baz.v1. The general convention (as I see it and use it) is to concatenate the last two, i.e. bazv1. This is as simple as removing all \ and / from the result of lastTwoComponents in internal/imports/fix.go and then re-checking.

Is there any way we could re-open this and check this? I'm happy to contribute a PR if so.

cc @myitcv as well

@bufdev

This comment has been minimized.

Copy link

@bufdev bufdev commented Nov 28, 2019

Just as an addition, what I'm proposing: golang/tools@ecd3221...bufdev:proto

This would obviously need to be cleaned up, this is doing this in the most inefficient way possible with the least amount of code edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.