fix: use curl for binary download to support proxy and add npmmirror …#226
fix: use curl for binary download to support proxy and add npmmirror …#226qianzhicheng95 merged 1 commit intomainfrom
Conversation
…fallback Node.js https.get() does not honor https_proxy/HTTP_PROXY env vars, causing silent download failures behind firewalls. Switch to curl which natively supports proxy settings, and add npmmirror.com as a fallback mirror for regions where GitHub is slow or blocked. Change-Id: If9ace1e467e46f2a3009610a808bce8d78259e78
github-actions
bot
added
the
size/M
📝 WalkthroughWalkthroughThe installation script replaces a Node.js HTTPS streaming download mechanism with a synchronous curl-based approach, introducing fallback support to an alternate mirror URL and adding Windows-specific SSL certificate handling alongside synchronous control flow with expanded error messaging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR replaces the custom Node.js HTTPS download implementation with Confidence Score: 5/5Safe to merge; the core proxy fix is correct and the fallback logic is sound — only minor P2 suggestions remain. All findings are P2 style suggestions (log the swallowed error, guard for missing curl). No P0/P1 defects were found. The switch to curl is a well-known pattern for proxy-aware downloads, the URL construction uses only fixed constants so there is no shell-injection risk, and the mirror fallback correctly overwrites any partially-written file. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[npm postinstall → install.js] --> B[Detect platform & arch]
B --> C{Supported?}
C -- No --> D[process.exit 1]
C -- Yes --> E[mkdtemp tmpDir]
E --> F[curl download GITHUB_URL]
F -- success --> H[Extract archive]
F -- failure --> G[curl download MIRROR_URL npmmirror]
G -- success --> H
G -- failure --> I[print proxy hint & process.exit 1]
H --> J[Copy binary to bin/]
J --> K[chmod 0o755]
K --> L[rmSync tmpDir]
L --> M[Done ✓]
Reviews (1): Last reviewed commit: "fix: use curl for binary download to sup..." | Re-trigger Greptile |
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/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 (1)
scripts/install.js (1)
57-61: Consider logging which download source was used.The fallback silently switches to the mirror without any indication to the user. For transparency (especially since the mirror is a third-party source), consider logging which URL succeeded or at least when falling back.
💡 Suggested improvement for transparency
try { download(GITHUB_URL, archivePath); } catch (err) { + console.log("GitHub download failed, trying mirror..."); download(MIRROR_URL, archivePath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` around lines 57 - 61, The current download fallback silently switches from GITHUB_URL to MIRROR_URL inside the try/catch around download(GITHUB_URL, archivePath), so update the block to log which source succeeded and when a fallback occurred: call processLogger/console.log before or after invoking download to record "attempting GITHUB_URL" and on catch log the error and "falling back to MIRROR_URL", then call download(MIRROR_URL, archivePath) and log its success or failure; reference the download function and the GITHUB_URL, MIRROR_URL, archivePath variables and include the caught err when logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.js`:
- Around line 42-50: The download function assumes curl is on PATH (causing
cryptic failures on older Windows or minimal Linux containers); add an
availability check before calling execSync in download (or during module init)
that runs a lightweight command (e.g., spawnSync('curl','--version') or
equivalent) and, if curl is missing, throw or log a clear error with remediation
(install curl or require Windows ≥ v1803) mentioning the symbols download and
isWindows so maintainers can find the code; alternatively, document the minimum
Windows version in package.json/postinstall message if you prefer not to fail
install automatically.
---
Nitpick comments:
In `@scripts/install.js`:
- Around line 57-61: The current download fallback silently switches from
GITHUB_URL to MIRROR_URL inside the try/catch around download(GITHUB_URL,
archivePath), so update the block to log which source succeeded and when a
fallback occurred: call processLogger/console.log before or after invoking
download to record "attempting GITHUB_URL" and on catch log the error and
"falling back to MIRROR_URL", then call download(MIRROR_URL, archivePath) and
log its success or failure; reference the download function and the GITHUB_URL,
MIRROR_URL, archivePath variables and include the caught err when logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@dcaba2d8c957746065adb39aff0f44d2f7feebbb🧩 Skill updatenpx skills add larksuite/cli#feat/install_curl -y -g |
2 participants
…fallback
Node.js https.get() does not honor https_proxy/HTTP_PROXY env vars, causing silent download failures behind firewalls. Switch to curl which natively supports proxy settings, and add npmmirror.com as a fallback mirror for regions where GitHub is slow or blocked.
Change-Id: If9ace1e467e46f2a3009610a808bce8d78259e78
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit