From aa5e3f600d7b3a4cd92c9780e3d0b7a8ee42dcd6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 13 Apr 2026 21:51:19 -0400 Subject: [PATCH] plan: vendor Graphviz C source via fabro-graphviz-sys FFI crate Detailed implementation plan for replacing the shell-out to `dot` with a vendored Graphviz C library compiled via the `cc` crate. Covers crate structure, build.rs approach, pre-generated parser files, FFI wrapper, caller updates, OpenAPI spec changes, and testing strategy. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs-internal/plans/vendor-graphviz-sys.md | 534 +++++++++++++++++++++ 1 file changed, 534 insertions(+) create mode 100644 docs-internal/plans/vendor-graphviz-sys.md diff --git a/docs-internal/plans/vendor-graphviz-sys.md b/docs-internal/plans/vendor-graphviz-sys.md new file mode 100644 index 000000000..e50d0024d --- /dev/null +++ b/docs-internal/plans/vendor-graphviz-sys.md @@ -0,0 +1,534 @@ +# Plan: Vendor Graphviz via `fabro-graphviz-sys` FFI Crate + +## Goal + +Remove the system dependency on the `dot` binary by vendoring the Graphviz C source code into a new `fabro-graphviz-sys` crate. Replace the `Command::new("dot")` shell-out in `render_dot()` with direct FFI calls to the statically compiled library. Drop PNG support (SVG only). + +## Approach: `cc` crate compiling vendored C source + +Use the `cc` crate in `build.rs` to compile the Graphviz C source files directly. This avoids requiring CMake, pkg-config, or a system-installed Graphviz at build time. The Graphviz source needs flex/bison-generated files (grammar.c, scan.c, htmlparse.c) and Python-generated files (colortbl.h, entities.h), which we pre-generate once and commit alongside the vendored source. + +## Step 0: Pre-generation of parser and table files + +Before vendoring, generate the required files from a temporary CMake build of Graphviz 14.1.5: + +```bash +# One-time setup (developer machine only, not part of build.rs) +git clone --depth 1 --branch 14.1.5 https://gitlab.com/graphviz/graphviz.git /tmp/graphviz-src +cd /tmp/graphviz-src +mkdir build && cd build +cmake .. -DGRAPHVIZ_CLI=OFF -DBUILD_SHARED_LIBS=OFF +make -j$(nproc) cgraph common_obj # generates the files we need +``` + +Copy the generated files into the vendored tree: +- `build/lib/cgraph/grammar.c` and `build/lib/cgraph/grammar.h` (from grammar.y) +- `build/lib/cgraph/scan.c` (from scan.l) +- `build/lib/common/htmlparse.c` and `build/lib/common/htmlparse.h` (from htmlparse.y) +- `build/lib/common/common/colortbl.h` (from make_colortbl.py) +- `build/lib/common/common/entities.h` (from entities.py) +- `build/builddate.h` (generated by CMake) + +These pre-generated files are committed and never regenerated during normal `cargo build`. + +## Step 1: Vendor the Graphviz C source + +Create `lib/crates/fabro-graphviz-sys/vendor/graphviz-14.1.5/` containing only the directories needed: + +``` +vendor/graphviz-14.1.5/ + lib/ + cdt/ -- container data types (all .c/.h) + cgraph/ -- graph library (all .c/.h + pre-generated grammar.c, grammar.h, scan.c) + common/ -- common utils (all .c/.h + pre-generated htmlparse.c, htmlparse.h, colortbl.h, entities.h) + dotgen/ -- dot layout algorithm (all .c/.h) + gvc/ -- graphviz context (all .c/.h) + label/ -- label placement (all .c/.h) + ortho/ -- orthogonal edge routing (all .c/.h) + pack/ -- packing (all .c/.h) + pathplan/ -- path planning (all .c/.h) + xdot/ -- xdot format (all .c/.h) + util/ -- utility headers (header-only, no .c needed except arena.c, list.c, xml.c, base64.c, random.c, gv_find_me.c, gv_fopen.c) + plugin/ + core/ -- core renderers including SVG (gvrender_core_svg.c, gvplugin_core.c, gvloadimage_core.c) + dot_layout/ -- dot layout plugin (gvlayout_dot_layout.c, gvplugin_dot_layout.c) + generated/ -- pre-generated files (grammar.c, grammar.h, scan.c, htmlparse.c, htmlparse.h, colortbl.h, entities.h, builddate.h) +``` + +Files to EXCLUDE from vendor: +- All CMakeLists.txt, Makefile.am, README, man pages, .pc.in files +- All test files (test_*.c) +- Libraries not needed: neatogen, fdpgen, sfdpgen, circogen, twopigen, osage, patchwork, sparse, expr, gvpr, etc. +- Plugins not needed: pango, gd, cairo, rsvg, webp, quartz, etc. +- Parser source files (grammar.y, scan.l, htmlparse.y) -- replaced by pre-generated .c +- Python scripts (make_colortbl.py, entities.py) -- replaced by pre-generated .h + +Include a `vendor/graphviz-14.1.5/LICENSE` file (EPL-2.0). + +## Step 2: Create `fabro-graphviz-sys` crate + +### Crate structure + +``` +lib/crates/fabro-graphviz-sys/ + Cargo.toml + build.rs + src/ + lib.rs -- safe Rust wrapper: render_dot_to_svg(source: &str) -> Result> + wrapper.h -- #include directives for bindgen (not used -- we write manual FFI declarations) + vendor/ -- vendored Graphviz source (from Step 1) +``` + +### Cargo.toml + +```toml +[package] +name = "fabro-graphviz-sys" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Vendored Graphviz C library compiled via cc, exposing DOT-to-SVG rendering" +links = "graphviz" + +[lib] +doctest = false + +[lints] +workspace = true + +[build-dependencies] +cc = "1" +``` + +The `links = "graphviz"` key prevents multiple crates from linking the same native library. + +### build.rs + +The build script uses `cc::Build` to compile each Graphviz library as a static archive. Key aspects: + +1. **Compile order**: cdt, cgraph, pathplan, xdot, label, pack, ortho, common, dotgen, gvc, plugins +2. **Include paths**: vendor root `lib/`, plus each library's own directory, plus `generated/` for pre-generated headers +3. **Defines**: + - `PACKAGE_VERSION="14.1.5"` (used by gvcontext.c) + - No `ENABLE_LTDL` (disables dynamic plugin loading -- builtins only) + - No `HAVE_EXPAT` (disables expat XML parsing -- not needed for SVG output) + - `DEFAULT_DPI=96` + - `GVPLUGIN_CONFIG_FILE=""` + - `BROWSER=""` + - Platform-specific: `DARWIN` on macOS +4. **Compiler flags**: `-std=c11`, `-fno-common` on macOS, suppress warnings for vendored code (`-w`) +5. **Exclude files**: `args.c` from common (CLI argument parsing, not needed), `gvtool_tred.c` from gvc (CLI tool), `gvevent.c` from gvc (GUI events) +6. **Static plugin registration**: Compile a small C file (`builtins.c`) that defines the `lt_preloaded_symbols` array referencing `gvplugin_core_LTX_library` and `gvplugin_dot_layout_LTX_library` + +### builtins.c (in crate root, not vendored) + +```c +#include + +extern gvplugin_library_t gvplugin_dot_layout_LTX_library; +extern gvplugin_library_t gvplugin_core_LTX_library; + +const lt_symlist_t lt_preloaded_symbols[] = { + { "gvplugin_dot_layout_LTX_library", (void*)&gvplugin_dot_layout_LTX_library }, + { "gvplugin_core_LTX_library", (void*)&gvplugin_core_LTX_library }, + { 0, 0 } +}; +``` + +### config.h (in crate root, hand-written) + +A minimal `config.h` that Graphviz source expects: + +```c +#pragma once + +// No dynamic plugin loading +// #undef ENABLE_LTDL + +// No expat XML parser +// #undef HAVE_EXPAT + +// No optional libraries +// #undef HAVE_LIBZ +// #undef HAVE_PANGOCAIRO +// etc. + +// Platform features available on macOS and Linux +#define HAVE_DRAND48 1 +#define HAVE_SRAND48 1 +#define HAVE_SETENV 1 +#define HAVE_STRCASESTR 1 + +// Build configuration +#define PACKAGE_VERSION "14.1.5" +#define DEFAULT_DPI 96 +#define GVPLUGIN_CONFIG_FILE "" +#define BROWSER "" + +#ifdef __APPLE__ +#define DARWIN 1 +#endif +``` + +### src/lib.rs -- Safe Rust API + +The `lib.rs` exposes a single safe function. The crate allows `unsafe_code` via a crate-level lint override since FFI requires it. + +```rust +#![allow(unsafe_code)] + +use std::ffi::{CStr, CString}; +use std::os::raw::{c_char, c_int, c_uint}; +use std::ptr; + +// Manual FFI declarations (no bindgen needed -- we only use 6 functions) +extern "C" { + fn gvContext() -> *mut std::ffi::c_void; + fn agmemread(cp: *const c_char) -> *mut std::ffi::c_void; + fn gvLayout(gvc: *mut std::ffi::c_void, g: *mut std::ffi::c_void, engine: *const c_char) -> c_int; + fn gvRenderData(gvc: *mut std::ffi::c_void, g: *mut std::ffi::c_void, format: *const c_char, result: *mut *mut c_char, length: *mut c_uint) -> c_int; + fn gvFreeRenderData(data: *mut c_char); + fn gvFreeLayout(gvc: *mut std::ffi::c_void, g: *mut std::ffi::c_void) -> c_int; + fn agclose(g: *mut std::ffi::c_void) -> c_int; + fn gvFreeContext(gvc: *mut std::ffi::c_void) -> c_int; +} + +/// Render a DOT source string to SVG bytes using the vendored Graphviz library. +/// +/// Creates a fresh `gvContext` per call for thread safety. +/// Returns the SVG output as a byte vector, or an error message. +pub fn render_dot_to_svg(dot_source: &str) -> Result, String> { + let source = CString::new(dot_source).map_err(|e| format!("invalid DOT source: {e}"))?; + let engine = CString::new("dot").unwrap(); + let format = CString::new("svg").unwrap(); + + // SAFETY: All pointers are checked for null. Resources are freed in reverse order. + // A fresh gvContext is created per call, so concurrent calls are safe. + unsafe { + let gvc = gvContext(); + if gvc.is_null() { + return Err("failed to create Graphviz context".into()); + } + + let graph = agmemread(source.as_ptr()); + if graph.is_null() { + gvFreeContext(gvc); + return Err("failed to parse DOT source".into()); + } + + let rc = gvLayout(gvc, graph, engine.as_ptr()); + if rc != 0 { + agclose(graph); + gvFreeContext(gvc); + return Err("Graphviz layout failed".into()); + } + + let mut buf: *mut c_char = ptr::null_mut(); + let mut len: c_uint = 0; + let rc = gvRenderData(gvc, graph, format.as_ptr(), &mut buf, &mut len); + if rc != 0 || buf.is_null() { + gvFreeLayout(gvc, graph); + agclose(graph); + gvFreeContext(gvc); + return Err("Graphviz render failed".into()); + } + + let result = std::slice::from_raw_parts(buf as *const u8, len as usize).to_vec(); + + gvFreeRenderData(buf); + gvFreeLayout(gvc, graph); + agclose(graph); + gvFreeContext(gvc); + + Ok(result) + } +} +``` + +**Thread safety**: Each call creates and destroys its own `gvContext`, `graph`, and layout. No shared mutable state. The server's `spawn_blocking` pattern remains correct. + +## Step 3: Wire `fabro-graphviz-sys` into `fabro-graphviz` + +### Update `fabro-graphviz/Cargo.toml` + +Add dependency: +```toml +fabro-graphviz-sys = { path = "../fabro-graphviz-sys" } +``` + +Remove: nothing new needed. + +### Update `fabro-graphviz/src/render.rs` + +1. **Remove `GraphFormat::Png`** -- only SVG is supported. Simplify the enum to a unit struct or remove entirely. Since `GraphFormat` is used by the server, simplify to always-SVG. + +2. **Replace `render_dot()`**: + +```rust +pub fn render_dot(source: &str) -> anyhow::Result> { + let styled_source = inject_dot_style_defaults(source); + let raw = fabro_graphviz_sys::render_dot_to_svg(&styled_source) + .map_err(|e| anyhow::anyhow!("Graphviz rendering failed: {e}"))?; + Ok(postprocess_svg(raw)) +} +``` + +Key changes: +- Signature changes from `render_dot(source, format)` to `render_dot(source)` (no format param) +- No `Command::new("dot")` -- direct FFI call +- No "not installed" error path +- `postprocess_svg` still runs on the output + +3. **Remove `dot_is_available()`** -- no longer needed. + +4. **Remove `GraphFormat` enum** -- or simplify to a constant. Since the OpenAPI spec and CLI still reference it, keep a simpler version: + +```rust +/// Output format for graph rendering (only SVG is supported). +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum GraphFormat { + #[default] + Svg, +} +``` + +5. **Remove `std::process::Command` import** and related code. + +6. **Remove the `#[expect(clippy::disallowed_methods)]` attribute** on `render_dot` -- no longer calling `Command::new`. + +## Step 4: Update callers of `render_dot` + +### `fabro-server/src/server.rs` + +**`render_graph_bytes()`** (line ~6182): +- Remove `format: GraphFormat` parameter +- Always return `image/svg+xml` content-type +- Call `render_dot(&source)` instead of `render_dot(&source, format)` + +**`render_graph_from_manifest()`** (line ~3593): +- Remove `GraphFormat` matching from `req.format` +- Ignore `format` field in request (or log a deprecation warning if PNG is requested) +- Always render SVG + +**`get_graph()`** (line ~6197): +- Already hardcodes `GraphFormat::Svg` -- just drop the parameter + +**Test `render_graph_from_manifest_returns_svg()`** (line ~7858): +- Remove the `BAD_GATEWAY` early-return guard (line ~7889) +- The test should now always succeed since Graphviz is always available + +### `fabro-server/src/demo/mod.rs` + +- Remove `GraphFormat::Svg` argument from `render_graph_bytes` call + +### `fabro-cli/src/args.rs` + +- Remove `GraphOutputFormat::Png` variant +- Remove `From for GraphFormat` impl (or simplify) +- Remove import of `GraphFormat` if no longer needed + +### `fabro-cli/src/commands/graph.rs` + +- Remove PNG branch from the `match args.format` expression +- Remove `GraphOutputFormat` to `RenderWorkflowGraphFormat` PNG mapping + +### `fabro-server/Cargo.toml` + +- No changes needed (depends on `fabro-graphviz` which now transitively brings in `fabro-graphviz-sys`) + +## Step 5: Update OpenAPI spec + +### `docs/api-reference/fabro-api.yaml` + +**`RenderWorkflowGraphFormat` enum** (line ~2336): +- Remove `png` from the enum, leaving only `svg` +- Or: keep `png` as deprecated (simpler for backward compat), but server ignores it and always returns SVG + +Recommended: remove `png` entirely since this is a breaking change anyway: +```yaml +RenderWorkflowGraphFormat: + type: string + enum: + - svg +``` + +**`/api/v1/graph/render` responses** (line ~187): +- Remove `image/png` response content type +- Remove `502` response (Graphviz is always available now) + +**`/api/v1/runs/{id}/graph` responses** (line ~380): +- Remove `502` response + +After editing the spec: +1. `cargo build -p fabro-api` -- regenerates Rust types +2. `cd lib/packages/fabro-api-client && bun run generate` -- regenerates TypeScript client + +## Step 6: Update diagnostics and doctor + +### `fabro-server/src/diagnostics.rs` + +- Remove `check_dot()` function and all related code (`probe_dot()`, `ProbeOutcome`, `DOT_RE`) +- Remove `dot` from the `tokio::join!` in `run_all()` +- Remove the "System" check section (or repurpose for other checks) + +### `fabro-cli/src/commands/doctor.rs` + +- Remove `dot` from `DEP_SPECS` array -- make it an empty slice or remove the array entirely +- Keep the `check_system_deps` function (it works on any list of deps, currently just `dot`) +- Update tests that reference `dot` in `DEP_SPECS` + +### `fabro-cli/src/commands/install.rs` + +- Remove `choose_graphviz_install()` from `InstallInputSource` trait and both impls +- Remove the `brew install graphviz` block (~lines 1345-1354) +- Remove the `dot_missing` variable and related logic + +## Step 7: Update workspace Cargo.toml + +Add the new crate to the workspace `members` list. Since `lib/crates/*` glob already covers it, no change needed to the members list. + +Add `cc` to workspace dependencies if desired (optional -- can be a direct dependency of `fabro-graphviz-sys`). + +Handle the `unsafe_code = "deny"` workspace lint: the `fabro-graphviz-sys` crate needs an override: + +```toml +# In fabro-graphviz-sys/Cargo.toml +[lints] +workspace = true + +[lints.rust] +unsafe_code = "allow" # FFI requires unsafe +``` + +## Step 8: Platform considerations + +### macOS (primary development platform) +- `-fno-common` flag needed (already in Graphviz CMakeLists) +- `DARWIN` define +- No special linker flags needed for static compilation +- Both x86_64 and aarch64 supported by `cc` crate + +### Linux (CI/production) +- Standard C11 compilation +- No platform-specific defines beyond standard POSIX +- May need `HAVE_SYS_MMAN_H`, `HAVE_SYS_SELECT_H` etc. -- handle in config.h + +### Cross-compilation +- The `cc` crate handles cross-compilation automatically via `CC` env var +- No platform-specific assembly or intrinsics in the subset we compile + +## Testing plan + +### Test 1: Build and regression baseline +- `cargo build --workspace` -- verify compilation +- `cargo nextest run --workspace` -- verify existing tests pass +- `cargo clippy --workspace -- -D warnings` +- `cargo +nightly fmt --check --all` + +### Test 2: FFI smoke test in `fabro-graphviz-sys` +New test in `fabro-graphviz-sys/src/lib.rs`: +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn smoke_test_dot_to_svg() { + let svg = render_dot_to_svg("digraph { a -> b }").unwrap(); + let svg_str = String::from_utf8(svg).unwrap(); + assert!(svg_str.contains("")); + } +} +``` + +### Test 3: `render_dot` produces SVG unconditionally +Update existing test in `fabro-graphviz/src/render.rs`: +- Remove `if !dot_is_available() { return; }` guard +- Test always runs and succeeds +- Add a complex graph test with multiple nodes, edges, subgraphs + +### Test 4: SVG post-processing preserved +Existing `postprocess_svg_removes_white_background` test is sufficient. + +### Test 5: Server API endpoints +Update `render_graph_from_manifest_returns_svg`: +- Remove `BAD_GATEWAY` early-exit guard +- Assert response is always 200 OK with SVG content + +### Test 6: Malformed DOT error handling +New test in `fabro-graphviz-sys`: +```rust +#[test] +fn malformed_dot_returns_error() { + let result = render_dot_to_svg("not valid dot"); + assert!(result.is_err()); +} +``` + +And in `fabro-graphviz`: +```rust +#[test] +fn render_dot_invalid_source_returns_error() { + let result = render_dot("not valid dot {{{"); + assert!(result.is_err()); +} +``` + +### Test 7: Diagnostics/doctor updated +- Update doctor tests to reflect empty `DEP_SPECS` +- Verify diagnostics report no longer includes "dot" check + +### Test 8: OpenAPI spec consistency +- `cargo nextest run -p fabro-server` -- conformance test catches spec/router drift + +### Test 9: Thread safety +New test in `fabro-graphviz-sys`: +```rust +#[test] +fn concurrent_renders_are_safe() { + let handles: Vec<_> = (0..8) + .map(|i| { + std::thread::spawn(move || { + let source = format!("digraph {{ node{i} -> node{} }}", i + 1); + render_dot_to_svg(&source).unwrap() + }) + }) + .collect(); + + for handle in handles { + let svg = handle.join().unwrap(); + assert!(!svg.is_empty()); + } +} +``` + +### Test 10: Reference output comparison +If `dot` is installed on the machine: +1. Before changes: capture `echo "digraph { a -> b }" | dot -Tsvg > reference.svg` +2. After changes: compare SVG structure (not byte-identical, but same node/edge structure) +3. This is a manual verification step, not an automated test + +## Execution order + +1. Create `fabro-graphviz-sys` crate skeleton (Cargo.toml, build.rs, src/lib.rs, config.h, builtins.c) +2. Download and vendor Graphviz 14.1.5 source (subset only) +3. Pre-generate parser files (grammar.c, scan.c, htmlparse.c) and table files (colortbl.h, entities.h) +4. Implement build.rs -- compile all C source with `cc` +5. Implement src/lib.rs -- `render_dot_to_svg()` safe wrapper +6. Verify: `cargo build -p fabro-graphviz-sys` and `cargo nextest run -p fabro-graphviz-sys` +7. Wire into `fabro-graphviz` -- update render.rs, remove PNG +8. Update `fabro-server` -- simplify render_graph_bytes, remove guards +9. Update `fabro-cli` -- simplify args, remove PNG +10. Update OpenAPI spec + regenerate clients +11. Update diagnostics/doctor/install -- remove dot checks +12. Full workspace build + test + clippy + fmt + +## Risk mitigation + +- **Build complexity**: The main risk is getting the `cc` build right. Mitigate by starting with the smallest possible subset (just cdt + cgraph + gvc) and adding libraries incrementally. +- **Missing C files**: Some Graphviz C files may have implicit dependencies we don't immediately see. The compiler will tell us. +- **Pre-generated files**: If the pre-generated parser files don't work with the exact source version, regenerate them from a CMake build of the same tag. +- **Binary size**: Statically compiled Graphviz adds ~2-3 MB. Acceptable for eliminating a system dependency. +- **Thread safety**: Each call creates/destroys its own context. Graphviz has some global state (`graphviz_errors` counter, `emit_once` hash), but the `gvContext()`/`gvFreeContext()` lifecycle resets most of it. For the server use case (rendering independent graphs), this is safe. The concurrent test will verify.