fix(benchx_cli): avoid downloading android_sdk_manager#1942
fix(benchx_cli): avoid downloading android_sdk_manager#1942colinaaa merged 1 commit intolynx-family:mainfrom
Conversation
|
📝 WalkthroughWalkthroughThe PR disables the Android SDK Manager dependency by applying a patch file, refactors the build script to remove Habitat build-from-source logic and use direct tools/hab sync invocation, and updates cache invalidation rules to account for patch file changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lynx/benchx_cli/scripts/build.mjs (1)
37-39: Binary cache check ignoresPICK_COMMITand patch changesRight now
checkBinaryonly validatesbenchx_cli.commitagainstCOMMIT, but the actual build inputs also include:
- The cherry-picked
PICK_COMMIT(git cherry-pick -n ${PICK_COMMIT}).- The applied patch
../patches/android_sdk_manager.diff.Because
git cherry-pick -ndoesn’t advanceHEADand the patch is uncommitted,git rev-parse HEADstill returns the baseCOMMIT, even though the working tree includes extra changes. That means:
- If you change
PICK_COMMITor the patch contents but leaveCOMMITunchanged,checkBinary()will consider the existing binary “up to date” and skip rebuilding, despite the underlying sources having changed.- The new
patches/*turbo input will cause the task to re-run, but the script may exit early and keep a stale binary.I’d treat this as a real cache bug. A few possible fixes:
- Treat the
.commitfile as a “build ID” instead of a pure git commit and write something like${COMMIT}-${PICK_COMMIT}-${patchVersion}into it, then compare against the same composite string incheckBinary().- Or, the simpler option: drop
checkBinary()altogether and rely on turbo’s inputs/outputs caching, since you’ve already modeledscripts/build.mjsandpatches/*as inputs.Also applies to: 58-69, 111-124
🧹 Nitpick comments (2)
packages/lynx/benchx_cli/scripts/build.mjs (2)
89-109: New clone/patch/sync flow looks reasonable; verifyuv/tools/habavailabilityThe new flow:
- clones the pinned Lynx repo,
- cherry-picks
PICK_COMMIT,- applies
android_sdk_manager.diff, and- sets up a
.venvwithuvbefore runningtools/hab sync .is coherent and nicely self-contained for CI, and the
process.platform === 'win32'guard still avoids Unix-specific logic on Windows.Two small things to verify:
- CI images running this script must have
uvandtools/habon PATH; otherwiseuv venv/tools/hab sync .will fail where the previous../habitat/venv/bin/habflow may have worked.- If
tools/envsetup.shalready handles venv activation or other env setup forhab, you might be able to simplify the venv steps, but that’s optional.
83-87: Leftoverhabitatdirectory cleanup can likely be removedYou still
rm -rf habitatbefore and after the build, but the script no longer creates or uses ahabitatdirectory (it now uses a.venvinsidelynxandtools/hab sync .).This is harmless but dead code; consider dropping the
rm -rf habitatlines to reduce confusion about whether Habitat artifacts are still expected here.Also applies to: 127-131
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/lynx/benchx_cli/patches/android_sdk_manager.diff(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(2 hunks)packages/lynx/benchx_cli/turbo.json(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
packages/lynx/benchx_cli/patches/android_sdk_manager.diffpackages/lynx/benchx_cli/scripts/build.mjspackages/lynx/benchx_cli/turbo.json
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.645Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
packages/lynx/benchx_cli/patches/android_sdk_manager.diffpackages/lynx/benchx_cli/scripts/build.mjspackages/lynx/benchx_cli/turbo.json
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
packages/lynx/benchx_cli/patches/android_sdk_manager.diffpackages/lynx/benchx_cli/scripts/build.mjspackages/lynx/benchx_cli/turbo.json
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
packages/lynx/benchx_cli/patches/android_sdk_manager.diffpackages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjspackages/lynx/benchx_cli/turbo.json
📚 Learning: 2025-08-19T11:25:36.127Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1558
File: .changeset/solid-squids-fall.md:2-2
Timestamp: 2025-08-19T11:25:36.127Z
Learning: In the lynx-family/lynx-stack repository, changesets should use the exact package name from package.json#name, not generic or unscoped names. Each package has its own specific scoped name (e.g., "lynx-js/react-transform" for packages/react/transform).
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-08-13T11:36:12.075Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1523
File: vitest.config.ts:52-72
Timestamp: 2025-08-13T11:36:12.075Z
Learning: The lynx-stack project requires Node.js >=22 as specified in package.json engines, so Node.js compatibility fallbacks for features introduced before v22 are unnecessary.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-08-20T04:56:36.011Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1566
File: package.json:53-53
Timestamp: 2025-08-20T04:56:36.011Z
Learning: In lynx-stack, Node.js v24 is the preferred/default version for development (established in PR #1557), but Node.js v22 compatibility is maintained specifically for external CI systems like rspack-ecosystem-ci. The engines.node specification uses "^22 || ^24" to support both versions while keeping v24 as the primary target.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 Learning: 2025-09-29T06:43:40.182Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T06:43:40.182Z
Learning: Applies to packages/**/etc/*.api.md : Always commit API extractor output after running `pnpm turbo api-extractor -- --local` (commit updated API report files)
Applied to files:
packages/lynx/benchx_cli/turbo.json
🔇 Additional comments (2)
packages/lynx/benchx_cli/turbo.json (1)
5-11: Includingpatches/*ininputsis appropriateHooking
patches/*into thebuildtask inputs matches the new patch-based build flow and ensures turbo invalidates the cache when patches change. Looks good.packages/lynx/benchx_cli/patches/android_sdk_manager.diff (1)
1-31: I can see the search confirms no problematic references toandroid_sdk_managerelsewhere in the codebase. Let me verify the patch applicability by checking the DEPS file location and ensuring the patch will still apply cleanly:Verified: Patch safely disables
buildtools/android_sdk_managerdependencyThe codebase search confirms there are no hard-coded references to
buildtools/android_sdk_manageroutside the patch file itself. The dependency is safely commented out in the DEPS file and won't break any existing code flows.The original concern about patch applicability when the COMMIT changes remains valid—verify the patch context still matches when updating the Chromium version in
build.mjs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1942 will degrade performances by 7.3%Comparing Summary
Benchmarks breakdown
Footnotes
|
Web Explorer#6221 Bundle Size — 366.92KiB (0%).fda1821(current) vs 6acb37b main#6207(baseline) Bundle metrics
|
| Current #6221 |
Baseline #6207 |
|
|---|---|---|
 Initial JS |
146.22KiB |
146.22KiB |
| Initial CSS |
32.37KiB |
32.37KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
221 |
221 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.21% |
3.21% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #6221 |
Baseline #6207 |
|
|---|---|---|
| JS |
240.74KiB |
240.74KiB |
| Other |
93.8KiB |
93.8KiB |
| CSS |
32.37KiB |
32.37KiB |
Bundle analysis report Branch colinaaa:colin/1118/benchx Project dashboard
Generated by RelativeCI Documentation Report issue
2 participants
Summary by CodeRabbit
Fix the build error: https://github.com/lynx-family/lynx-stack/actions/runs/19352194166/job/55366736827?pr=1938
Checklist