fix(benchx_cli): resolve upstream conflict#2221
Conversation
🦋 Changeset detectedLatest commit: 56ffe5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
95072cb to
95e1561
Compare
There was a problem hiding this comment.
Pull request overview
Updates the benchx_cli CI build script to resolve an upstream conflict by moving to new upstream commit references and adjusting how the Lynx source is fetched and built.
Changes:
- Updated pinned upstream commit SHAs used to fetch/cherry-pick Lynx changes.
- Replaced
git clonewith an explicitgit init+git fetchflow for preparing the Lynx repo. - Adjusted GN args generation (including a macOS-specific
use_flutter_cxx=false) and removed the Android SDK manager patch application.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/lynx/benchx_cli/scripts/build.mjs | Updates commit pins and modifies repo preparation + build steps for Lynx in CI. |
| packages/lynx/benchx_cli/patches/android_sdk_manager.diff | Removes an obsolete patch that was previously applied during the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a changeset file, comments out the Android SDK manager DEPS entry, and updates the benchx_cli build script: commit targets, repo checkout flow, virtualenv/pip steps, habitat sync adjustments, GN gen args, and artifact copy logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 32.31%
Performance Changes
Comparing Footnotes
|
Web Explorer#7673 Bundle Size — 383.74KiB (0%).56ffe5d(current) vs 87682f6 main#7667(baseline) Bundle metrics
Bundle size by type
|
| Current #7673 |
Baseline #7667 |
|
|---|---|---|
252.83KiB |
252.83KiB |
|
95.85KiB |
95.85KiB |
|
35.06KiB |
35.06KiB |
Bundle analysis report Branch hzy:p/hzy/benchx_cli_maintenance Project dashboard
Generated by RelativeCI Documentation Report issue
cc9d315 to
29c19be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/lynx/benchx_cli/scripts/build.mjs`:
- Around line 91-100: The script currently adds a hardcoded personal fork via
the git command "git remote add hzy https://github.com/hzy/lynx" and then
fetches and cherry-picks ${PICK_COMMIT}, which is fragile; update the script to
stop referencing the personal URL directly by either (A) pushing the patch
commit into an org-owned repository and replace "https://github.com/hzy/lynx"
with the org repo URL, or (B) make the remote configurable by introducing an
environment variable (e.g., PICK_REMOTE_URL and PICK_REMOTE_NAME) and use those
in the "git remote add" and "git fetch" steps so the script uses a stable org
repo by default while allowing overrides; ensure references to the remote name
"hzy" and usage of ${PICK_COMMIT} are updated to use the new variable names.
078e395 to
30b6dd4
Compare
There was a problem hiding this comment.
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)
103-111:⚠️ Potential issue | 🟡 MinorDocument
uvas a required tool and add installation or error handling.The build script uses
uv(lines 103-111) but there's no documentation of this requirement in CONTRIBUTING.md or elsewhere. Ifuvis not pre-installed in CI runners, the build fails with a confusing "command not found" error. Either:
- Add
uvto the documented requirements in CONTRIBUTING.md and ensure CI installs it, or- Add a guard with a clear error message explaining that
uvmust be installed
🧹 Nitpick comments (2)
packages/lynx/benchx_cli/scripts/build.mjs (2)
118-125: macOS-onlyuse_flutter_cxx=false— consider adding a comment explaining why.The conditional GN arg
use_flutter_cxx=falseis only added on macOS. A brief inline comment explaining the reason (e.g., a macOS-specific linker issue, incompatible system library, etc.) would help future maintainers understand this platform divergence.💡 Suggested comment
gn gen --args=${ process.platform === 'darwin' - ? `enable_unittests=true enable_trace="perfetto" jsengine_type="quickjs" enable_frozen_mode=true use_flutter_cxx=false` + // use_flutter_cxx=false is required on macOS to avoid <reason> + ? `enable_unittests=true enable_trace="perfetto" jsengine_type="quickjs" enable_frozen_mode=true use_flutter_cxx=false` : `enable_unittests=true enable_trace="perfetto" jsengine_type="quickjs" enable_frozen_mode=true` } out/Default
118-122: Duplicated GN args string — extract to reduce maintenance burden.The two GN arg strings are identical except for the trailing
use_flutter_cxx=falseon macOS. If more flags are added later, both branches need updating. Consider extracting the common base:♻️ Suggested refactor
const BASE_GN_ARGS = `enable_unittests=true enable_trace="perfetto" jsengine_type="quickjs" enable_frozen_mode=true`; const GN_ARGS = process.platform === 'darwin' ? `${BASE_GN_ARGS} use_flutter_cxx=false` : BASE_GN_ARGS;Then use
${GN_ARGS}in the template literal.
cfcc97c to
ff884f3
Compare
Update commit references and improve build script for lynx
ff884f3 to
56ffe5d
Compare
Update commit references and improve build script for lynx
Summary by CodeRabbit
Checklist