Skip to content

Introduce PRODUCTION flag to enable/disable stripping binaries after build #2057

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

egypcio
Copy link

@egypcio egypcio commented May 8, 2025

by merging this PR we would:

  • be able to set PRODUCTION to strip the exporter and validator binaries;
  • avoid adding DWARF debug information to newer releases;
  • reduce the size/footprint of the built binaries;
  • still be able to find/follow debug symbols using Delve, should necessary;
  • promote the usage of less energy/power consumption.

example using PRODUCTION to enable/disable stripped build for the exporter locally:

$ PRODUCTION=1 make _build_local
$ file _output/bin/linux_amd64/kepler 
_output/bin/linux_amd64/kepler: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=45a457a614ec12d4f18e983f7f28c9bfd3dfc9f3, for GNU/Linux 3.2.0, stripped
$ ls -laish _output/bin/linux_amd64/kepler
27662536 37M -rwxrwxr-x 1 egypcio egypcio 37M May  8 11:42 _output/bin/linux_amd64/kepler
$ make _build_local
$ file _output/bin/linux_amd64/kepler 
_output/bin/linux_amd64/kepler: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=ee248b3f1e691fa128d33baa5f91eaa82286a12b, for GNU/Linux 3.2.0, with debug_info, not stripped
$ ls -laish _output/bin/linux_amd64/kepler
27662536 53M -rwxrwxr-x 1 egypcio egypcio 53M May  8 11:46 _output/bin/linux_amd64/kepler

impact on the binary created after building the validator:

8.4M -rwxrwxr-x 1 egypcio egypcio 8.4M May  8 11:49 _output/bin/validator-debug
5.7M -rwxrwxr-x 1 egypcio egypcio 5.7M May  8 11:49 _output/bin/validator

Host: Linux version 6.11.0-25-generic (buildd@lcy02-amd64-027) (x86_64-linux-gnu-gcc-13 (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0, GNU ld (GNU Binutils for Ubuntu) 2.42), a.k.a. Ubuntu 24.04.2 LTS (Noble Numbat)

@sthaha
Copy link
Collaborator

sthaha commented May 9, 2025

@egypcio Thank you for the patch but TBH I don't feel strongly about potential saving ..

As mentioned in the article you linked ...

Symbol information takes up a lot of storage space, so when there are space constraints, it makes sense to work with stripped binaries.

Where exactly are you hitting space constraint ?

Also if we are to enable this, we would want make build PRODUCTION=1 so that the default build contains debug information.

Could you also please squash the commits into a single one?

@sthaha sthaha requested review from sthaha and Copilot May 9, 2025 01:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a DEBUG flag for controlling whether binaries are stripped after being built. The changes include adding a new DEBUG flag and conditional Makefile logic to include or exclude the stripping linker flags for both the exporter and validator binaries.

@egypcio egypcio force-pushed the main-strip-build branch from 126d634 to e72eca1 Compare May 9, 2025 08:20
@egypcio egypcio changed the title Introduce DEBUG flag to enable/disable stripping binaries after build Introduce PRODUCTION flag to enable/disable stripping binaries after build May 9, 2025
@egypcio
Copy link
Author

egypcio commented May 9, 2025

@egypcio Thank you for the patch but TBH I don't feel strongly about potential saving ..

As mentioned in the article you linked ...

Symbol information takes up a lot of storage space, so when there are space constraints, it makes sense to work with stripped binaries.

Where exactly are you hitting space constraint ?

Also if we are to enable this, we would want make build PRODUCTION=1 so that the default build contains debug information.

Could you also please squash the commits into a single one?

olá @sthaha,

many thanks for the input/feedback. much appreciated!

I've applied the suggested changes and squashed the commits into a single one.

now, on the space constraint: I wouldn't take that specific cut from that blog post so literal, as per serving a smaller binary without debug symbols would reduce I/O and memory consumption (now: consider a large scale deployment, or multiple pipelines running to perform CI/CD; consider multiple deployments world wide).

one of the motivations for the proposed change in the PR comes from this article:

@sthaha
Copy link
Collaborator

sthaha commented May 10, 2025

@egypcio , could you please elaborate what exactly in the blog is the motivation that you are referring to ..

one of the motivations for the proposed change in the PR comes from this article:
https://www.cncf.io/blog/2023/10/11/exploring-keplers-potentials-unveiling-cloud-application-power-consumption/

@egypcio
Copy link
Author

egypcio commented May 12, 2025

@egypcio , could you please elaborate what exactly in the blog is the motivation that you are referring to ..

one of the motivations for the proposed change in the PR comes from this article:
https://www.cncf.io/blog/2023/10/11/exploring-keplers-potentials-unveiling-cloud-application-power-consumption/

I would prefer not to; appreciate the curiosity/interest though.

PS: gonna apply your latest suggestions later and rebase/squash the commit again to update the PR.

    * chore(Makefile): add DEBUG flag to handle stripped builds
    * chore(Makefile): enable stripped build for the exporter
    * chore(Makefile): enable stripped builds for the validator
    * chore(Makefile): write a few notes about new DEBUG flag
    * fix(Makefile): properly set go_ldflags_strip_args for DEBUG
    * chore(Makefile): rename DEBUG to PRODUCTION and adjust its logic
    * chore(Makefile): change LDFLAGS directly after conditional check

Signed-off-by: Vinícius Zavam <[email protected]>
@egypcio egypcio force-pushed the main-strip-build branch from e72eca1 to 902bb7c Compare May 12, 2025 09:37
Copy link

codecov bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.53%. Comparing base (639982d) to head (902bb7c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2057   +/-   ##
=======================================
  Coverage   51.53%   51.53%           
=======================================
  Files          39       39           
  Lines        3522     3522           
=======================================
  Hits         1815     1815           
  Misses       1555     1555           
  Partials      152      152           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

2 participants