-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add an option to keep native debug symbols #39203
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
51f12b5 to
e935340
Compare
e935340 to
063a014
Compare
|
Anyone willing to review this? |
|
@jkoritzinsky maybe you'd like to review this change--cmake conditions and new build script arg. |
dagood
left a comment
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.
Seems very reasonable. One suggestion and one comment about the context--I'm not particularly familiar with this area.
eng/build.sh
Outdated
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.
The help message doesn't mention it takes an argument, but it seems to take true/false and reject nothingness. 😕 But it looks like this is also the case with other settings, so this PR appears to be consistent to me.
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.
Should I take a stab at fixing up the other settings too?
063a014 to
417e787
Compare
tmds
left a comment
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, thanks!
|
@janvorli hey, can you help me find some reviewers for this PR? Thanks in advance! |
janvorli
left a comment
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.
I would like to ask you to change few things.
eng/build.sh
Outdated
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.
I would prefer creating a msbuild property for this instead of passing it in the cmakeargs. We do that e.g. for the -portablebuild option, so I'd like to use unified way for all of them. You would then add this option to eng/build-commons.sh, which is used to parse the common options for coreclr, libraries and installer .sh scripts and add the cmake arg there. You'll need to update src/coreclr/runtime.proj, src/installer/corehost/corehost.proj and src/libraries/Native/build-native.proj to convert the msbuild property to the -keepsymbols option passed to the respective .sh scripts.
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.
Does the updated patch match what you had in mind?
eng/build.sh
Outdated
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.
A nit - based on various cmake args settings in coreclr, I'd prefer this to be named CLR_CMAKE_KEEP_SYMBOLS to keep things unified.
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.
Done!
417e787 to
f23aca6
Compare
janvorli
left a comment
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, thank you!
|
@janvorli Does the "keepSymbols" name appear clear enough to you? Should I look at disambiguating it to "keepNativeSymbols" or "keepUnmanagedSymbols" or something similar? |
|
Hmm, |
f23aca6 to
2fcdb9d
Compare
This is a backport of dotnet/runtime#39203
When packaging .NET for Linux distributions, the package builders
generally use a different workflow for shipping symbols to users:
1. The package maintainer builds code with the debug flags (such as
`-g`) to generate full native debug info and symbols.
2. Nothing is stripped from build by the package maintainer.
3. The build system (`rpmbuild`, `debuild`) removes the debug
info (or debug symbols) from the code and creates separate
`-debuginfo` or `-debug` packages that contain just the debug
symbols.
4. These debug packages are then distributed along with the normal
packages using the normal Linux distribution mechanisms. This lets
users install the exact set of debug symbols matching their other
package.
To support this workflow in dotnet/runtime, we need to add optional
support for not stripping debug symbols. I used it has follows:
CFLAGS=-g CXXFLAGS=-g ./build.sh --keepnativesymbols true
After this build, the built binaries include all debug symbols.
I can then rely on the distro package build system to identify, strip,
package and ship the debug info/symbols separately.
See dotnet#3781 and
dotnet/source-build#267 for more details on
the background and motivation.
For some related fixes, see:
- dotnet/coreclr#3445
- dotnet/corefx#24979
2fcdb9d to
2ff2c5b
Compare
|
The CI failures look unrelated to my changes (json tests and mono build issues) but I have rebased this branch onto master to see if that fixes the CI failures. |
|
@janvorli Does this still look okay? |
janvorli
left a comment
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 with the latest updates.
|
I'll still re-run the runtime tests to see the |
When packaging .NET for Linux distributions, the package builders generally use a different workflow for shipping symbols to users:
The package maintainer builds code with the debug flags (such as
-g) to generate full native debug info and symbols.Nothing is stripped from build by the package maintainer.
The build system (
rpmbuild,debuild) removes the debug info (or debug symbols) from the code and creates separate-debuginfoor-debugpackages that contain just the debug symbols.These debug packages are then distributed along with the normal packages using the normal Linux distribution mechanisms. This letsusers install the exact set of debug symbols matching their other package.
To support this workflow in dotnet/runtime, we need to add optional support for not stripping debug symbols. I used it has follows:
After this build, the built binaries include all debug symbols.
I can then rely on the distro package build system to identify, strip, package and ship the debug info/symbols separately.
See #3781 and dotnet/source-build#267 for more details on the background and motivation.
For some related fixes, see: