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

Add binary size bounds checking #1079

Merged
merged 27 commits into from Mar 2, 2020
Merged

Add binary size bounds checking #1079

merged 27 commits into from Mar 2, 2020

Conversation

coilysiren
Copy link
Member

@coilysiren coilysiren commented Feb 29, 2020

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature
  • maintenance / operations

What this PR does / why we need it:

This PR adds binary size bounds checking, so that we can avoid more people having issues like that one described in #1055.

Which issue(s) this PR fixes:

Fixes #1057

Changes

  • moved build.go to internal/build.go
  • I didn't change any of the existing bits of build.go 😝 (except the top comment) the git diff is just betraying me
  • adds a build script for checking binary size

Special notes for your reviewer:

Here's parts of this PR that I find interesting:

  • the binary size diffs are different on ubuntu / mac / windows, and different on different go versions. as of a8a1ef0 they're 2.0, 2.1.
  • the specific values for the min and max sizes are interesting to me, I think there being 0.5MB between them is a comfortable distance. I would be open to decreasing the min and max to fit the current values more tightly (eg. min 4.7 max 4.9), though I changed the min and max to just be the min and max size diffs on the various platforms
  • I assume that the next person to trigger test failures based on binary size won't be me, eg. it'll be some new open source contributor working on their 1st PR. For that person, do you think the included guidance is sufficient?

Potential Future Work

I thought about adding either of these things:

I think they'd be cool to add ✨ but I'm not that invested in adding them personally.

Trivia

People really care about binary sizes! golang/go#6853

Testing

I briefly changed the min and max values to make sure that they are actually exiting with a failure code and displaying the guidance properly.

Release Notes

There's no user-facing changes in this PR ^_^

@codecov
Copy link

codecov bot commented Feb 29, 2020

Codecov Report

Merging #1079 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   72.83%   72.91%   +0.07%     
==========================================
  Files          33       33              
  Lines        2474     2481       +7     
==========================================
+ Hits         1802     1809       +7     
  Misses        565      565              
  Partials      107      107              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c380b...a8a1ef0. Read the comment docs.

@coilysiren coilysiren self-assigned this Feb 29, 2020
@coilysiren coilysiren marked this pull request as ready for review February 29, 2020 09:00
@coilysiren coilysiren requested a review from a team as a code owner February 29, 2020 09:00
@coilysiren coilysiren requested review from rliebz and asahasrabuddhe and removed request for a team February 29, 2020 09:00
@coilysiren coilysiren changed the title WIP binary size work Add binary size bounds checking Feb 29, 2020
@coilysiren
Copy link
Member Author

Possible future optimization: instead of the raw size, use the diff between main.go with "hello world" and a main.go with urfave/cli

internal/build/build.go Outdated Show resolved Hide resolved
internal/build/build.go Outdated Show resolved Hide resolved
internal/build/build.go Show resolved Hide resolved
coilysiren and others added 2 commits February 29, 2020 13:21
Co-Authored-By: Sascha Grunert <sgrunert@suse.com>
@coilysiren coilysiren merged commit 1b7e4e0 into master Mar 2, 2020
@coilysiren coilysiren deleted the binary-size branch March 2, 2020 20:01
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

Successfully merging this pull request may close these issues.

maint: add binary size checking to CI
3 participants