new(build): add RelWithDebInfo target #3440
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shane-lawrence The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
poiana
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
|
I've only tried to remove debug symbols from the original binary, but I'm seeing a substantial change in file size compared to the release binary. Building with bundled deps and no specified CMAKE_BUILD_TYPE (defaults to Release), I get a userspace/falco/falco binary that's 398MB. Building with CMAKE_BUILD_TYPE=RelWithDebInfo, I get a userspace/falco/falco.debug file that's 222MB and a userspace/falco/falco binary that's 90% smaller at only 26MB. The binary seems to work the same though. |
shane-lawrence
force-pushed
the
sl/add-relwithdebinfo
branch
from
074aaab to
09418cb
Compare
|
Hi! Thanks for this PR! I really like the idea and i think we could start shipping (perhaps just as github release artifacts) the debug symbols files, wdyt?
Mmmh the 0.39.2-x86_64 Falco released binary is: thus we are pretty near. |
|
Yes that would be great. Just need the symbols to come from the same build as the running binary to use them. |
Yep exactly! |
|
Oh, I closed it by mistake. |
|
That's a classic 🤣 |
|
Can you remove last commit? |
FedeDP
changed the title
|
What i see is:
This is something that has been on my mind for so long, but never scratched my head around it :) Thank you so much!
|
|
@FedeDP: The provided milestone is not valid for this repository. Milestones in this repository: [ Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/milestone 0.40.0 |
| @@ -194,6 +194,24 @@ if(MUSL_OPTIMIZED_BUILD AND CMAKE_BUILD_TYPE STREQUAL "release") | |||
| ) | |||
| endif() | |||
|
|
|||
| if(CMAKE_BUILD_TYPE STREQUAL "relwithdebinfo" AND NOT WIN32) | |||
There was a problem hiding this comment.
So, what happens on win32 here?
There was a problem hiding this comment.
We'll have to figure out how to do the equivalent of objcopy in Windows if we want to separate out the debug info properly. I'm only familiar with one way of doing so, and it only works with Visual Studio so I'd like to come up with a more generic method.
There was a problem hiding this comment.
Thanks for the insight! For now it is not a big deal since we don't ship windows artifacts (ie: Falco is not officially distributed for windows; we only want to make sure that we build there), perhaps we can leave a TODO comment there?
…ug symbols file. Signed-off-by: Shane Lawrence <[email protected]>
shane-lawrence
force-pushed
the
sl/add-relwithdebinfo
branch
from
dbc2341 to
e7f7506
Compare
|
I had pushed the empty commit because one of the action runners timed out waiting for the job to start but I think it was a fluke. I've removed it now and rebased. I think that's a good idea to publish the artifact as part of the release workflow. |
|
Yes we are having issues with the arm64 cncf provided runners, that's not your fault :) |
Signed-off-by: Shane Lawrence <[email protected]>
shane-lawrence
force-pushed
the
sl/add-relwithdebinfo
branch
from
0237269 to
f8d5aa9
Compare
|
I added a TODO comment for win32 and opened an issue to track it #3445. Is there anything else missing before we can merge this? |
|
Nope, we just need to fix arm64 runners :) |
Signed-off-by: Shane Lawrence [email protected]
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area build
What this PR does / why we need it:
Debug binaries can behave differently from release binaries, which can make it hard to reproduce Release bugs in a Debug environment for certain edge cases. However, the release target doesn't produce debug symbols so it is difficult to trace certain kinds of bugs.
This PR adds the cmake-standard RelWithDebInfo target so we can create a binary that behaves exactly the same as the release target but has debug symbols available in a separate file.
Which issue(s) this PR fixes:
Fixes #2268.
Special notes for your reviewer:
This PR requires additional testing to ensure that the binary really is the same as the release binary. This first attempt might also need additional changes to conform to the existing style and good coding practices for this project.
Does this PR introduce a user-facing change?:
NO