Feat/arm64desktop

[rikled]

No description provided.

[chrip]

Review — #114 "Feat/arm64desktop"

Note: I built this before reviewing — checked out feat/arm64desktop in the core submodule and ran the real Docker core build on a native linux/arm64 host. It compiled and linked cleanly ([1140/1140], x2t + libx2tlib.so, exit 0, ~3.7 min, no OOM) before I read a line. So the notes below sit on top of a confirmed-good build.

What it does

1. .docker/core.bake.Dockerfile — selects the Ubuntu base by PRODUCT (server → 22.04; desktop → 24.04-on-arm / 22.04-on-amd) and TARGETARCH; moves TARGETARCH to a global ARG.
2. Desktop 3rd-party scripts (openssl, v8, boost, cef, icu, qt) — arch-aware toolchain/target selection via a new nc.target_arch() / nc.is_arm64() helper in build_3rdparty_common.py.

Scope caveat

The build above and all CI in this repo only exercise the Dockerfile change — both the Linux docker build and build-windows.yml (x64-only) use the vcpkg+cmake path and never run the Common/3dParty/*/nc-build.* scripts. Those target the external desktop-editors arm64 pipeline, so findings #1 and #3–#8 rest on inspection, not a green build.

---

Findings (most severe first)

1. Common/3dParty/icu/nc-build.bat:20 — arch detection contradicts its own stated priority.
Line 18 honors the caller's VSCMD_ARG_TGT_ARCH, but line 20 unconditionally overwrites TGTARCH with PROCESSOR_ARCHITEW6432 whenever it's defined. The comment says VSCMD wins with host as fallback; the code makes the host win. On an arm64 host with emulated-x64 cmd (PROCESSOR_ARCHITEW6432=ARM64) and an explicit VSCMD_ARG_TGT_ARCH=x64, ICU builds arm64 for an x64 target. Fix: guard line 20 with if not defined VSCMD_ARG_TGT_ARCH.

2. .docker/core.bake.Dockerfile:108-116 — jammy clang-13 apt repo added onto the noble desktop base.
core-base unconditionally adds apt.llvm.org/jammy/ llvm-toolchain-jammy-13 and installs clang-13/libc++-13, but the desktop-arm64 base is ubuntu:24.04 (noble) — which the comment itself says clang-13 is not available on. On the desktop-arm64 path this pins jammy packages onto noble (mismatched libc/libstdc++, or a 404). The server path (always 22.04) is unaffected, so this bites exactly the new path the PR builds toward.

3. Common/3dParty/qt/nc-build.py — removed pip install --upgrade aqtinstall, but still calls python -m aqt install-qt.
aqt/aqtinstall appears nowhere else in the repo (no requirements file, no base image, no other pip step). If the build environment doesn't already provide aqt, the qt fetch fails with No module named aqt; and per the file's own comment, a stale preinstalled aqt can't resolve newer Qt (6.11.x). Please confirm the external pipeline installs aqt.

4. Common/3dParty/v8/nc-build.py:528 — version.startswith("13.") can dereference None.
Inside the re-guarded if nc.is_linux() and targetarch == "arm64" block, if clang --version lacks an X.Y.Z triple, version is None (line 526) and line 528 raises AttributeError instead of the intended abort_op("Need clang 13…"). Live on the linux-arm64 desktop path.

5. Common/3dParty/v8/nc-build.py:72 — Path.write_text(..., newline="\n") requires Python ≥3.10.
patch_vs_toolchain() runs unconditionally; the write branch is taken on a fresh checkout (needle matches). On a <3.10 interpreter it raises TypeError: unexpected keyword argument 'newline', aborting the v8 build — masked on re-runs. The external pipeline's Python version isn't pinned, so worth confirming.

6. Common/3dParty/openssl/nc-build.py:95 — dead diagnostic perl configdata.pm --dump.
Output is discarded (no artifact); it only adds a fallible perl subprocess between Configure and nmake, and run_command treats a non-zero exit as fatal — so a diagnostic-only step can abort a good build. Remove it or gate behind a verbose flag. (Minor: the file also lost its trailing newline at line 131.)

7. Altitude — arch/toolchain selection is re-derived in ~6 places from disagreeing signals.
target_arch() (python, reads VSCMD_ARG_TGT_ARCH) vs icu/nc-build.bat (reads PROCESSOR_ARCHITEW6432/PROCESSOR_ARCHITECTURE), plus per-tool mappers get_cpu / boost_msvc_arch / openssl_windows_target / cef_platform. They can and already do diverge (see #1). Consider centralizing the canonical arch and the per-toolchain mapping so one build can't silently mix arches.

8. Altitude — patch_vs_toolchain() patches upstream V8 by fragile in-place substring replace, warn-and-continues on no-match.
A whitespace/wording drift in a future V8 pin silently skips the patch, resurfacing as a cryptic gn gen TypeError downstream. Prefer a real .patch applied through the existing apply_patches() machinery (like win_toolchain.patch) so a miss is loud/fatal.

Secondary notes

• Bake wiring gap: build/docker-bake.hcl sets neither PRODUCT nor platforms on the core target, so the new desktop/arm64 path is unreachable via docker buildx bake core today (defaults to server/host-arch). Not a bug — just noting the new path is unexercised until wired.
• Common/3dParty/boost/nc-build.py:88 has a pre-existing Python implicit string-concat ("/c" "bootstrap.bat" → "/cbootstrap.bat") sitting in the reworked Windows hunk. Suspect, worth a glance since this PR enables Windows boost — though not introduced here.
• Refuted: the global ARG TARGETARCH does not break FROM resolution — confirmed by the successful arm64 build.

Process

PR body is empty; AGENTS.md asks that PRs disclose AI tool use and that AI-assisted commits carry an Assisted-by: trailer. Worth checking before merge.

---

Bottom line: The Dockerfile change is sound and verified to build on arm64. The desktop-script changes are reasonable in direction but carry the issues above (#1, #4, #5 are real bugs; #2, #3 are environment landmines on the desktop-arm64 path), and none are covered by CI — so they need manual desktop-pipeline testing or the fixes above before relying on them.

Assisted-by: ClaudeCode:claude-opus-4-8

[rikled]

Thank you very much for the review. I fixed 1, 6 and 8.
Comments for the others:

• 2: is something we used already before. Worth taking a closer look, but unrelated to this PR,
• 3: I made sure that it gets installed beforehand,
• 4: also unrelated, new PR makes sense here,
• 5: obsolete because I fixed 8,
• 7: already consolidated as much as I could, but the build processes need different arguments each time.

[chrip]

Clean improvements after review. LGTM.