Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unify some cmake function definitions #33716
Unify some cmake function definitions #33716
Changes from all commits
27eef50
5324f02
4fc15c2
2264fa0
f193079
ee03fc0
b861b05
1b2b5c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It is weird that we always strip symbols for libraries and strip them conditionally for coreclr. Since you are touching this code, I would take this opportunity to make the stripping unconditional. It should be fine (debugging should not be affected, I believe neither lldb nor gdb cares whether the symbols are in the .so files or in separate files) and it would also fix #32957 on OSX where debugging a core dump from checked / debug build is currently broken due to missing symbols.
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 we start by removing the top-level option:
runtime/eng/native/build-commons.sh
Line 190 in 8db7154
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.
II would do it in 3 steps in this order:
-stripsymbols
option as part of this PR-stripsymbols
to the build scripts in the CI / build lab scripts in a follow up PR-stripsymbols
option support from thebuild-commons.sh
The point is that we don't want to have intermediate period where we would not be stripping the symbols and we also cannot pass in the -stripsymbols from the CI / build lab scripts after we remove support for that option from the
build-commons.sh
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.
Part 1 is pushed f193079. I will make followup PRs for 2 and 3. Thanks!