You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Replaces x64-based emulation with native arm64-based Hermes (by virtue of being a python package and changing the interpretor). Arm64-native builds will give better performance compared to emulated counterparts.
AI code review — automated review for reference; please use your judgment.
Add native, arm64-python interpreter for Hermes on WoA — substantial, careful installer work. Three coherent pieces: uv -aarch64 preference with fallbacks (qualified-first find, venv retry on qualified failure → x64 with an emulation warning), Install-WoaOpenSsl to let cryptography 50.x build from source on win_arm64, and arch detection wired after function registration. The qualified-then-fallback pattern is consistently applied and the $venvExitCode -ne 0 -and $venvPythonRequest -ne $PythonVersion retry net is a good safety valve.
Main notes (non-blocking):
Supply-chain maintenance: Install-WoaOpenSsl downloads a ~220 MB third-party MSI (slproweb.com) and installs it with a pinned SHA256 — good hygiene (deterministic, verified). But pinning a specific version (3_6_4) means (a) the pin must be re-verified against the vendor's release page on every bump, and (b) the install fires a large download whenever WoA has no OpenSSL laid out, even before it's known the source build will be triggered. A comment pointing at how the hash was obtained + when to refresh it (and ideally a -SkipWoAOpenSSL escape hatch) would reduce future-maintainer friction. The Write-Warn on mismatch is correct (refuses to install).
Naming/side-effect: Test-NativeArm64PythonAvailableinstalls an interpreter when none is found. The name implies a pure predicate; the side effect is documented and the find-first makes the common case cheap, but a name like Ensure-NativeArm64Python would set the right expectation.
Install-WoaOpenSsl scope: it runs unconditionally inside Install-Dependencies for arm64 hosts. The early-return when OPENSSL_DIR/an existing layout is found mitigates the common case, but a user who has no OpenSSL and doesn't need cryptography source builds (e.g. only wants an interpreter fix) still pulls the ~220 MB. Acceptable, but worth noting the check-first structure is what keeps this bounded.
No automated tests (installer PS1 isn't in the test harness) — the logic is intricate enough (arch preference matrix, venv retry, MSI install + path discovery) that a manual WoA validation pass with both uv python find cache states (empty vs primed) is warranted before merge.
The arch-detection scoping comment (functions must be registered before the call) and the emulation warning surfaced after venv creation are thoughtful touches. Approve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
comp/cliCLI entry point, hermes_cli/, setup wizardP2Medium — degraded but workaround existsplatform/windowsNative Windows-specific behavior or breakagesweeper:risk-compatibilitySweeper risk: may break existing users, config, migrations, defaults, or upgradessweeper:risk-platform-windowsSweeper risk: may break or behave differently on native Windowstype/perfPerformance improvement or optimization
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Replaces x64-based emulation with native arm64-based Hermes (by virtue of being a python package and changing the interpretor). Arm64-native builds will give better performance compared to emulated counterparts.
Related Issue
Fixes #95569
Type of Change
Changes Made
Concrete changes made to
install.ps1Not sure if/how these will translate to good dev UX, but end-users should be fine
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs