Repository navigation
Update cURL pin - #1083
Update cURL pin#1083msarahan wants to merge 9 commits into
Conversation
|
Libcurl >= 8.21 carries a performance bug that unexpectedly disables caching for the CA bundle file and causes it to be repeatedly re-parsed during TLS. Microbenchmark shows the TLS time for stead-state S3 read is: I will need to further evaluate the end-to-end performance impact using Polars and Presto GPU TPC-H. If it is trivial, then we can upgrade. #1080 tried to work around this performance bug, but caused another severe regression for Presto GPU TPC-H benchmark, so I reverted the change in #1081. |
|
I've made several tests. The TL;DR of my findings are:
Microbenchmark
PolarsPresto GPUInterestingly, in Presto GPU, KvikIO does not use its own libcurl library, but the one bundled in Velox, which currently pins to 8.4.0, and is not affected by the regression issue. |
|
So I think we should either hold off the upgrade until the curl PR is merged into a newer version of curl, or we proceed with the upgrade and advise users to upgrade their OpenSSL if they are using this common backend. |
|
curl v2.18+ requires openssl 3.x. We have to find a source for that. RHEL 8 does not seem to provide static libraries here - only dynamic builds. This PR obtains the conda-forge openssl-static package and links it statically into the wheel builds. Statically linking OpenSSL is not good in general, because if there's a security issue, you must rebuild packages to fix the issue. Dynamic linking would allow drop-in of a new openssl shared library, without touching kvikio or other consumers. This said, static linking is already what we were doing with the system openssl, so at least we're not any worse. I am verifying that there are no compatibility concerns as far as libgcc, ABI, or otherwise. |
Avoid the curl CA-cache regression in the bundled source dependency while preserving conda's older build floor and run_exports. Require the conda host shared library so CPM cannot silently replace it with a source build. Add the required Bandit and flake8-bugbear pre-commit checks.
The build.sh parser requires literal quotes around --cmake-args. Shell quoting alone removed them, so the conda C++ job rejected the options before invoking CMake. Preserve the inner quotes and exercise the actual recipe command with normal and long conda prefixes, checking that the supplied shared curl options reach CMake.
Use checksum-pinned conda-forge archives and matching headers without installing conda, so curl 8.23 can build on the existing manylinux images. Initialize the private OpenSSL copy through its supported API before curl, preserving OPENSSL_CONF overrides and failing on invalid selected configuration. Keep installed curl handling unchanged for conda builds. Add GPU-independent configuration and TLS handshake regressions to wheel CI, and include the OpenSSL license in wheel installs.
The TLS policy test invokes the build image's openssl command, whose compiled default configuration path may not exist. Supply a minimal request configuration when generating the local server certificate so that missing image configuration cannot prevent the handshake regression from running. Let pytest capture stderr directly so future certificate failures include their diagnostics.
Replace conda-forge binary archives with the checksum-pinned upstream OpenSSL 3.6.5 release. Build private PIC static libraries before curl runs its configuration checks, preserving assembly optimizations and host configuration handling. Keep generated files outside CPM's shared source cache and reuse completed builds until the source version, compiler, or configuration changes. Ensure wheel CI has the Perl core modules required by OpenSSL 3, and extend dependency-selection tests to prove that conda builds bypass the source build even when static OpenSSL is enabled. Validate both architectures and the production curl CPM path, including HTTPS, S3, certificate rejection, and license installation.
Main now determines the CUDA version before processing build targets. Keep the conda argument regression independent of a CUDA installation by supplying the version explicitly while continuing to stub CMake.
f28c2b6 to
32de9de
Compare
Update cURL pin to the most recent release, 8.23.0.
This was done to avoid security issues associated with versions <8.22.
8.22 is not an option, because it has a performance bug that is described in comments below. 8.23.0 is supposed to fix the performance bug and keep the security issue fixes.
This is complicated by the fact that cURL >=8.18 requires openssl 3.x, which is not in our default build environment. Static libraries for openssl 3.x do not seem to be available for RockyLinux 8. This PR changes openssl to be built from source for wheels and statically linked. For conda, it changes to dynamic linkage for openssl, rather than static linkage, which is better hygiene in the conda ecosystem, and allows security fixes for openssl to be deployed without rebuilding kvikio.