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

Module rename has broken indirectly golangci-lint #34

Open
XoseRamon opened this issue May 22, 2019 · 16 comments
Open

Module rename has broken indirectly golangci-lint #34

XoseRamon opened this issue May 22, 2019 · 16 comments

Comments

@XoseRamon
Copy link

Your latest commit s/Quasilyte/quasilyte might have broken projects that reference indirectly (maybe even directly, not sure) go-consistent since go mod is case-sensitive and checks the paths (see this issue).

I am not sure if this is the right place to report it, but since it is the source of the error, I just wanted to warn you in case it is an unexpected side effect. The dependency chain is golangci-lint -> go-critic -> go-consistent.

To reproduce:

GO111MODULE=on go get -u github.com/golangci/golangci-lint/cmd/golangci-lint@master              
go: finding github.com/golangci/golangci-lint/cmd/golangci-lint master                                                                      
go: finding github.com/golangci/golangci-lint/cmd master                                                                                    
go: finding github.com/golangci/golangci-lint master                                                                                        
go: finding github.com/golangci/goconst latest
...
 go: finding github.com/golangci/unconvert latest
go: github.com/Quasilyte/[email protected]: parsing go.mod: unexpected module path "github.com/quasilyte/go-consistent"
go: finding github.com/logrusorgru/aurora latest
go: finding gopkg.in/tomb.v1 latest
go: finding github.com/StackExchange/wmi latest
go get: error loading module requirements     
@quasilyte
Copy link
Owner

Hmm, I think go-consistent is only used in ci for go-critic. It shouldn't be a first-class dependency. I'll take a look, thanks.

@mec07
Copy link

mec07 commented May 23, 2019

I'm having the same problem... 😢

@mec07
Copy link

mec07 commented May 23, 2019

I've found a solution, it's not super satisfactory, but seems to work in circleci:
go get github.com/golangci/golangci-lint/cmd/golangci-lint@de1d1ad
It came from this: golang/go#31148

quasilyte added a commit to go-critic/go-critic that referenced this issue May 23, 2019
Refs quasilyte/go-consistent#34

Signed-off-by: Iskander Sharipov <[email protected]>
@quasilyte
Copy link
Owner

See the referenced commit. Should help to resolve the problem.

quasilyte added a commit to go-critic/go-critic that referenced this issue May 23, 2019
(I was not struggling with this commit at all. Not a bit. No.)

Refs quasilyte/go-consistent#34

Signed-off-by: Iskander Sharipov <[email protected]>
@mec07
Copy link

mec07 commented May 23, 2019

Thanks for this. I tried it out again to see if the issue was resolved by your commit, but it's still happening...

go: github.com/Quasilyte/[email protected]: parsing go.mod: unexpected module path "github.com/quasilyte/go-consistent"

So I'm sticking with the suboptimal solution for now:

go get github.com/golangci/golangci-lint/cmd/golangci-lint@de1d1ad

@thepudds
Copy link

@quasilyte When modules are enabled, the go tool enforces that the module name declared in the module statement on the first line of the go.mod matches the import path used by consumers.

Is it necessary to change the case for the import path for github.com/Quasilyte/go-consistent?

Changes like that are breaking changes. They can end up having wide repercussions across the ecosystem.

For example, Sirupsen/logrus vs. sirupsen/logrus has caused a large amount of churn (see for example golang/go#28489). A similar but slightly different example is golang.org/x/lint vs. github.com/golang/lint (e.g., golang/lint#436 and golang/go#30831), where the ecosystem was using two different import paths and introducing a canonical name in the go.mod caused problems for many consumers.

golangci-lint happens to be having other issues now (including due to a change in x/tools/go/packages), but would be good if this rename here could be avoided?

CC @dmitshur @matloob

@quasilyte
Copy link
Owner

go-consistent is not a library and the reason it end up in a dep tree is https://github.com/go-critic/go-critic/blob/master/tools.go. I don't like that a tool that is used during ci tests of go-critic is a dependency of golangci-lint. This is the problem, not the import path, it shouldn't be a dependency on the first place.

@quasilyte
Copy link
Owner

And now import path should match. I've done the renaming in go-critic.

@thepudds
Copy link

thepudds commented May 27, 2019

Hi @quasilyte, With the change you made in go-critic, do upgrades end up working for you?

For example, if I do:

cd $(mktemp -d)
go mod init tempmod
go get -m github.com/go-critic/go-critic@f6f702b31734df2
go get -u

It fails during the upgrade step with:

go: finding github.com/Quasilyte/go-consistent latest
go: github.com/Quasilyte/[email protected]: 
parsing go.mod: unexpected module path "github.com/quasilyte/go-consistent"
go: finding github.com/logrusorgru/aurora latest
go: finding golang.org/x/tools latest
go get: error loading module requirements

@thepudds
Copy link

That was a synthetic example intended to show the problem more clearly, but here is closer to a real example:

cd $(mktemp -d)
go mod init tempmod
go get github.com/golangci/golangci-lint@master
go get -u

which fails during the upgrade step with:

...
go: github.com/Quasilyte/[email protected]: 
parsing go.mod: unexpected module path "github.com/quasilyte/go-consistent"
go get: error loading module requirements

@quasilyte
Copy link
Owner

go get -m github.com/go-critic/go-critic@f6f702b31734df2

Do you get the revision prior to the fix intentionally?

golangci-lint probably uses gocritic version prior to the fix as well. So it needs to be updated. But all that is an issue only if you're using unreleased versions, I think.

@quasilyte
Copy link
Owner

quasilyte commented May 27, 2019

Changes like that are breaking changes. They can end up having wide repercussions across the ecosystem

I believe it's mostly true for libraries.

For example, Sirupsen/logrus vs. sirupsen/logrus has caused a large amount of churn (see for example golang/go#28489). A similar but slightly different example is golang.org/x/lint vs. github.com/golang/lint (e.g., golang/lint#436 and golang/go#30831), where the ecosystem was using two different import paths and introducing a canonical name in the go.mod caused problems for many consumers.

I hear you, but again, those are released libraries intended for public use. Go-consistent is a linter, not something that others can/should depend on inside their modules. The problem is that it made it's way into dependencies tree of golangci-lint. We should probably concentrate on that. This is my point of view.

@thepudds
Copy link

thepudds commented Jun 9, 2019

FWIW, the new v1.17.0 release of golangci-lint seems to fail when doing a go get -u:

cd $(mktemp -d)
go mod init tempmod
go get -u github.com/golangci/[email protected]

which fails with:

go: github.com/Quasilyte/[email protected]: parsing go.mod: unexpected module path "github.com/quasilyte/go-consistent"

Some related discussion in golangci/golangci-lint#479.

@jirfag
Copy link

jirfag commented Jun 10, 2019

try go get -v -u github.com/golangci/[email protected], please

@cristaloleg
Copy link
Contributor

Close?

@quasilyte
Copy link
Owner

Should be fixed by go-critic/go-critic#1162
If anyone can confirm that it is, we can close this issue.

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

No branches or pull requests

6 participants