bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.17k stars 392 forks source link

Vendor libdebuginfod into our wheels #592

Closed godlygeek closed 4 months ago

godlygeek commented 4 months ago

This allows us to guarantee that libdebuginfod is always available for every Memray install. Previously we were dependent upon using dlopen to find a copy of it already installed in the runtime environment.

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.91%. Comparing base (41248ed) to head (88aedf1). Report is 64 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #592 +/- ## ========================================== + Coverage 92.55% 92.91% +0.36% ========================================== Files 91 92 +1 Lines 11304 11236 -68 Branches 1581 2058 +477 ========================================== - Hits 10462 10440 -22 + Misses 837 796 -41 + Partials 5 0 -5 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/592/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.91% <ø> (+6.97%)` | :arrow_up: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/592/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.91% <ø> (-2.81%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 4 months ago

@pablogsal This probably needs a bit more testing, but I'd like your first impressions here before I sink more effort into it.

  1. Are you satisfied with the increase in wheel size? I don't find it concerning, but you raised it as a possible concern.
  2. Should we invest effort into getting libzstd built on manylinux2010? libdw wants it in case a library has zstd-compressed DWARF, but I'm not sure how common that case is or whether it's worth the effort to build it on the old distro.
  3. Will vendoring libcurl cause us issues with SSL certificates? We may need some research to ensure that company-private self-signed certs for private debuginfod servers would work with a vendored libdebuginfod...
  4. And of course, vendoring this increases CI time, so that's something we'll need to consider.
pablogsal commented 4 months ago
  • Are you satisfied with the increase in wheel size? I don't find it concerning, but you raised it as a possible concern.

I am, I don't think this is a problem.

  • Should we invest effort into getting libzstd built on manylinux2010? libdw wants it in case a library has zstd-compressed DWARF, but I'm not sure how common that case is or whether it's worth the effort to build it on the old distro.

Unfortunately it's quite common in distributions like Arch, so this it's a must.

  • Will vendoring libcurl cause us issues with SSL certificates? We may need some research to ensure that company-private self-signed certs for private debuginfod servers would work with a vendored libdebuginfod...

Ugh, that needs to be discussed because this can actually raise a lot of issues for consumers of the wheels :(

  • And of course, vendoring this increases CI time, so that's something we'll need to consider.

That's fine as long as this happens only on release

godlygeek commented 4 months ago

Will vendoring libcurl cause us issues with SSL certificates? We may need some research to ensure that company-private self-signed certs for private debuginfod servers would work with a vendored libdebuginfod...

Ugh, that needs to be discussed because this can actually raise a lot of issues for consumers of the wheels :(

We discussed offline and decided to punt on this until we get an actual user report of a problem it causes. In theory, it should be possible to work around virtually any issue that pops up using the CURL_CA_BUNDLE environment variable. If it turns out we need more than that, we're going to wait until we have a concrete problem report and a concrete problem to solve.

And of course, vendoring this increases CI time, so that's something we'll need to consider.

That's fine as long as this happens only on release

It doesn't, but it is amortized across all the different versions' builds, and it doesn't affect the slowest job, so it doesn't affect the total wall time required to run the CI pipeline (currently, building macOS x86-64 wheels is the slowest thing, and we don't have to deal with libdebuginfod for macOS).

Should we invest effort into getting libzstd built on manylinux2010? libdw wants it in case a library has zstd-compressed DWARF, but I'm not sure how common that case is or whether it's worth the effort to build it on the old distro.

Unfortunately it's quite common in distributions like Arch, so this it's a must.

It proved to be only slightly annoying to build this as well, so I've added it, too.

The diff against what you had previously reviewed is:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 26a55bfb..4484668b 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -86,7 +86,7 @@ jobs:
           curl https://sourceware.org/elfutils/ftp/$VERS/elfutils-$VERS.tar.bz2 > ./elfutils.tar.bz2
           tar -xf elfutils.tar.bz2
           cd elfutils-$VERS
-          CFLAGS='-Wno-error -DFNM_EXTMATCH=0 -g -O2' CXXFLAGS='-Wno-error -DFNM_EXTMATCH=0 -g -O2' ./configure --enable-libdebuginfod --disable-debuginfod --disable-nls --with-zstd
+          CFLAGS='-Wno-error -DFNM_EXTMATCH=0 -g -O3' CXXFLAGS='-Wno-error -DFNM_EXTMATCH=0 -g -O3' ./configure --enable-libdebuginfod --disable-debuginfod --disable-nls --with-zstd
           make install
       - name: Clone lcov repository
         run: |
diff --git a/pyproject.toml b/pyproject.toml
index 49c7a323..e8f6c3cf 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -77,20 +77,29 @@ before-all = [
   "yum install -y openssl-devel",
   "cd /",
   "CURL_VERS=8.7.1",
-  "curl https://curl.se/download/curl-$CURL_VERS.tar.bz2 >./curl.tar.bz2",
-  "tar xf ./curl.tar.bz2",
+  "curl -LO https://curl.se/download/curl-$CURL_VERS.tar.bz2",
+  "tar xf ./curl-$CURL_VERS.tar.bz2",
   "cd curl-$CURL_VERS",
   "./configure --with-openssl",
   "make install",

+  # Build the latest zstd from source
+  "yum install -y lz4-devel xz-devel",
+  "cd /",
+  "ZSTD_VERS=1.5.6",
+  "/usr/bin/curl -LO https://github.com/facebook/zstd/releases/download/v1.5.6/zstd-$ZSTD_VERS.tar.gz",
+  "tar xf ./zstd-$ZSTD_VERS.tar.gz",
+  "cd zstd-$ZSTD_VERS",
+  "V=1 LDLIBS=-lrt make install",
+
   # Build the latest elfutils from source.
-  "yum install -y curl-devel lz4-devel",
+  "yum install -y lz4-devel",
   "cd /",
   "VERS=0.191",
-  "curl https://sourceware.org/elfutils/ftp/$VERS/elfutils-$VERS.tar.bz2 > ./elfutils.tar.bz2",
+  "/usr/bin/curl https://sourceware.org/elfutils/ftp/$VERS/elfutils-$VERS.tar.bz2 > ./elfutils.tar.bz2",
   "tar -xf elfutils.tar.bz2",
   "cd elfutils-$VERS",
-  "CFLAGS='-Wno-error -g -O3' CXXFLAGS='-Wno-error -g -O3' LDFLAGS=-lrt ./configure --enable-libdebuginfod --disable-debuginfod --disable-nls",
+  "CFLAGS='-Wno-error -g -O3' CXXFLAGS='-Wno-error -g -O3' LDFLAGS=-lrt ./configure --enable-libdebuginfod --disable-debuginfod --disable-nls --with-zstd",
   "make install",

   # Install Memray's other build and test dependencies