fix(install): detect curl version before using --ssl-revoke-best-effort#1124
Conversation
(cherry picked from commit da14737)
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a curl --version parser and a memoized probe in install.js, exports the helpers, and conditions Windows download() to prepend --ssl-revoke-best-effort only when curl >= 7.70.0; tests validate the parsing logic across threshold, future, and malformed inputs. ChangesWindows curl version detection and conditional flag usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
github-actions
Bot
added
the
size/M
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.js`:
- Around line 123-125: The regex in isCurlVersionSupported currently can match
substrings like "libcurl 8.0.0"; change the pattern to require the leading
"curl" token by anchoring it (e.g., use a start-of-string anchor or a word
boundary) so only outputs beginning with "curl" are accepted; update the match
in isCurlVersionSupported to use the anchored pattern while keeping the existing
capture groups for major/minor/patch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ce6dbc7-4c15-468d-b785-42589528b814
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1124 +/- ##
=======================================
Coverage 68.22% 68.22%
=======================================
Files 617 617
Lines 57192 57192
=======================================
Hits 39017 39017
Misses 14945 14945
Partials 3230 3230 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2604eda45d692886d8cb4055f6f0f01c669bb6c0🧩 Skill updatenpx skills add larksuite/cli#fix/install-curl-version-gate -y -g |
Extract the version comparison out of curlSupportsSslRevokeBestEffort() into a pure isCurlVersionSupported(output), so the >= 7.70.0 logic is unit testable without spawning curl. Add cases for 7.55.1 / 7.69.0 / 7.70.0 / 8.x plus the unparseable and libcurl-token edge cases (the regex must read the leading "curl X.Y.Z", not the trailing "libcurl/X.Y.Z"). Memoize the `curl --version` probe: curl's version is invariant for the install's lifetime while download() runs once per mirror URL, so probe at most once instead of re-spawning curl on every attempt.
c2abb78 to
2604eda
Compare
…rt (larksuite#1124) * fix(install): detect curl version before using --ssl-revoke-best-effort (cherry picked from commit da14737) * test(install): cover curl version gate and refactor for testability Extract the version comparison out of curlSupportsSslRevokeBestEffort() into a pure isCurlVersionSupported(output), so the >= 7.70.0 logic is unit testable without spawning curl. Add cases for 7.55.1 / 7.69.0 / 7.70.0 / 8.x plus the unparseable and libcurl-token edge cases (the regex must read the leading "curl X.Y.Z", not the trailing "libcurl/X.Y.Z"). Memoize the `curl --version` probe: curl's version is invariant for the install's lifetime while download() runs once per mirror URL, so probe at most once instead of re-spawning curl on every attempt. --------- Co-authored-by: EllienTang <146210093+Ellien-Tang@users.noreply.github.com> Co-authored-by: liangshuo-1 <266696938+liangshuo-1@users.noreply.github.com>
4 participants
Problem
On older Windows systems (e.g. Windows 10 build 19044 with curl 7.55.1),
npm installfails duringpostinstallbecausescripts/install.jsunconditionally adds--ssl-revoke-best-efforton Windows. That flag was only introduced in curl 7.70.0 (2020-04-29), so older curl exits with:Fixes #1099.
Solution
Gate the flag behind a runtime curl-version probe: only pass
--ssl-revoke-best-effortwhenisWindows && curl >= 7.70.0. Older curl falls back to standard SSL revocation checks and installs successfully; modern Windows curl behaves exactly as before (still avoidsCRYPT_E_REVOCATION_OFFLINE).Credit
The original fix is by @Ellien-Tang in #1116 — cherry-picked here with authorship preserved (first commit). This PR supersedes #1116 and adds maintainer follow-ups on top (second commit).
Follow-ups added on top
isCurlVersionSupported(output)(no I/O), so the>= 7.70.0logic is unit-testable without spawning a process.scripts/install.test.jsnow covers7.55.1/7.69.0/7.70.0/8.x, the unparseable case, and a regression guard ensuring the regex reads the leadingcurl X.Y.Zrather than the trailinglibcurl/X.Y.Z.download()runs once per mirror URL; thecurl --versionresult is now cached so curl is probed at most once per install instead of on every attempt.Test
--ssl-revoke-best-effortBackward compatibility
Summary by CodeRabbit
Bug Fixes
Tests