fix(cmake): use GITHUB_TOKEN for dependency downloads to avoid rate limits#27334
fix(cmake): use GITHUB_TOKEN for dependency downloads to avoid rate limits#27334dylan-conway wants to merge 3 commits intomainfrom
Conversation
…imits Pass Authorization header for GitHub URLs when GITHUB_TOKEN is set. Raises the rate limit from 60 to 5000 requests/hour, preventing download failures when many dependencies are fetched in parallel. Only applies to github.com URLs. No-op when GITHUB_TOKEN is unset.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds Authorization header support for GitHub token in download calls; replaces a single WebKit download with a three-attempt retry loop, cleans up cache and existing WEBKIT_PATH after extraction, and performs an OS-specific post-download adjustment on macOS. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Newest first ✅ b3b80 — Looks good! Reviewed 3 files across Previous reviews✅ 98064 — Looks good! Reviewed 2 files across Previous reviews✅ e6237 — Looks good! Reviewed 1 file in |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/tools/SetupWebKit.cmake (1)
265-267: 🧹 Nitpick | 🔵 TrivialConsider adding a comment explaining why unicode directory is removed on Apple.
This Apple-specific cleanup isn't immediately obvious. A brief comment explaining the rationale (e.g., "Use system ICU headers instead of bundled ones") would improve maintainability.
📝 Suggested documentation improvement
if(APPLE) + # Use system ICU unicode headers instead of bundled ones on macOS file(REMOVE_RECURSE ${WEBKIT_INCLUDE_PATH}/unicode) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/tools/SetupWebKit.cmake` around lines 265 - 267, Add a brief explanatory comment above the APPLE conditional that removes the unicode directory to state why we remove it on macOS (e.g., "On Apple platforms use system ICU / system Unicode headers instead of the bundled WebKit unicode/ headers to avoid conflicts"), so place the comment immediately before the if(APPLE) ... file(REMOVE_RECURSE ${WEBKIT_INCLUDE_PATH}/unicode) endif() block to clarify the rationale for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/tools/SetupWebKit.cmake`:
- Around line 244-258: The WebKit download loop uses CMake's file(DOWNLOAD)
without Authorization, so change it to call the existing DownloadUrl.cmake
helper (as DownloadZig.cmake does) via execute_process and pass
DOWNLOAD_URL=${WEBKIT_DOWNLOAD_URL} and
DOWNLOAD_PATH=${CACHE_PATH}/${WEBKIT_FILENAME}, or alternatively add an
HTTPHEADER using the GITHUB_TOKEN check to the file(DOWNLOAD) invocation; prefer
the DownloadUrl.cmake approach because it supplies the Authorization header,
longer retry logic and extraction, and replace the manual retry loop around
WEBKIT_DOWNLOAD_ATTEMPT with the helper invocation (retain the variables
WEBKIT_DOWNLOAD_URL, CACHE_PATH, WEBKIT_FILENAME for parameter names).
---
Outside diff comments:
In `@cmake/tools/SetupWebKit.cmake`:
- Around line 265-267: Add a brief explanatory comment above the APPLE
conditional that removes the unicode directory to state why we remove it on
macOS (e.g., "On Apple platforms use system ICU / system Unicode headers instead
of the bundled WebKit unicode/ headers to avoid conflicts"), so place the
comment immediately before the if(APPLE) ... file(REMOVE_RECURSE
${WEBKIT_INCLUDE_PATH}/unicode) endif() block to clarify the rationale for
future maintainers.
| foreach(WEBKIT_DOWNLOAD_ATTEMPT RANGE 1 3) | ||
| file( | ||
| DOWNLOAD ${WEBKIT_DOWNLOAD_URL} ${CACHE_PATH}/${WEBKIT_FILENAME} SHOW_PROGRESS | ||
| STATUS WEBKIT_DOWNLOAD_STATUS | ||
| ) | ||
| if("${WEBKIT_DOWNLOAD_STATUS}" MATCHES "^0;") | ||
| break() | ||
| endif() | ||
| message(WARNING "WebKit download attempt ${WEBKIT_DOWNLOAD_ATTEMPT} failed: ${WEBKIT_DOWNLOAD_STATUS}") | ||
| file(REMOVE ${CACHE_PATH}/${WEBKIT_FILENAME}) | ||
| if(WEBKIT_DOWNLOAD_ATTEMPT EQUAL 3) | ||
| message(FATAL_ERROR "Failed to download WebKit after 3 attempts: ${WEBKIT_DOWNLOAD_STATUS}") | ||
| endif() | ||
| execute_process(COMMAND ${CMAKE_COMMAND} -E sleep 5) | ||
| endforeach() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DownloadUrl.cmake to verify its existence
echo "=== Searching for DownloadUrl.cmake ==="
fd -t f "DownloadUrl.cmake"
echo -e "\n=== CMake files in cmake/scripts directory ==="
find cmake/scripts -type f -name "*.cmake" 2>/dev/null | sort
echo -e "\n=== All cmake files that reference DOWNLOAD or DownloadUrl ==="
rg "DOWNLOAD|DownloadUrl" --type cmake -l
echo -e "\n=== Check cmake/tools/SetupWebKit.cmake for context ==="
fd -t f "SetupWebKit.cmake" --exec wc -l {} \;Repository: oven-sh/bun
Length of output: 678
🏁 Script executed:
#!/bin/bash
echo "=== Content of DownloadUrl.cmake ==="
cat -n cmake/scripts/DownloadUrl.cmake
echo -e "\n=== Content of DownloadZig.cmake (similar use case) ==="
cat -n cmake/scripts/DownloadZig.cmake
echo -e "\n=== SetupWebKit.cmake full content ==="
cat -n cmake/tools/SetupWebKit.cmakeRepository: oven-sh/bun
Length of output: 18634
WebKit download doesn't use GITHUB_TOKEN authentication, unlike similar downloads in the codebase.
The PR objective is to add Authorization: Bearer $GITHUB_TOKEN headers for downloads from github.com to increase the rate limit. However, this file uses the native CMake file(DOWNLOAD) command without any authorization headers, while WebKit is downloaded from https://github.com/oven-sh/WebKit/releases/download/...
The DownloadUrl.cmake module already provides this functionality and is successfully used by DownloadZig.cmake for a similar Zig release download from github.com. The recommended approach is to use the same pattern:
- Call
DownloadUrl.cmakeviaexecute_process(asDownloadZig.cmakedoes) withDOWNLOAD_URLandDOWNLOAD_PATHparameters, or - Add the HTTPHEADER with
GITHUB_TOKENcheck directly to thefile(DOWNLOAD)command.
Using DownloadUrl.cmake is preferable because it also provides automatic retry logic (10 retries vs. the current 3) and handles archive extraction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake/tools/SetupWebKit.cmake` around lines 244 - 258, The WebKit download
loop uses CMake's file(DOWNLOAD) without Authorization, so change it to call the
existing DownloadUrl.cmake helper (as DownloadZig.cmake does) via
execute_process and pass DOWNLOAD_URL=${WEBKIT_DOWNLOAD_URL} and
DOWNLOAD_PATH=${CACHE_PATH}/${WEBKIT_FILENAME}, or alternatively add an
HTTPHEADER using the GITHUB_TOKEN check to the file(DOWNLOAD) invocation; prefer
the DownloadUrl.cmake approach because it supplies the Authorization header,
longer retry logic and extraction, and replace the manual retry loop around
WEBKIT_DOWNLOAD_ATTEMPT with the helper invocation (retain the variables
WEBKIT_DOWNLOAD_URL, CACHE_PATH, WEBKIT_FILENAME for parameter names).
2 participants
When
GITHUB_TOKENis set in the environment, pass anAuthorization: Bearerheader on downloads fromgitlite.zycloud.tk. This raises the API rate limit from 60 to 5,000 requests/hour, which prevents the frequent download failures seen in CI when many dependencies are fetched in parallel.gitlite.zycloud.tkURLs — non-GitHub downloads are unaffectedGITHUB_TOKENis not set — fully backwards compatibleGITHUB_TOKENto workflows