Skip to content

ci: remove binary size check #710

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

Conversation

workingjubilee
Copy link
Member

Remove the "binary size check" workflow because it is often broken and now and then makes me sit through a discussion about its security needs. These discussions always make me feel very silly.

I originally hoped this CI job would be a useful part of assuring that we would not accidentally bloat binaries when backtrace merges into std. However, it feels like that is not achieved by this workflow and at some point it has definitely outweighed the cost of maintaining it.

We can readd something if we find a better way to implement things which is notably less jank and gets less complaints.

@workingjubilee
Copy link
Member Author

And anything which depends on an implementation in YamlScript is right the fuck out.

@tgross35
Copy link
Contributor

tgross35 commented May 7, 2025

If the goal is to delete the current implementation and add back something different later, all of .github/actions can get wiped too.

+1 to getting rid of this, probably should have been a python script if something more json-capable than bash was needed. It's impossible to repro this job locally as-is.

Remove the "binary size check" workflow because it is often broken and
now and then makes me sit through a discussion about its security needs.
These discussions always make me feel very silly.

I originally hoped this CI job would be a useful part of assuring that
we would not accidentally bloat binaries when backtrace merges into std.
However, it feels like that is not achieved by this workflow and at some
point it has definitely outweighed the cost of maintaining it.

We can readd something if we find a better way to implement things which
is notably less jank and gets less complaints.
@workingjubilee workingjubilee force-pushed the remove-binary-size-check-workflow branch from 55e8a40 to 7718dae Compare May 7, 2025 20:40
@workingjubilee
Copy link
Member Author

Good catch. Thank you.

@tgross35
Copy link
Contributor

tgross35 commented May 7, 2025

If you do wind up reimplementing, I have some logic for finding the exact PR diff that you could probably steal https://github.com/rust-lang/compiler-builtins/blob/a4c748f72a1dce652cc3e41c3a8425731bd1519a/ci/ci-util.py#L124-L179 (used there for running the multi-hour tests only if relevant files changed) and for comparing to a baseline that was saved as an artifact https://github.com/rust-lang/compiler-builtins/blob/a4c748f72a1dce652cc3e41c3a8425731bd1519a/ci/ci-util.py#L271-L343 (the icount benchmarks use the latest run on master as a baseline rather than running twice across the diff). I'm not "very satisfied" with that solution because it seems like those things should be way easier, but I would say "somewhat satisfied" since it's been reasonably problem-free for a while.

@workingjubilee workingjubilee merged commit 57c7529 into rust-lang:master May 8, 2025
40 of 41 checks passed
@workingjubilee workingjubilee deleted the remove-binary-size-check-workflow branch May 8, 2025 00:40
@detly
Copy link
Contributor

detly commented Jun 5, 2025

Just popping in to say that even though it didn't last and was annoying for you while it did, I really appreciate that you did this in the first place. It was a cool experiment, I learnt a lot about this library and Rust internals from all of the discussions around it, and it was just frankly nice to know that factors that are important to embedded developers continue to be important to people who work on Rust. I just don't want you to think that your initial effort on this was a waste of time or unappreciated because you've had to turf it now. It happens.

I honestly think that the best way to make concessions for backtrace's impact on code size is with something as "simple" as an equivalent to panic=abort in Cargo.toml (eg. backtrace=none). So, measuring small increments in it on every PR is probably not part of that feedback loop anyway.

("Simple" is in quotes because I know it's not, I've looked into it, it's extremely non-trivial and there's even an RFC for it somewhere that languishes because of the difficulty. Simple as in, interface simple, not implementation simple.)

It's currently achievable by DIY building std, which we're going to have to do anyway since our platform was demoted to tier 3, so it's at least doable.

Thanks again.

stormshield-ebzh pushed a commit to stormshield-ebzh/fork-backtrace-rs that referenced this pull request Jun 5, 2025
Remove the "binary size check" workflow because it is often broken and
now and then makes me sit through a discussion about its security needs.
These discussions always make me feel very silly.

I originally hoped this CI job would be a useful part of assuring that
we would not accidentally bloat binaries when backtrace merges into std.
However, it feels like that is not achieved by this workflow and at some
point it has definitely outweighed the cost of maintaining it.

We can readd something if we find a better way to implement things which
is notably less jank and gets less complaints.
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.

3 participants