Xsn/curl ci test #18
Xsn/curl ci test #18
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
CMakeLists.txt (1)
171-175: Conditional Disabling of CURL When Examples Are Not BuiltIn the conditional block (lines 171–174), the code sets
LLAMA_CURLto OFF when examples are not being built, as noted by the comment “# curl is only needed for examples”. This design ensures that libcurl is only enabled when required. Consider adding a brief explanatory note or updating documentation to make this behavior clearer to downstream users..github/workflows/build-linux-cross.yml (1)
26-31: Explicit Disabling of LLAMA_CURL in CMake CommandFor the build steps (lines 26–31), the command now explicitly passes
-DLLAMA_CURL=OFFdue to the current issues highlighted by the TODO comment. Verify that this explicit disablement correctly interacts with the new conditional behavior in the CMakeLists (i.e. enabling curl only when examples are built) and update the TODO note when a proper fix forLLAMA_CURL=ONis available..github/workflows/server.yml (2)
194-196: Utilization of the Windows Setup CURL Composite ActionIn the Windows job section, the new composite action (
./.github/actions/windows-setup-curl.yml) is invoked (lines 194–196) to prepare libcurl. This modularizes the CURL setup on Windows and improves maintainability. Please ensure that this action is consistently tested across different Windows environments.
197-203: Passing CURL Configuration to CMakeThe build step in the Windows job now uses the
CURL_CMAKE_ARGoutput from the composite action (lines 197–203) to supply additional parameters to thecmakecommand. Confirm that the generated argument correctly points to the libcurl library and include directories, and that downstream targets (likellama-server) detect this configuration properly..github/actions/windows-setup-curl.yml (1)
24-28: Clarify Output Variable FormattingThe composite action sets the
curl_pathand especially thecurl_cmake_argoutput (lines 24–28) using echo commands with embedded quotes and backticks. Double-check that the quoting and escaping are correct so that downstream workflows correctly parse these values. Adding inline comments to explain the intended format could help future maintainability..github/workflows/build.yml (2)
56-57: Addcurlinstallation for macOS ARM64 Dependencies.
The newly addedbrew install curlensures that the macOS runner has the curl tool available for later steps. This aligns with the removal of the explicit-DLLAMA_CURL=ONflag in the build configuration.
63-69: Consistent Removal of-DLLAMA_CURL=ONFlag in Build Steps.
In the macOS ARM64 Build step, the CMake configuration no longer passes the-DLLAMA_CURL=ONflag. This change signals a move to relying on the system-installed curl (ensured by the dependency steps) rather than enforcing it via CMake. Please verify that this approach meets all runtime and linking requirements, and update the documentation if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.devops/cpu.Dockerfile(1 hunks).devops/cuda.Dockerfile(1 hunks).devops/intel.Dockerfile(1 hunks).devops/musa.Dockerfile(1 hunks).devops/rocm.Dockerfile(1 hunks).github/actions/windows-setup-curl.yml(1 hunks).github/workflows/bench.yml.disabled(0 hunks).github/workflows/build-linux-cross.yml(3 hunks).github/workflows/build.yml(10 hunks).github/workflows/server.yml(2 hunks)CMakeLists.txt(2 hunks)examples/server/tests/README.md(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/bench.yml.disabled
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build-linux-cross / ubuntu-latest-arm64-vulkan-cross
- GitHub Check: build-linux-cross / ubuntu-latest-riscv64-cpu-cross
- GitHub Check: build-linux-cross / ubuntu-latest-riscv64-vulkan-cross
- GitHub Check: windows-latest-cmake-hip
- GitHub Check: windows-latest-cmake-sycl
- GitHub Check: windows-msys2 (UCRT64, ucrt-x86_64, Release)
- GitHub Check: windows-2019-cmake-cuda (12.4, cuda)
- GitHub Check: windows-2019-cmake-cuda (11.7, cuda)
- GitHub Check: windows-msys2 (CLANG64, clang-x86_64, Release)
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: macOS-latest-cmake-tvos
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-22-cmake-sycl
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: macOS-latest-cmake-visionos
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: server (Release)
- GitHub Check: server (UNDEFINED, RelWithDebInfo)
- GitHub Check: server (ADDRESS, RelWithDebInfo)
🔇 Additional comments (16)
examples/server/tests/README.md (1)
20-20: Removal of-DLLAMA_CURL=ONFlag in Build Command
The cmake build command now omits the explicit-DLLAMA_CURL=ONflag, which is in line with the overall PR objectives. Please ensure that the default configuration or any separate actions appropriately handle the cURL setup for the server build..devops/cuda.Dockerfile (1)
24-24: Removed cURL Flag in CUDA Build Configuration
The updated cmake command no longer includes the-DLLAMA_CURL=ONflag, aligning with the intended changes. This streamlines the build configuration. Verify that downstream behavior relying on cURL configuration is correctly handled via defaults or alternative setup steps..devops/rocm.Dockerfile (1)
42-44: Updated ROCm CMake Command Without cURL Flag
The cmake invocation now omits the-DLLAMA_CURL=ONflag as expected. Please confirm that the default behavior for cURL (or its alternative setup) is properly configured within the ROCm build process..devops/intel.Dockerfile (1)
20-21: Intel Dockerfile: Removal of cURL Flag
The cmake command in this Dockerfile has been updated to remove the-DLLAMA_CURL=ONflag, consistent with other build configurations in the repository. It is advisable to double-check that the absence of this flag does not adversely affect the build on the OneAPI base environment..devops/musa.Dockerfile (1)
38-38: MUSA Dockerfile: cURL Flag Omission
The cmake command no longer includes the-DLLAMA_CURL=ONflag, which is consistent with the PR’s broader changes. Ensure that any required cURL handling is now managed either by default settings in CMakeLists.txt or through alternative setup actions..devops/cpu.Dockerfile (1)
16-19: Confirm Removal of CURL Flag in CMake CommandsThe CMake invocations for both the
amd64(line 17) andarm64(line 19) branches no longer include the-DLLAMA_CURL=ONflag. This change follows the new strategy where the CURL support is now controlled inside the CMake configuration rather than being forced via the Dockerfile. Please verify that the removal is intentional and that the installation oflibcurl4-openssl-dev(line 10) remains appropriate for any downstream dependency resolution.CMakeLists.txt (1)
84-86: Set LLAMA_CURL Default to ONThe
LLAMA_CURLoption is now defined as ON by default (line 84), which aligns with the plan to enable libcurl capabilities by default. This is a clear change compared to the previous explicit flag usage. Please double-check that this default will be acceptable for users who may not need curl unless examples are built..github/workflows/build-linux-cross.yml (1)
22-23: Addition of libcurl4-openssl-dev DependencyThe installation commands now include the
libcurl4-openssl-devpackage (lines 22–23) in the cross-build jobs. This update ensures that the necessary libcurl development files are available even though the CMake command now forces-DLLAMA_CURL=OFF. Please confirm that this dependency remains necessary given the new build configurations..github/workflows/server.yml (1)
216-221: Copying the libcurl DLL for Windows BuildsA subsequent step (lines 216–221) copies the libcurl DLL from the location indicated by
CURL_PATHinto the build output directory. Ensure that the copied DLL is in the correct location and that the runtime linker will find it when executingllama-serveron Windows..github/workflows/build.yml (7)
125-126: Addcurlinstallation for macOS x64 Dependencies.
The insertion ofbrew install curlin this job confirms that curl is installed on macOS x64 environments as well. This change is consistent with the overall approach to handling curl through system packages.
281-283: Ensure curl development libraries are installed in Ubuntu Sanitizer Job.
The updated dependency command now installslibcurl4-openssl-devalong with build-essential, which is important since the build no longer explicitly passes the-DLLAMA_CURL=ONflag.
322-324: Include curl dependencies for Ubuntu LLGuidance Job.
The change to installlibcurl4-openssl-devhere ensures that the necessary curl development libraries are available. This supports the revised build configuration where curl is provided by the system rather than through CMake flags.
359-362: Update dependency installation in Ubuntu RPC Job.
The dependency step now installslibcurl4-openssl-devafter updating apt-get. This modification is critical to maintain curl functionality after the removal of the-DLLAMA_CURL=ONflag from the CMake commands.
398-400: Refresh dependencies for Ubuntu Vulkan Job.
By extending the installation command to includelibcurl4-openssl-dev(in addition to build-essential, mesa-vulkan-drivers, and vulkan-sdk), this change ensures that all necessary libraries—including those for curl—are present. Verify that these dependencies install correctly on your target environment.
579-581: Update oneAPI dependency installation in Ubuntu SYCL FP16 Job.
The command now installsintel-oneapi-compiler-dpcpp-cpp libcurl4-openssl-devafter updating apt. This ensures that, along with the oneAPI compiler, the required curl development libraries are available, which is consistent with the new configuration approach.
991-993: Ensure all curl-related libraries are available in the Ubuntu CUDA Build.
The dependency installation now includeslibcurl4-openssl-devas part of the apt install list. This adjustment complements the removal of the-DLLAMA_CURL=ONflag from CMake, ensuring that curl functionality is still met using system packages.
| env: | ||
| CURL_VERSION: 8.6.0_6 | ||
| run: | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Utilize Action Input for CURL Version
The environment variable CURL_VERSION is hardcoded as 8.6.0_6 (lines 21–23) rather than using the action input provided in the inputs block. To allow flexibility, consider replacing the hardcoded value with ${{ inputs.curl_version }} so that users can override the version as needed.
✅ Actions performedReviews paused. |
Make sure to read the contributing guidelines before submitting a PR
Summary by CodeRabbit
New Features
Chores
Documentation