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

Bundle-size delta check/message on PR #501

Open
LesnyRumcajs opened this issue Jul 20, 2022 · 3 comments · May be fixed by #937
Open

Bundle-size delta check/message on PR #501

LesnyRumcajs opened this issue Jul 20, 2022 · 3 comments · May be fixed by #937
Assignees
@LesnyRumcajs
Labels
build good first issue Good for newcomers P2

Comments

@LesnyRumcajs
Copy link
Contributor

LesnyRumcajs commented Jul 20, 2022 •
edited
Loading

Based on a discussion with @anorth in #497 it would be beneficial to include the bundle size delta between master and candidate branch in a PR message.

Some changes are clearly increasing bundle size, but some of them are a bit sneaky (e.g. bumping a dependency that by default would now include a large set of unused features). It would be great to not have to have it at the back of the head but right in front of us.

I'd imagine a GH bot posting a message to PR (or modifying an existing one to not bloat the discussion) with a simple cargo clean && cargo build && make bundle && du -b output/builtin-actors.car on both branches, calculating the delta in bytes and %.

Probably something along the lines of this workflow would do the trick (that is to say, having a modifiable message with shell output): https://github.com/LesnyRumcajs/grpc_bench/blob/master/.github/workflows/issue_bench.yml
@anorth
Copy link
Member

anorth commented Jul 20, 2022

Thanks!

An alternative implementation is a build step that writes the output size to a file that's checked in to git (orchestrate from Makefile or similar). In CI, build should check that the calculated value equals the written value. This puts the delta into code review directly rather than a comment on the GitHub issue, and makes the history of sizes easy to see.

Either could work,

@jennijuju jennijuju added this to the Network v17 milestone Aug 11, 2022
@anorth anorth moved this to Todo in Network nv17 Aug 11, 2022
@anorth anorth added the good first issue Good for newcomers label Aug 11, 2022
@anorth anorth moved this from Todo to Opportunistic in Network nv17 Aug 11, 2022
@jennijuju jennijuju removed this from Network nv17 Nov 25, 2022
@jennijuju jennijuju moved this to 📋 Backlog in Network v18 Nov 25, 2022
@jennijuju
Copy link
Member

@LesnyRumcajs would you be open to take a stab on this one?

@LesnyRumcajs
Copy link
Contributor Author

@jennijuju Sure thing, I'll look into this next week.

@LesnyRumcajs LesnyRumcajs self-assigned this Dec 7, 2022
@jennijuju jennijuju removed this from the Network v17 milestone Dec 7, 2022
@jennijuju jennijuju moved this from 📋 Nice To Haves to 🏗 In progress in Network v18 Dec 9, 2022
@LesnyRumcajs LesnyRumcajs linked a pull request Dec 12, 2022 that will close this issue
Draft
@jennijuju jennijuju removed this from Network v18 Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LesnyRumcajs LesnyRumcajs

Labels
build good first issue Good for newcomers P2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Bundle size delta filecoin-project/builtin-actors
3 participants
@anorth @LesnyRumcajs @jennijuju