andodeki
bd01604e65
Phase 1 of NIGIG_PDF_FEATURE_PARITY_PLAN.md. Findings verified by running them, written up in REVIEWS/PDF_PARITY_PHASE1_STATUS.md. Three of the five exit criteria are met: the workspace is green on the new rev (pdf 606 passing), both pins are already at 5efe6e24c, and the upstream baseline is documented - libs/pdf_parse is 4,575 lines with no save/write path at all, against nigig-pdf's 25,845 lines with writing, encryption, signatures, structure tree and transparency. nigig-pdf supersedes both libs/pdf_parse and widgets/src/pdf_view.rs; nothing in either is a capability we lack. The remaining two criteria need fork work this repo cannot do. WHY THE UI SUITE CANNOT PASS YET The six #[ignore] markers were removed from pdf-makepad/tests/ui.rs and the docs now claim the suite runs without a Studio hub. The markers went but the tests did not start passing - TEST_TARGET=pdf-ui was simply red. Three layers, each found by fixing the one in front of it: 1. studio/hub/src/build_manager.rs:398 spawns the build with `sh -lc`. The -l makes it a LOGIN shell, which discards the inherited PATH and rebuilds it from /etc/profile, where ~/.cargo/bin does not appear. cargo is not found and the child exits 127 in 0.4s. This breaks any rustup-based CI, not just this sandbox. `sh -c`, or resolving cargo through the CARGO env var, would fix it. 2. Past that the build runs (88s) and the failure becomes 101. libs/makepad_test sets MAKEPAD=headless for the child, but platform/src/os/linux/windowing_backend.rs only knows X11 and Wayland - there is no headless backend and the env var is not consulted. The app selects X11, finds no display, and segfaults (139). This is the real Phase 1 fork task: "terminal/standalone mode" needs a backend, not just an env var the harness sets. 3. Under xvfb-run the app starts properly and OpenGL initialises, so the binary is fine - but the hub spawns its child outside that display. The markers are restored, with a reason pointing at the status document. A red suite everyone knows to disregard stops reporting the next real regression, which is strictly worse than an explicit skip. Also fixes a latent build break this exposed: the fork's app_main! macro expands to #[cfg(native_activity)], a cfg this crate never declares, which is a hard error under -D warnings. Declared as expected-but-unset via [lints.rust] check-cfg rather than silencing unexpected_cfgs wholesale, which would also hide our own typos. One genuine improvement on this rev: pdf-makepad now builds in release inside the workspace. That was previously blocked by a Makepad os::linux feature-unification bug. TEST_TARGET=pdf 606 passing, TEST_TARGET=pdf-ui 651 passing + 6 ignored.
161 lines
6.3 KiB
Markdown
161 lines
6.3 KiB
Markdown
# Phase 1 status — fork sync + makepad_test enablement
|
||
|
||
Against `NIGIG_PDF_FEATURE_PARITY_PLAN.md` §1 Phase 1. Everything below was
|
||
verified by running it, not by reading code.
|
||
|
||
Fork rev pinned at time of writing: `5efe6e24c`.
|
||
|
||
## Exit criteria
|
||
|
||
| Criterion | State | Evidence |
|
||
|---|---|---|
|
||
| Workspace green on the new rev | ✅ | `TEST_TARGET=pdf` **606 passing** |
|
||
| `rev =` bumped in `pdf-makepad` + `map` | ✅ | both already at `5efe6e24c` |
|
||
| Baseline the gap list vs upstream `libs/pdf_parse` + `pdf_view.rs` | ✅ | §2 below |
|
||
| `cargo test -p nigig-pdf-makepad --test ui` on the desktop, no Studio | ❌ | **blocked, see §3** |
|
||
| Same suite on an Android device | ❌ | no device in this environment |
|
||
|
||
Two of five criteria are outside what this sandbox can deliver. §3 states
|
||
precisely where the wall is and what the fix looks like, so the fork work can
|
||
be picked up without rediscovering it.
|
||
|
||
## 1. What changed since the last PDF session
|
||
|
||
The fork moved from `a79f0dce` to `5efe6e24c`, and the six
|
||
`#[ignore = "needs a Makepad Studio hub"]` markers were removed from
|
||
`pdf-makepad/tests/ui.rs`. The module docs now claim the suite runs
|
||
"without a Studio hub".
|
||
|
||
**The markers were removed but the tests do not pass.** They fail, which is a
|
||
worse state than being ignored: `TEST_TARGET=pdf-ui` is red, so the gate that
|
||
is supposed to protect the widget now fails for an environmental reason and
|
||
will be tuned out.
|
||
|
||
That is not a criticism of the fork work — the failure mode genuinely did
|
||
change, and it changed for the better (see §3). It just is not finished.
|
||
|
||
## 2. Baseline: upstream PDF code vs nigig-pdf
|
||
|
||
The plan asks for this so we can demonstrate nigig-pdf has superseded the
|
||
upstream code.
|
||
|
||
| | upstream `libs/pdf_parse` | `nigig-pdf` |
|
||
|---|---|---|
|
||
| Source | 4,575 lines | **25,845 lines** |
|
||
| Writing a PDF | ✗ none — no `save`, `write` or `serialize` entry point exists | ✓ `writer.rs`, `incremental.rs`, `save.rs` |
|
||
| Encryption | ✗ | ✓ RC4 40/128, AES-128, AES-256 (ADR 0005) |
|
||
| Signatures | ✗ | ✓ read + byte-range integrity (ADR 0010) |
|
||
| Structure tree / PDF-UA | ✗ | ✓ `structure.rs` (ADR 0011) |
|
||
| Transparency, ICC, functions | ✗ | ✓ `transparency.rs`, `icc.rs`, `function.rs` (ADRs 0006, 0009) |
|
||
| Forms | partial | ✓ edit, actions, validation (ADRs 0003, 0012) |
|
||
| Annotations | partial | ✓ edit + save (ADR 0004) |
|
||
|
||
`widgets/src/pdf_view.rs` is 1,161 lines and view-only: selection, zoom, page
|
||
caching. `pdf-makepad` covers that plus form interaction, typed host actions
|
||
and event routing.
|
||
|
||
**Conclusion: nigig-pdf supersedes both.** Nothing in `libs/pdf_parse` or
|
||
`pdf_view.rs` is a capability we lack. This satisfies the plan's baseline
|
||
requirement.
|
||
|
||
## 3. Why the UI suite cannot pass here — three layers, in order
|
||
|
||
Each was found by fixing the one in front of it. The first two are real bugs
|
||
worth fixing in the fork; the third is a genuine environment limit.
|
||
|
||
### Layer 1 — `sh -lc` loses `cargo` (exit 127)
|
||
|
||
`studio/hub/src/build_manager.rs:398` takes a "direct stdio run" path that
|
||
spawns the build through:
|
||
|
||
```rust
|
||
"/bin/sh".to_string(), vec!["-lc".to_string(), script]
|
||
```
|
||
|
||
`-l` makes it a **login** shell, which discards the inherited `PATH` and
|
||
rebuilds it from `/etc/profile`. A rustup toolchain lives in
|
||
`~/.cargo/bin`, which is not on that path:
|
||
|
||
```
|
||
$ /bin/sh -c 'command -v cargo' → /home/user/.cargo/bin/cargo
|
||
$ /bin/sh -lc 'command -v cargo' → not found
|
||
$ /bin/sh -lc 'echo $PATH' → /usr/local/bin:/usr/bin:/bin:...
|
||
```
|
||
|
||
So `cargo` is not found and the child exits **127** before any build starts —
|
||
in 0.4 s, which is the tell.
|
||
|
||
This hits *any* CI or container using a rustup toolchain, not just this
|
||
sandbox. **Suggested fix in the fork:** use `sh -c` rather than `sh -lc`, or
|
||
resolve `cargo` to an absolute path (`CARGO` env var, which cargo sets for
|
||
its children) instead of relying on the login shell's `PATH`.
|
||
|
||
Worked around here by symlinking `cargo` and `rustc` into `/usr/local/bin`.
|
||
|
||
### Layer 2 — no headless backend exists on Linux (exit 101 → 139)
|
||
|
||
With `cargo` reachable the build runs (88 s) and the failure becomes **101**.
|
||
|
||
`libs/makepad_test/src/runtime.rs:112` sets `MAKEPAD=headless` for the child.
|
||
But `platform/src/os/linux/windowing_backend.rs` only knows two backends:
|
||
|
||
```rust
|
||
pub enum WindowingProtocol { X11, Wayland }
|
||
```
|
||
|
||
There is no headless variant, and `MAKEPAD=headless` is not consulted in the
|
||
selection. Running the app with it and no display:
|
||
|
||
```
|
||
Selected: X11 backend
|
||
Reason: Default fallback (no display variables set)
|
||
exit=139 ← SIGSEGV
|
||
```
|
||
|
||
It picks X11, finds no display, and segfaults. **This is the substantive
|
||
Phase 1 fork work**: "terminal/standalone mode" as the plan calls it needs an
|
||
actual headless backend in `windowing_backend.rs`, not just an env var the
|
||
test harness sets.
|
||
|
||
### Layer 3 — the sandbox has no display
|
||
|
||
Under `xvfb-run` the app starts properly and OpenGL initialises:
|
||
|
||
```
|
||
DISPLAY: :99 → Selected: X11 backend
|
||
Makepad GL: vendor="Mesa" renderer="llvmpipe (LLVM 19.1.7)" ...
|
||
```
|
||
|
||
The binary is fine. But the hub spawns its own child process outside the
|
||
`xvfb-run` wrapper's inherited display in a way I could not thread through,
|
||
so the suite still reports 101. Past layer 2 this stops mattering, because a
|
||
real headless backend needs no display at all.
|
||
|
||
## 4. Verified state
|
||
|
||
```
|
||
TEST_TARGET=pdf 606 passing ✅
|
||
TEST_TARGET=pdf-ui 6 failing ❌ (environmental, layers 1–3)
|
||
```
|
||
|
||
One genuine improvement worth recording: **`pdf-makepad` now builds in
|
||
release inside the workspace.** That was previously blocked by a Makepad
|
||
`os::linux` feature-unification bug and is fixed on this rev
|
||
(`cargo build --release -p nigig-pdf-makepad` succeeds in 1 m 54 s).
|
||
|
||
## 5. Recommendation
|
||
|
||
Phase 1 is **not complete**, and the remaining work is in the Makepad fork,
|
||
not in `nigig-org`:
|
||
|
||
1. Fix `sh -lc` → `sh -c` in `build_manager.rs` (small, unblocks every
|
||
rustup-based CI).
|
||
2. Add a real headless backend to `windowing_backend.rs` honouring
|
||
`MAKEPAD=headless`.
|
||
3. Then Android mode, which the plan already scopes.
|
||
|
||
Until (2) exists, `pdf-makepad/tests/ui.rs` cannot pass anywhere without a
|
||
display. **The six tests should be `#[ignore]`d again with a reason pointing
|
||
at this document** rather than left failing — a red suite that everyone knows
|
||
to disregard is worse than an explicitly skipped one, because it hides the
|
||
next real regression.
|