Files
ElyOffice/tasks/reviews/C01.2a-r2.md
T

3.4 KiB
Raw Blame History

FAIL

Checks run: cargo check --workspace, cargo clippy --workspace --all-targets and cargo fmt --check are all clean. cargo test --workspace passes 5 tests. tests/scripts/bundle-macos.sh, desktop-files.sh, installer-windows.sh, appimage-pins.sh and apps-start.sh all pass, and none is skipped. Every file is under 500 lines.

Round 1 defects:

  1. installer.nsi, VCRUNTIME140.dll: fixed. .cargo/config.toml sets +crt-static for cfg(all(windows, target_env = "msvc")). I ran cargo check --target x86_64-pc-windows-msvc -p ely-platform -v, and every target rustc call it prints carries -C target-feature=+crt-static. gpui_windows loads no other non-system DLL at link time. nvapi/amd_ags are optional LoadLibrary calls, and D3DCompileFromFile exists only under debug_assertions.
  2. appimage.sh, missing shared libraries: fixed. linuxdeploy 1-alpha-20251107-1 is pinned for both architectures, and both hashes match the downloaded assets (appimage-pins.sh). Its source (src/core/elf_file.cpp:244-250, src/core/appdir.cpp:372-374) throws DependencyNotFoundError on an ldd "not found" line and fails the deploy, so the comment at line 78 is accurate.

New defects:

  1. packaging/linux/appimage.sh:70-76 — Lines 72, 74, 75 and 76, and the two usr/share directories created at line 70, do work that linuxdeploy redoes on line 79, so their output is never used. That is dead code (rule 6). Per the linuxdeploy source: deployDesktopFile copies the --desktop-file into usr/share/applications/; deployIcon copies the --icon-file into usr/share/icons/hicolor/<its real size>/apps/; symlinkFile removes the root .desktop/.png and replaces them with symlinks; and the AppRun symlink is created the same way when none exists. The hand-made copy at line 74 is also wrong whenever the icon is not 256×256. Failing scenario: assets/icons/ely-doc.png is delivered at 512×512. Line 74 puts it under hicolor/256x256/apps, and linuxdeploy adds a second copy under hicolor/512x512/apps. The AppImage then ships a 512 px image declared as 256 px, and desktop integration installs that copy into the user's 256×256 icon theme directory. Fix: keep only the desktop entry that linuxdeploy reads (line 73, or write it to $work). Delete the other manual copies and the AppRun link, and let linuxdeploy place them.

  2. tests/scripts/installer-windows.sh:2 — The comment has eleven words ("The Windows installer builds, keeps user data, needs no runtime DLL."). Rule 5 caps comments at ten words. Failing scenario: review.sh and the rule-5 check reject the file as written. Fix: shorten it to ten words or fewer.

  3. tests/scripts/installer-windows.sh:32 — The check that the C runtime is linked statically is a plain substring grep over the whole file. It cannot tell whether the flag applies to the msvc target. Failing scenario: someone comments the line out (# rustflags = ["-C", "target-feature=+crt-static"]) or moves it under [target.x86_64-pc-windows-gnu]. The test still prints "needs no runtime DLL", while an msvc build again imports VCRUNTIME140.dll and fails to start on a clean Windows 11 machine. Fix: parse .cargo/config.toml (for example with python3 -c 'import tomllib…') and assert that target."cfg(all(windows, target_env = \"msvc\"))".rustflags contains target-feature=+crt-static. An alternative is to assert that the flag appears in cargo check --target x86_64-pc-windows-msvc -v output.