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

Upgrade go 1.15 for smaller binary #1303

Merged
merged 1 commit into from
Aug 18, 2020
Merged

Upgrade go 1.15 for smaller binary #1303

merged 1 commit into from
Aug 18, 2020

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Aug 12, 2020 •
edited
Loading

The binary is much smaller from 37MB to 31MB

-rwxrwxr-x  1 tammach tammach 38016216 Aug 12 22:36 golangci-lint.1.14
-rwxrwxr-x  1 tammach tammach 32142632 Aug 12 22:44 golangci-lint

Performance test

### Cilium project
$ golangci-lint cache clean
$ time golangci-lint run ./...
golangci-lint run ./...  100.29s user 2.16s system 382% cpu 26.753 total

$ golangci-lint-1.15 cache clean
$ time golangci-lint-1.15 run ./...
golangci-lint-1.15 run ./...  98.14s user 2.21s system 380% cpu 26.389 total

### Kubernetes project
$ golangci-lint cache clean
$ time golangci-lint run --timeout 30m ./...
golangci-lint run --timeout 30m ./...  996.16s user 15.05s system 1333% cpu 1:15.85 total

$ golangci-lint-1.15 cache clean
$ time golangci-lint-1.15 run --timeout 30m ./...
golangci-lint-1.15 run --timeout 30m ./...  989.70s user 15.09s system 1307% cpu 1:16.83 total

Please refer to the commit message for more details.

Signed-off-by: Tam Mach [email protected]

@sayboras sayboras marked this pull request as draft August 12, 2020 12:52
@sayboras
Copy link
Member Author

checking on failed test, so mark this one as draft

@SVilgelm
Copy link
Member

LGTM

@ernado
Copy link
Member

ernado commented Aug 12, 2020

please also check for performance regressions on large repos, e.g. on k8s

@sepetrov
Copy link

I believe this will resolve #1318

@SVilgelm SVilgelm linked an issue Aug 18, 2020 that may be closed by this pull request
Missing warning for string(x) #1318
Closed
3 tasks
@sayboras sayboras force-pushed the feature/golang-1-15 branch from 962738e to 3e4a868 Compare August 18, 2020 12:14
@sayboras
build(golang1.15): Upgrade to golang 1.15 for smaller binary
12adeca 
Golang 1.15 comes to few improvements, one of them is to have smaller
binary.

This PR is to make golang 1.15 as default version in CI, I also update
Docker based image to golang:1.15* as well.

Two issues faced with golang 1.15:
- Conflict between -v in `golangci-lint` and `go test`. Update to --verbose
to avoid the same.
- `nolintlint_unused.go` testdata is not matching regex. Correct by adding one
space after //

[1]: golang/go#40763

Signed-off-by: Tam Mach <[email protected]>
@sayboras sayboras force-pushed the feature/golang-1-15 branch from 34bd373 to 12adeca Compare August 18, 2020 13:06
@sepetrov
Copy link

I wonder, shouldn't we also have the min Go version in go.mod updated to Go 1.15? golangci-lint is meant to be distributed as a binary.

@sayboras
Copy link
Member Author

I wonder, shouldn't we also have the min Go version in go.mod updated to Go 1.15? golangci-lint is meant to be distributed as a binary.

There were a few conversations on this matter. Some of them can be found #1210 and #1269

@sayboras sayboras marked this pull request as ready for review August 18, 2020 13:17
@sayboras sayboras requested review from a team August 18, 2020 13:17
@sayboras
Copy link
Member Author

For other reviewer, the failure in vulnerability scanner is not related to this PR. Will address seperately.

@SVilgelm
Copy link
Member

nancy fails due to a vulnerability in etcd, see the #1316

SVilgelm
SVilgelm approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@SVilgelm SVilgelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you

@sayboras Did you try to run it on huge repos, like k8s? Just to compare

@sayboras
Copy link
Member Author

Looks good to me. Thank you

@sayboras Did you try to run it on huge repos, like k8s? Just to compare

Yes, I did. I ran for cilium and k8s projects (as I have these two in my machine), details can be found in PR description.

@ernado
Copy link
Member

ernado commented Aug 18, 2020

Great, thank you @sayboras!

ernado
ernado approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@ernado ernado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sayboras sayboras merged commit 18fd36b into golangci:master Aug 18, 2020
@sayboras sayboras deleted the feature/golang-1-15 branch August 18, 2020 23:05
@jerome-laforge jerome-laforge mentioned this pull request Aug 29, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Jun 22, 2021
Closed
@snyk-bot snyk-bot mentioned this pull request Sep 21, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Oct 28, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Dec 2, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Jan 30, 2022
Open
@alpha-phoenicis alpha-phoenicis mentioned this pull request Oct 27, 2022
Open
@DevenGengan DevenGengan mentioned this pull request Feb 16, 2024
Open
@ldez ldez added this to the v1.31 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin approved these changes

@ernado ernado ernado approved these changes

@SVilgelm SVilgelm SVilgelm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.

Missing warning for string(x)
6 participants
@sayboras @SVilgelm @ernado @sepetrov @denis-tingaikin @ldez