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

[chore] Ability to provide custom ld and gc flags #11996

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

Dainerx
Copy link
Contributor

@Dainerx Dainerx commented Dec 30, 2024

Description

The primary purpose of this PR is to provide greater flexibility in how OTEL binaries are built, enabling the inclusion of debugging symbols when needed, without always stripping them by default.

Currently, debugging symbols are only retained when debug_compilation=true. However, this approach also disables all compiler inlining and optimizations (gcflags=all=-N -l) to ensure an exact match between written and executed code, resulting in a significant increase in CPU consumption. There are scenarios where we want binaries with debugging symbols and DWARF information while still allowing the compiler to optimize and inline. This PR addresses that need by introducing configurable GCFlags.

ocb --ldflags="" --gcflags="" --config=builder-config.yaml

Link to tracking issue

Fixes #58

Testing

Manual

Override LDflags:

image

Override both

image

Documentation

README file updated.

--

Backward compatibility concerns:

  • As of today, passing cfg.LDFlags will append to LD flags that are by default to -s -w.

Questions:

  • Should we deprecate DebugCompilation property?

@Dainerx Dainerx requested a review from a team as a code owner December 30, 2024 17:52
@Dainerx Dainerx requested a review from jpkrohling December 30, 2024 17:52
@Dainerx Dainerx force-pushed the builder/ld-gc-flags branch from 9889e99 to a6d8bb0 Compare December 31, 2024 14:37
@Dainerx Dainerx changed the title [#58] Ability to provide custom ld and gc flags [chore] Ability to provide custom ld and gc flags Dec 31, 2024
@Dainerx Dainerx force-pushed the builder/ld-gc-flags branch from c3b79eb to ee35153 Compare December 31, 2024 16:18
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (52d1414) to head (e2ee644).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11996   +/-   ##
=======================================
  Coverage   91.71%   91.72%           
=======================================
  Files         463      463           
  Lines       24803    24821   +18     
=======================================
+ Hits        22749    22767   +18     
  Misses       1672     1672           
  Partials      382      382           

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

@@ -71,7 +74,7 @@ type Distribution struct {
OutputPath string `mapstructure:"output_path"`
Version string `mapstructure:"version"`
BuildTags string `mapstructure:"build_tags"`
DebugCompilation bool `mapstructure:"debug_compilation"`
DebugCompilation bool `mapstructure:"debug_compilation"` // TODO depercate?
Copy link
Member

Choose a reason for hiding this comment

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

Instead of TODOs in code, please file an issue :)

Suggested change
DebugCompilation bool `mapstructure:"debug_compilation"` // TODO depercate?
DebugCompilation bool `mapstructure:"debug_compilation"`

cmd/builder/README.md Outdated Show resolved Hide resolved
@Dainerx Dainerx force-pushed the builder/ld-gc-flags branch from ffc36a7 to e2ee644 Compare January 20, 2025 17:55
@mx-psi mx-psi enabled auto-merge January 21, 2025 10:34
@mx-psi mx-psi added this pull request to the merge queue Jan 21, 2025
auto-merge was automatically disabled January 21, 2025 10:47

Pull Request is not mergeable

Merged via the queue into open-telemetry:main with commit 24da249 Jan 21, 2025
52 checks passed
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.

Enabling debug symbols
2 participants