-
Notifications
You must be signed in to change notification settings - Fork 10
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
[profiling] Ensure shared library name has no soname set. #408
Conversation
We talked about this. We are OK with removing the so name as we are not using it today. Would it be possible to just drop this within a cargo build script as part of the effort described in #353 |
can it be part of the rust flags (like linker flags)? I think @gleocadie did work that is similar, we should put settings in a similar place. |
From what I chatted with @AlexJF, he did not find such an option, hence why he went with the "fix-after-the-fact" approach. |
I see some failures in our build pipeline
We'll need a PR on that side to make sure our images have patchelf. |
PR to add |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #408 +/- ##
=======================================
Coverage 66.14% 66.14%
=======================================
Files 187 187
Lines 23166 23166
=======================================
Hits 15324 15324
Misses 7842 7842
|
CI is now green for this one! @r1viollet thoughts on merging? Or do you want to hold on until we can see if there's a cleaner way to tell the rust compiler to not emit the soname at all? |
Actually CI isn't green lol, either I was looking at the wrong tab, or some of the jobs hadn't finished when I looked at it. Pardon the temporary insanity. Will look into the missing issue -- I think I didn't bump the CI image version thingy correctly. |
I'm moving this back to draft until we can pick it up again (sorry Alex! ;_;) |
aef7a37
to
1487913
Compare
BenchmarksComparisonBenchmark execution time: 2024-09-11 14:37:28 Comparing candidate commit 1487913 in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 45 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
Closing as superceded by #628 |
What does this PR do?
Ensure the libdatadog profiling shared library has no SONAME set in
.so
shared libraries.Motivation
In some environments, the libdatadog profiling shared library will include a SONAME and it will point to the library name previous to the rename in:
libdatadog/build-profiling-ffi.sh
Line 138 in f5c7e88
The existence of this SONAME makes it impossible to load libdatadog at runtime because the loading will be done based on the SONAME, not the actual library name:
Additional Notes
We haven't been able to fully pinpoint what leads to different SONAME handling between different environments (e.g. my Ubuntu 24 LTS VM always includes them but not Ivo's Ubuntu 22 laptop) but we found the following places that show how "finicky" this can be:
How to test the change?
Describe here in detail how the change can be validated.
For Reviewers
@DataDog/security-design-and-guidance
.