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.

Sign up for GitHub

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

Jump to bottom

x/tools/cmd/goimports: prefer main module requirements #31030

Open
myitcv opened this issue Mar 25, 2019 · 2 comments
Open

x/tools/cmd/goimports: prefer main module requirements #31030

myitcv opened this issue Mar 25, 2019 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@myitcv
Copy link
Member

myitcv commented Mar 25, 2019

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

$ go version
go version go1.12.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build898038110=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran testscript on the following:

# install goimports
env GOMODPROXY=$GOPROXY
env GOPROXY=
go install golang.org/x/tools/cmd/goimports

# switch back to our local proxy
env GOPROXY=$GOMODPROXY

# "warm" the module (download) cache
go get gopkg.in/tomb.v1
go get gopkg.in/tomb.v2

# test goimports
cd mod
go mod tidy
exec goimports main.go
stdout '\Q"gopkg.in/tomb.v2"\E'

-- go.mod --
module goimports

require golang.org/x/tools v0.0.0-20190322203728-c1a832b0ad89

-- mod/go.mod --
module mod

require gopkg.in/tomb.v2 v2.0.0

-- mod/main.go --
package main

import (
	"fmt"
)

func main() {
	fmt.Println(tomb.Tomb)
}

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/.mod --
module gopkg.in/tomb.v1

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/.info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/go.mod --
module gopkg.in/tomb.v1

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/tomb.go --
package tomb

const Tomb = "A great package v1"

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/.mod --
module gopkg.in/tomb.v2

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/.info --
{"Version":"v2.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/go.mod --
module gopkg.in/tomb.v2

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/tomb.go --
package tomb

const Tomb = "A great package v2"

What did you expect to see?

A passing run.

What did you see instead?

A failed run.

goimports should be "consulting" the main module for matches before dropping down to a module cache-based search. Here that would have resulted in gopkg.in/tomb.v2 being correctly resolved, instead of gopkg.in/tomb.v1.

This will, I suspect, also massively improve the speed of goimports in a large majority of cases.

cc @heschik
@myitcv myitcv added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 25, 2019
@gopherbot gopherbot added this to the Unreleased milestone Mar 25, 2019
@heschi
Copy link
Contributor

heschi commented Mar 25, 2019

@ianthehat

This would be fairly easy to do as a separate pass. I don't think it would make a huge performance difference for most people, but maybe if you have a gigantic module cache or are looking for a very generic package like "util" or something. For me, the stronger argument is just that we should be biased against adding new deps when an existing dep suffices.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@DeedleFake
Copy link

Any progress on this? It's a constant thorn in my side when dealing with any dependency that's v2 or up.

I don't think it would make a huge performance difference for most people

It might actually be quite a bit faster, as I think that 99% of the time you're looking for a package that's in one of the dependencies in go.mod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@DeedleFake @myitcv @gopherbot @heschi