build: add libcurl to contrib build system#173
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 94-95: The help string for the BUILD_TYPE CMake option is missing
entries present in VALID_BUILD_TYPES; update the BUILD_TYPE declaration so its
help text matches VALID_BUILD_TYPES by adding "EIGEN" and "HDF5" (i.e., change
the set( BUILD_TYPE LIST CACHE STRING ... ) value to include EIGEN and HDF5)
ensuring the BUILD_TYPE and VALID_BUILD_TYPES lists remain consistent; verify
the tokens match exactly
("ALL","LIBSVM","XERCESC","BOOST","COINOR","BZIP2","ZLIB","GLPK","EIGEN","KISSFFT","HDF5","OPENMP","CURL").
In `@dockerfiles/pyopenms/manylinux/Dockerfile`:
- Around line 10-12: The three separate RUN yum install lines leave yum metadata
in image layers; combine the package installs into a single RUN and clean caches
in the same step (e.g., run the installs for wget, xz, qt6-qtbase-devel,
qt6-qtsvg-devel, libcurl-devel, libtool, cmake3 in one RUN and then run yum
clean all && rm -rf /var/cache/yum/*) so the yum metadata is removed in the same
layer that performed the install; update the Dockerfile lines with a single RUN
that performs the installs followed by the cache cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35a70ab6-4842-4bbd-a4b6-e71ce3fc85a6
📒 Files selected for processing (5)
CMakeLists.txtdockerfiles/Dockerfiledockerfiles/pyopenms/manylinux/Dockerfilelibraries.cmake/curl.cmakemacros.cmake
CMakeLists.txt
Outdated
| set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,KISSFFT,OPENMP,CURL") | ||
| set( VALID_BUILD_TYPES "ALL" "LIBSVM" "XERCESC" "BOOST" "COINOR" "BZIP2" "ZLIB" "GLPK" "EIGEN" "KISSFFT" "HDF5" "OPENMP" "CURL") |
There was a problem hiding this comment.
Keep BUILD_TYPE help text consistent with VALID_BUILD_TYPES.
Line 94 omits EIGEN and HDF5, but Line 95 allows both. This can confuse users invoking -DBUILD_TYPE=....
Suggested fix
-set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,KISSFFT,OPENMP,CURL")
+set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,EIGEN,KISSFFT,HDF5,OPENMP,CURL")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,KISSFFT,OPENMP,CURL") | |
| set( VALID_BUILD_TYPES "ALL" "LIBSVM" "XERCESC" "BOOST" "COINOR" "BZIP2" "ZLIB" "GLPK" "EIGEN" "KISSFFT" "HDF5" "OPENMP" "CURL") | |
| set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,EIGEN,KISSFFT,HDF5,OPENMP,CURL") | |
| set( VALID_BUILD_TYPES "ALL" "LIBSVM" "XERCESC" "BOOST" "COINOR" "BZIP2" "ZLIB" "GLPK" "EIGEN" "KISSFFT" "HDF5" "OPENMP" "CURL") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 94 - 95, The help string for the BUILD_TYPE
CMake option is missing entries present in VALID_BUILD_TYPES; update the
BUILD_TYPE declaration so its help text matches VALID_BUILD_TYPES by adding
"EIGEN" and "HDF5" (i.e., change the set( BUILD_TYPE LIST CACHE STRING ... )
value to include EIGEN and HDF5) ensuring the BUILD_TYPE and VALID_BUILD_TYPES
lists remain consistent; verify the tokens match exactly
("ALL","LIBSVM","XERCESC","BOOST","COINOR","BZIP2","ZLIB","GLPK","EIGEN","KISSFFT","HDF5","OPENMP","CURL").
| RUN yum install -y wget | ||
| RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel | ||
| RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel | ||
| RUN yum install -y libtool cmake3 |
There was a problem hiding this comment.
Clean yum metadata after install to avoid unnecessary image bloat.
The added install line keeps yum caches in the layer. Please clean caches in the same RUN step.
Suggested Dockerfile fix
-RUN yum install -y wget
-RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel
-RUN yum install -y libtool cmake3
+RUN yum install -y wget xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel libtool cmake3 \
+ && yum clean all \
+ && rm -rf /var/cache/yum📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN yum install -y wget | |
| RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel | |
| RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel | |
| RUN yum install -y libtool cmake3 | |
| RUN yum install -y wget xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel libtool cmake3 \ | |
| && yum clean all \ | |
| && rm -rf /var/cache/yum |
🧰 Tools
🪛 Trivy (0.69.1)
[error] 12-12: 'yum clean all' missing
'yum clean all' is missed: yum install -y libtool cmake3
Rule: DS-0015
(IaC/Dockerfile)
[error] 10-10: 'yum clean all' missing
'yum clean all' is missed: yum install -y wget
Rule: DS-0015
(IaC/Dockerfile)
[error] 11-11: 'yum clean all' missing
'yum clean all' is missed: yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel libcurl-devel
Rule: DS-0015
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dockerfiles/pyopenms/manylinux/Dockerfile` around lines 10 - 12, The three
separate RUN yum install lines leave yum metadata in image layers; combine the
package installs into a single RUN and clean caches in the same step (e.g., run
the installs for wget, xz, qt6-qtbase-devel, qt6-qtsvg-devel, libcurl-devel,
libtool, cmake3 in one RUN and then run yum clean all && rm -rf
/var/cache/yum/*) so the yum metadata is removed in the same layer that
performed the install; update the Dockerfile lines with a single RUN that
performs the installs followed by the cache cleanup.
|
Can you make sure that the dependency chain curl - aws-sdk - arrow/parquet uses the same curl? |
|
By the way, Arrow has a cool dependency resolution concept:
|
- Add CURL to BUILD_TYPE/VALID_BUILD_TYPES - Add curl-8.12.1 source archive config (SHA256 validated) - Add curl.cmake build macro with platform-native TLS backends (Schannel on Windows, SecureTransport on macOS, OpenSSL on Linux) - Add libcurl-dev packages to Dockerfiles - Disable LDAP and libpsl in curl build Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
timosachsenberg
force-pushed
the
feature/add-curl-to-contrib
branch
from
d4f8f2c to
ed114d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
CMakeLists.txt (1)
94-95:⚠️ Potential issue | 🟡 MinorKeep
BUILD_TYPEhelp text aligned withVALID_BUILD_TYPES.The help text still omits
EIGENandHDF5, while they are valid options.Suggested fix
-set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,KISSFFT,OPENMP,ARROW,LIBZIP,CURL") +set( BUILD_TYPE LIST CACHE STRING "Can be used to restrict building to a single library: ALL,LIBSVM,XERCESC,BOOST,COINOR,BZIP2,ZLIB,GLPK,EIGEN,KISSFFT,HDF5,OPENMP,ARROW,LIBZIP,CURL")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 94 - 95, Update the BUILD_TYPE help string to match VALID_BUILD_TYPES: include "EIGEN" and "HDF5" in the first set(...) call so the help text lists all valid options; specifically edit the set(BUILD_TYPE LIST CACHE STRING "...") line to add EIGEN and HDF5 consistent with the VALID_BUILD_TYPES variable.dockerfiles/pyopenms/manylinux/Dockerfile (1)
10-12:⚠️ Potential issue | 🟡 MinorCombine yum installs and clean metadata in the same layer.
This still leaves yum cache metadata in image layers and keeps triggering DS-0015.
Suggested fix
-RUN yum install -y wget -RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel -RUN yum install -y libtool cmake3 libcurl-devel +RUN yum install -y \ + wget \ + xz qt6-qtbase-devel qt6-qtsvg-devel \ + libtool cmake3 libcurl-devel \ + && yum clean all \ + && rm -rf /var/cache/yum/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfiles/pyopenms/manylinux/Dockerfile` around lines 10 - 12, Combine the three separate RUN yum install lines into a single RUN that installs all packages in one command and then cleans yum metadata/cache in the same layer (e.g., run yum install -y wget xz qt6-qtbase-devel qt6-qtsvg-devel libtool cmake3 libcurl-devel && yum clean all && rm -rf /var/cache/yum) so the metadata isn't preserved across layers; update the existing RUN lines (the three RUN lines installing packages) in the Dockerfile to this single consolidated RUN with cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfiles/pyopenms/manylinux/ARM64_Dockerfile`:
- Around line 10-12: Combine package installation and cache cleanup into the
same RUN steps that call yum so the yum metadata isn't preserved across layers;
for the RUN lines that install wget, qt6-qtbase-devel/qt6-qtsvg-devel, and
libtool/cmake3/libcurl-devel, append the standard cache-clean commands (e.g.,
yum clean all && rm -rf /var/cache/yum /var/cache/dnf) to each corresponding RUN
invocation so install and cleanup occur in one layer per package group.
In `@libraries.cmake/curl.cmake`:
- Around line 43-46: The diff currently forces shared builds by setting -D
BUILD_SHARED_LIBS=ON which overrides the global shared/static policy and can
desync curl from other contrib; update the curl CMake invocation to stop forcing
BUILD_SHARED_LIBS=ON — either remove the -D BUILD_SHARED_LIBS line entirely or
make it conditional/forward the consuming variable (e.g., pass -D
BUILD_SHARED_LIBS=${BUILD_SHARED_LIBS} or only set it when an explicit
curl-specific option is provided), and keep the other flags (CMAKE_BUILD_TYPE,
CMAKE_INSTALL_PREFIX, BUILD_CURL_EXE) unchanged so curl follows the project's
global shared/static policy.
- Around line 8-13: The ZIP_ARGS variable is currently set with quoted strings
making them single-element lists; update the MSVC and non-MSVC branches to set
ZIP_ARGS without quotes so flags are tokenized (e.g., change set(ZIP_ARGS "x -y
-osrc") and set(ZIP_ARGS "xzf") to unquoted forms) so OPENMS_SMARTEXTRACT(
ZIP_ARGS ARCHIVE_CURL "CURL" "CMakeLists.txt") receives separate arguments and
7z flag parsing behaves like the other libraries.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 94-95: Update the BUILD_TYPE help string to match
VALID_BUILD_TYPES: include "EIGEN" and "HDF5" in the first set(...) call so the
help text lists all valid options; specifically edit the set(BUILD_TYPE LIST
CACHE STRING "...") line to add EIGEN and HDF5 consistent with the
VALID_BUILD_TYPES variable.
In `@dockerfiles/pyopenms/manylinux/Dockerfile`:
- Around line 10-12: Combine the three separate RUN yum install lines into a
single RUN that installs all packages in one command and then cleans yum
metadata/cache in the same layer (e.g., run yum install -y wget xz
qt6-qtbase-devel qt6-qtsvg-devel libtool cmake3 libcurl-devel && yum clean all
&& rm -rf /var/cache/yum) so the metadata isn't preserved across layers; update
the existing RUN lines (the three RUN lines installing packages) in the
Dockerfile to this single consolidated RUN with cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c48866ac-30e9-434b-97da-d48c6abb3c89
📒 Files selected for processing (5)
CMakeLists.txtdockerfiles/Dockerfiledockerfiles/pyopenms/manylinux/ARM64_Dockerfiledockerfiles/pyopenms/manylinux/Dockerfilelibraries.cmake/curl.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- dockerfiles/Dockerfile
| RUN yum install -y wget | ||
| RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel | ||
| RUN yum install -y libtool cmake3 | ||
| RUN yum install -y libtool cmake3 libcurl-devel |
There was a problem hiding this comment.
Clean yum caches in the same RUN step after package install.
Current layered installs retain yum metadata and inflate the image.
Suggested fix
-RUN yum install -y wget
-RUN yum install -y xz qt6-qtbase-devel qt6-qtsvg-devel
-RUN yum install -y libtool cmake3 libcurl-devel
+RUN yum install -y \
+ wget \
+ xz qt6-qtbase-devel qt6-qtsvg-devel \
+ libtool cmake3 libcurl-devel \
+ && yum clean all \
+ && rm -rf /var/cache/yum/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dockerfiles/pyopenms/manylinux/ARM64_Dockerfile` around lines 10 - 12,
Combine package installation and cache cleanup into the same RUN steps that call
yum so the yum metadata isn't preserved across layers; for the RUN lines that
install wget, qt6-qtbase-devel/qt6-qtsvg-devel, and
libtool/cmake3/libcurl-devel, append the standard cache-clean commands (e.g.,
yum clean all && rm -rf /var/cache/yum /var/cache/dnf) to each corresponding RUN
invocation so install and cleanup occur in one layer per package group.
| if(MSVC) | ||
| set(ZIP_ARGS "x -y -osrc") | ||
| else() | ||
| set(ZIP_ARGS "xzf") | ||
| endif() | ||
| OPENMS_SMARTEXTRACT(ZIP_ARGS ARCHIVE_CURL "CURL" "CMakeLists.txt") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/libraries.cmake/*.cmake" -type f | head -20Repository: OpenMS/contrib
Length of output: 1855
🏁 Script executed:
cat -n libraries.cmake/curl.cmake 2>/dev/null | head -20Repository: OpenMS/contrib
Length of output: 825
🏁 Script executed:
rg -n "set\(ZIP_ARGS" libraries.cmake/ -A 1 -B 1Repository: OpenMS/contrib
Length of output: 3461
🏁 Script executed:
rg -n "OPENMS_SMARTEXTRACT" . -A 5 -B 2 --type cmake | head -50Repository: OpenMS/contrib
Length of output: 3165
Remove quotes from ZIP_ARGS to match project conventions and ensure correct flag parsing.
curl.cmake uses quoted strings for ZIP_ARGS ("x -y -osrc" and "xzf"), making them single-element lists in CMake. All 14 other library files in libraries.cmake/ use the unquoted tokenized form (x -y -osrc and xzf), which correctly splits extraction flags as separate arguments. The quoted form risks incorrect argument passing to 7z on MSVC.
Suggested fix
if(MSVC)
- set(ZIP_ARGS "x -y -osrc")
+ set(ZIP_ARGS x -y -osrc)
else()
- set(ZIP_ARGS "xzf")
+ set(ZIP_ARGS xzf)
endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libraries.cmake/curl.cmake` around lines 8 - 13, The ZIP_ARGS variable is
currently set with quoted strings making them single-element lists; update the
MSVC and non-MSVC branches to set ZIP_ARGS without quotes so flags are tokenized
(e.g., change set(ZIP_ARGS "x -y -osrc") and set(ZIP_ARGS "xzf") to unquoted
forms) so OPENMS_SMARTEXTRACT( ZIP_ARGS ARCHIVE_CURL "CURL" "CMakeLists.txt")
receives separate arguments and 7z flag parsing behaves like the other
libraries.
| -D CMAKE_BUILD_TYPE=Release | ||
| -D CMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR} | ||
| -D BUILD_SHARED_LIBS=ON | ||
| -D BUILD_CURL_EXE=OFF |
There was a problem hiding this comment.
Do not force shared curl builds unconditionally.
This overrides the global shared/static policy and can desync curl artifacts from the rest of contrib.
Suggested fix
- -D BUILD_SHARED_LIBS=ON
+ -D BUILD_SHARED_LIBS=${BUILD_SHARED_LIBRARIES}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libraries.cmake/curl.cmake` around lines 43 - 46, The diff currently forces
shared builds by setting -D BUILD_SHARED_LIBS=ON which overrides the global
shared/static policy and can desync curl from other contrib; update the curl
CMake invocation to stop forcing BUILD_SHARED_LIBS=ON — either remove the -D
BUILD_SHARED_LIBS line entirely or make it conditional/forward the consuming
variable (e.g., pass -D BUILD_SHARED_LIBS=${BUILD_SHARED_LIBS} or only set it
when an explicit curl-specific option is provided), and keep the other flags
(CMAKE_BUILD_TYPE, CMAKE_INSTALL_PREFIX, BUILD_CURL_EXE) unchanged so curl
follows the project's global shared/static policy.
- Fix ZIP_ARGS quoting in curl.cmake (fixes Windows CI extraction failure)
- Use ${BUILD_SHARED_LIBRARIES} instead of hardcoded BUILD_SHARED_LIBS=ON
- Move CURL build before ARROW so Arrow can find contrib-built curl
- Enable ARROW_S3 with AWSSDK_SOURCE=BUNDLED to use contrib curl
- Align BUILD_TYPE help string with VALID_BUILD_TYPES (add EIGEN, HDF5)
- Consolidate Dockerfile yum installs with cache cleanup
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request integrates CURL as a new contrib library into the OpenMS build system. It adds CMake build configuration for CURL compilation, updates Docker environments to include libcurl development packages, modifies Arrow's configuration to enable S3 support via bundled AWS SDK, and documents the integration design specifying CURL must build before Arrow. Changes
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/plans/2026-03-06-curl-arrow-integration-design.md (1)
8-11: Consider clarifying the "Currently" section to reflect the before/after state.The "Currently" section describes the state before this PR's changes but reads as if it describes the current state. Consider rewording to "Before this PR:" or "Previously:" to avoid confusion for readers who encounter this document after the changes are merged.
📝 Suggested clarification
-Currently: -- CURL is built after Arrow (wrong order) -- Arrow has no S3/curl options enabled -- Arrow does not bundle curl — it requires it via `find_package(CURL)` +Before this PR: +- CURL was built after Arrow (wrong order) +- Arrow had no S3/curl options enabled +- Arrow does not bundle curl — it requires it via `find_package(CURL)`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-06-curl-arrow-integration-design.md` around lines 8 - 11, The "Currently:" bullet list in the document reads like the post-change state; update that header to "Before this PR:" or "Previously:" and adjust any surrounding text so the three bullets ("CURL is built after Arrow...", "Arrow has no S3/curl options...", "Arrow does not bundle curl — it requires it via `find_package(CURL)`") are clearly marked as the prior state to avoid confusion for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/plans/2026-03-06-curl-arrow-integration-design.md`:
- Around line 8-11: The "Currently:" bullet list in the document reads like the
post-change state; update that header to "Before this PR:" or "Previously:" and
adjust any surrounding text so the three bullets ("CURL is built after
Arrow...", "Arrow has no S3/curl options...", "Arrow does not bundle curl — it
requires it via `find_package(CURL)`") are clearly marked as the prior state to
avoid confusion for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d528e08-927e-4e80-b89c-767f4737084b
📒 Files selected for processing (7)
CMakeLists.txtdockerfiles/Dockerfiledockerfiles/pyopenms/manylinux/ARM64_Dockerfiledockerfiles/pyopenms/manylinux/Dockerfiledocs/plans/2026-03-06-curl-arrow-integration-design.mdlibraries.cmake/arrow.cmakelibraries.cmake/curl.cmake
2 participants
Summary
curl.cmakebuild script for building libcurl from source with platform-native TLS backends (Schannel/SecureTransport/OpenSSL)2024-04-29-000000to current3.6.0release tagNeeded by OpenMS PR OpenMS/OpenMS#8841 which replaces Qt Network with libcurl.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores