-
Notifications
You must be signed in to change notification settings - Fork 19
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
CI production build size summary #401
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Where does the output go? I haven't used GITHUB_STEP_SUMMARY
before
Top level of the workflow for the PR/commit (disappointingly, 3 clicks from a PR (show all checks, pick anything, hit summary top left)) - this one's at https://github.com/kylebarron/parquet-wasm/actions/runs/7068033091. There's probably/hopefully a Github app that surfaces it in the PR (like Dependabot) :-/. Also, pv apparently uses base-2 SI prefixes (about 5% deviation at the MiB vs MB level) 😞 |
Excellent, found one, adding it now. |
are the failing runs expected? |
Yeah, the safeguards in Github Actions prevented the comment creation from going through - I've added a workflow_run indirection (described at https://securitylab.github.com/research/github-actions-preventing-pwn-requests/, I figure this particular bit of CI is largely set and forget, so the added hoops are fine). The comment workflow won't take effect until it's merged, this dummy PR demonstrates what it should look like (and the comment upsert behaviour): H-Plus-Time#2 |
That looks great! I think one additional improvement that would be great is having sizes for a couple different build options: i.e.
Then you'd also be able to see whether changes are affecting only the size in the async bundle, for example. A delta with the previous known size would also be cool, but that sounds pretty hard, and it's not too hard to find the most recent PR. I'm happy to merge this as is though, what do you think? |
🤔 yeah, though it looks like we'd have to use a synthetic full target (the one in cargo.toml breaks with symbol collisions, presumably arrow1, arrow2 in the same bundle). I ended up just switching away from scripts/build.sh (one platform target, multiple feature flags is relevant for reporting, while multiple targets, fixed feature flags is relevant for publishing). Since it's a new script, I figured I'd just ignore the arrow2 build as well (trivial to add it anyway, if we still want to keep an eye on build size for the duration of the deprecation window).
It looks like the deltas are fully functional (and correct) too 🎉 . Also, apparently, closing and re-opening PRs (so long as they still have un-merged changes) is sufficient to trigger fresh workflows (with the results based on the target branch), so that's all we have to do with the motivating example (#400 ) to see this in action. |
7f1dc95
to
770a0a7
Compare
The comment output from the equivalent PR in my fork: Asset Sizes
|
arrow2 hasn't gotten a commit in a month and at this point I consider it on its way to dying, sadly. So I'm content with deprecating and removing arrow2 as soon as the arrow1 bindings have a superset of functionality |
Thanks again! |
Hmm looks like this failed on my PR . https://github.com/kylebarron/parquet-wasm/actions/runs/7215812740/job/19660787302 Maybe that's because there wasn't a previous summary to download from? |
I would have liked to do a head vs target % change (apparently there's a way involving artifacts), but this is a sufficiently useful gauge. Presumably the artifacts required for that would also be used in package publishing.
As far as I can tell, it's only the JS bindings that differ between targets, so we can probably just do
pkg/esm/*.wasm
and be done with it.