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

issues when upgrading to the latest release #391

Closed
RaduBerinde opened this issue Jan 11, 2019 · 7 comments
Closed

issues when upgrading to the latest release #391

RaduBerinde opened this issue Jan 11, 2019 · 7 comments

Comments

@RaduBerinde
Copy link

Hi,

I am hoping we can get some help to move CockroachDB to the latest release of go-tools.
We have a TestUnused is trying to run the unused checker. This is the change I made to it so it builds with the new release: (only the lint_test.go file is relevant): https://github.com/cockroachdb/cockroach/compare/master...RaduBerinde:update-honnef?expand=1#diff-24cf5c91d964c43f696360c5bc9286f8

With this change, it builds but the test times out after 10 minutes, using 30+GB RAM. For reference, with the previous version it runs for 30s.

I am hoping someone can take a quick look over the diff and check if I'm doing something egregiously wrong; if I'm not, then there must be a problem with the unused checker.

@dominikh
Copy link
Owner

With which version(s) of Go does this occur? 1.10 or 1.11 or both? What's the exact invocation/set of packages passed to unused?

@RaduBerinde
Copy link
Author

The packages are all packages in https://github.com/cockroachdb/cockroach/tree/master/pkg:
paths := ctx.ImportPaths([]string{cockroachDB + "/pkg/..."})

This was with 1.10. Hm, I just tried it with 1.11 and it finishes (but it spits out a ton of problems, basically everything that isn't used within its own package). I'm invoking it with this:

		paths := ctx.ImportPaths([]string{cockroachDB + "/pkg/..."})

		unusedChecker := unused.NewChecker(unused.CheckAll)
		unusedChecker.WholeProgram = true

		problems, err := lintutil.Lint(
			[]lint.Checker{unused.NewLintChecker(unusedChecker)},
			paths,
			&opt,
		)

Is there a known issue in go 1.10 that's fixed in 1.11?

@dominikh
Copy link
Owner

Is there a known issue in go 1.10 that's fixed in 1.11?

Not as such, but the two employ different ways of loading packages and have different performance characteristics in doing so. There is also #377 which may be related.

@RaduBerinde
Copy link
Author

I see, thanks! We're switching to go 1.11 anyway, so that's all good. I still don't know how to correctly invoke it so that it only reports values that are not used in any of the packages (with the invocation above it shows everything that's not used within the package it's defined).

This is how we were doing it with 2017.2.2: https://github.com/cockroachdb/cockroach/blob/master/pkg/testutils/lint/lint_test.go#L839

@dominikh
Copy link
Owner

I will look at it when I get the chance.

@RaduBerinde
Copy link
Author

Thank you so much for your help! No rush whatsoever!

@RaduBerinde
Copy link
Author

I worked on this some more and I just needed to set LintTests. Closing, thanks for the help!

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

2 participants