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) <noreply@anthropic.com>
2026-10-07 03:00:29 +00:00 · 2026-04-13 21:51:19 -04:00 · 2026-04-13 21:51:19 -04:00 · aa5e3f600d
commit aa5e3f600d
parent e5b7bb1909
1 changed files with 534 additions and 0 deletions
--- a/docs-internal/plans/vendor-graphviz-sys.md
+++ 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<Vec<u8>>
+  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 <gvc/gvplugin.h>
+
+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<Vec<u8>, 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<Vec<u8>> {
+    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<GraphOutputFormat> 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("<svg"));
+        assert!(svg_str.contains("</svg>"));
+    }
+}
+```
+
+### 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.