Skip to content
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

Vendor libdebuginfod into our wheels #592

Merged
merged 3 commits into from
May 30, 2024

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented May 6, 2024

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
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
cpp 92.91% <ø> (+6.97%) ⬆️
python_and_cython 92.91% <ø> (-2.81%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@godlygeek godlygeek force-pushed the vendor_libdebuginfod branch 2 times, most recently from c2ef910 to 0aac9d4 Compare May 6, 2024 23:19
@godlygeek
Copy link
Contributor Author

@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
Copy link
Member

  • 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 godlygeek marked this pull request as ready for review May 9, 2024 19:04
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
This will allow `auditwheel repair` to vendor libdebuginfod as well,
allowing us to guarantee that it's 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.

Signed-off-by: Matt Wozniski <[email protected]>
@godlygeek godlygeek force-pushed the vendor_libdebuginfod branch 4 times, most recently from 97bc8ae to 39db4de Compare May 29, 2024 23:50
Download and build the latest `elfutils` so that `auditwheel` can vendor
the latest `libdebuginfod` into our wheels.

Signed-off-by: Matt Wozniski <[email protected]>
Now that we've introduced a build-time dependency on `libdebuginfod` on
Linux, we need to ensure it's installed in every environment that we
perform an install in.

On Alpine in particular, this means building it from source, since
Alpine doesn't package it.

Signed-off-by: Matt Wozniski <[email protected]>
@godlygeek
Copy link
Contributor Author

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

@godlygeek godlygeek enabled auto-merge (rebase) May 30, 2024 00:29
@godlygeek godlygeek merged commit 475c538 into bloomberg:main May 30, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants