diff --git a/Cargo.lock b/Cargo.lock index 731a30eb3..088e188fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1588,7 +1588,6 @@ dependencies = [ "fabro-devcontainer", "fabro-github", "fabro-graphviz", - "fabro-graphviz-sys", "fabro-hooks", "fabro-http", "fabro-interview", @@ -1611,6 +1610,7 @@ dependencies = [ "fabro-workflow", "futures", "git2", + "graphviz-sys", "httpmock", "indicatif", "insta", @@ -1716,21 +1716,14 @@ name = "fabro-graphviz" version = "0.176.2" dependencies = [ "anyhow", - "fabro-graphviz-sys", "fabro-types", + "graphviz-sys", "nom", "regex", "serde", "thiserror 2.0.18", ] -[[package]] -name = "fabro-graphviz-sys" -version = "0.176.2" -dependencies = [ - "cc", -] - [[package]] name = "fabro-hooks" version = "0.176.2" @@ -2730,6 +2723,14 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "graphviz-sys" +version = "0.1.0" +source = "git+https://github.com/fabro-sh/graphviz-sys#e6ed96592b8b9af8ebc3744a601d3829e5ab3b3c" +dependencies = [ + "cc", +] + [[package]] name = "h2" version = "0.4.13" diff --git a/Cargo.toml b/Cargo.toml index 192b60d63..4d4125de7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,6 +75,7 @@ rust-embed = "8" percent-encoding = "2" minijinja = "2" fabro-http = { path = "lib/crates/fabro-http" } +graphviz-sys = { git = "https://github.com/fabro-sh/graphviz-sys" } [workspace.lints.rust] unsafe_code = "deny" diff --git a/docs-internal/plans/vendor-graphviz-sys.md b/docs-internal/plans/vendor-graphviz-sys.md deleted file mode 100644 index 322fdb648..000000000 --- a/docs-internal/plans/vendor-graphviz-sys.md +++ /dev/null @@ -1,580 +0,0 @@ -# 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 and source files. The exact set of .c files needed depends on what Graphviz 14.1.5 compiles; include all .c/.h initially, then trim if unused (compiler errors will guide which files are required) - 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**: Most defines live in `config.h` (included via `-include config.h`). build.rs should NOT pass `-D` for values already in `config.h`. build.rs only needs: - - `-include config.h` (via `.flag("-include").flag("config.h")` with the crate root in the include path) - - No `ENABLE_LTDL` (disables dynamic plugin loading -- builtins only) - - No `HAVE_EXPAT` (disables expat XML parsing -- not needed for SVG output) -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. All build configuration defines (`PACKAGE_VERSION`, `DEFAULT_DPI`, etc.) live here -- do NOT duplicate them as `-D` flags in build.rs to avoid redefinition warnings. - -```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 workspace denies `unsafe_code`, so the `fabro-graphviz-sys` Cargo.toml overrides it to `"allow"` (see Step 7). No crate-level `#![allow(unsafe_code)]` attribute is needed in `lib.rs` -- the Cargo.toml override suffices. - -```rust - -use std::ffi::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. The server's `spawn_blocking` pattern remains correct. Note: Graphviz has some internal global state (error handlers, string interning). The concurrent test (Test 9) will verify correctness. If it reveals data races, fall back to wrapping `render_dot_to_svg` in a `std::sync::Mutex` -- acceptable since rendering is already offloaded to `spawn_blocking`. - -## 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** entirely. The enum, its `Display` impl (lines 18-25), and the `Png` variant are all unnecessary when only SVG is supported. The callers (`fabro-server`, `fabro-cli`) no longer need to pass a format. If keeping a simpler version is preferred for forward compat: - -```rust -/// Output format for graph rendering (only SVG is supported). -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] -pub enum GraphFormat { - #[default] - Svg, -} -``` - -But removing it entirely is simpler since `render_dot` no longer takes a format parameter. - -5. **Remove imports** no longer used: `std::process::Command`, `std::io::Write` (was for `stdin.write_all`), and `anyhow::bail` (all three `bail!` calls are inside `render_dot` and will be replaced by `anyhow::anyhow!`). Keep `use anyhow` since `render_dot` still returns `anyhow::Result`. - -6. **Remove the `#[expect(clippy::disallowed_methods)]` attribute** on `render_dot` -- `Command::new` is a disallowed method (see `clippy.toml`), and we no longer call it. - -## 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)` -- Change the error status from `BAD_GATEWAY` (502) to `BAD_REQUEST` (400) -- with vendored Graphviz, a render failure means bad DOT input, not a missing external service - -**Imports** (lines 33, 42): -- Remove `RenderWorkflowGraphFormat` from the `fabro_api::types` import list (line 33) -- no longer referenced after removing the format match block -- Remove `use fabro_graphviz::render::GraphFormat;` (line 42) -- no longer used after removing the `format` parameter from `render_graph_bytes` - -**`render_graph_from_manifest()`** (line ~3593): -- Remove the `format` match block (lines 3593-3596) that converts `RenderWorkflowGraphFormat` to `GraphFormat` -- Remove the `format` parameter from the `render_graph_bytes` call (line 3602) -- The `req.format` field still exists in the OpenAPI-generated request struct (as `Option` with only `Svg`), but the handler ignores it since SVG is always produced - -**`get_graph()`** (line ~6197): -- Already hardcodes `GraphFormat::Svg` -- just drop the parameter - -**Test `get_graph_returns_svg()`** (line ~7792): -- Remove the `BAD_GATEWAY` early-return guard (line ~7834) -- The test should now always succeed since Graphviz is always available - -**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 to identity) -- Remove `use fabro_graphviz::render::GraphFormat;` import (line 6) if `GraphFormat` is removed entirely -- Update the doc comment on `Graph(GraphArgs)` (line 938): change `"Render a workflow graph as SVG or PNG"` to `"Render a workflow graph as SVG"` -- Update the `Display` impl for `GraphOutputFormat` (lines 321-328): remove `Png` arm - -### `fabro-cli/src/commands/graph.rs` - -- Remove PNG branch from the `match args.format` expression (lines 53-56). Since only SVG remains, simplify to `format: Some(types::RenderWorkflowGraphFormat::Svg)` (the field is still `Option` in the OpenAPI spec, so we hardcode `Some(Svg)`). -- The JSON output (line 66-70) still references `args.format.to_string()` -- keep as-is since `GraphOutputFormat` retains an `Svg` variant with a working `Display` impl. -- The debug log (line 76-79) references `args.format` -- keep as-is for the same reason. -- The `--format` CLI flag is retained with a single `svg` value. This is intentional forward-compat: if new formats are added later, the flag is already wired. Clap will show `[possible values: svg]`. - -### `fabro-cli/tests/it/cmd/json_global.rs` - -- Remove the `dot_is_available()` helper function (lines 13-20) -- Remove the `if !dot_is_available() { return; }` guard from `graph_json_with_output_reports_file` (line 146) -- Remove the module-level `#[expect(clippy::disallowed_methods)]` attribute (lines 1-4) -- it was only needed because `Command::new("dot")` is called in `dot_is_available()` -- Remove `use std::process::Command;` import (line 6) -- no longer used after removing `dot_is_available()` - -### `fabro-cli/tests/it/cmd/graph.rs` - -- Update the inline snapshot in `help()` test (line 10-72): - - Line 14: Change `"Render a workflow graph as SVG or PNG"` to `"Render a workflow graph as SVG"` - - Line 42: Change `[possible values: svg, png]` to `[possible values: svg]` (or remove `--format` from CLI entirely since SVG is the only option; if kept, clap will still show the possible values) -- Run `cargo insta accept` after verifying the pending snapshot is correct - -### `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`, `parse_version`) -- Remove `dot` from the `tokio::join!` in `run_all()` (line 153) -- Remove the `CheckSection { title: "System", checks: vec![dot] }` block from the `DiagnosticsReport` sections (line 170-172) -- `dot` is the only check in the "System" section, so remove the entire section -- Remove `use std::sync::LazyLock;`, `use regex::Regex;`, `use semver::Version;` imports if no longer used after removing `DOT_RE` and `parse_version` - -### `fabro-cli/src/commands/doctor.rs` - -- Make `DEP_SPECS` an empty slice: `pub(crate) const DEP_SPECS: &[DepSpec] = &[];` -- Remove `DOT_RE` static (line 41-42) and the `DepSpec` fields that reference it -- Keep `DepSpec`, `ProbeOutcome`, `probe_system_deps`, `check_system_deps` functions -- they are generic and still called from `install.rs` pre-flight checks; with an empty slice they become no-ops -- Remove the `parse_version_dot` test (line 665-669) and `parse_version_garbage_returns_none` test (line 672-674) since there is no `DOT_RE` to test. The `parse_version` function itself can stay (it is generic and used by `probe_system_deps`). -- The `spec()` helper (line 704-712) references `&DOT_RE` for its `pattern` field. Since `DOT_RE` is removed, add a test-local static: `static TEST_RE: LazyLock = LazyLock::new(|| Regex::new(r"version (\d+)\.(\d+)\.(\d+)").unwrap());` and update `spec()` to use `&TEST_RE`. The tests `check_system_deps_all_present` (line 715), `check_system_deps_required_missing_is_error` (line 726), `check_system_deps_optional_missing_is_warning` (line 734), and `check_system_deps_outdated_is_warning` (line 742) exercise the generic `check_system_deps` infrastructure and should remain -- only the regex reference changes. - -### `fabro-cli/src/commands/install.rs` - -- Remove `choose_graphviz_install()` from `InstallInputSource` trait (line 418) and both impls: `InteractiveInstallInputSource` (line 442-448) and `NonInteractiveInstallInputSource` (line 679-681) -- Remove the `dot_missing` variable (line 1326-1329) and the `if input_source.choose_graphviz_install(dot_missing).await?` block (lines 1345-1354) -- The `dep_outcomes`/`dep_check` pre-flight section (lines 1324-1325) becomes a no-op with empty `DEP_SPECS`, but keep it so the infrastructure is in place if future system deps are added - -## Step 6b: Update documentation - -Several docs reference Graphviz as a system dependency. Update them to reflect that Graphviz is now bundled: - -- **`docs/reference/cli.mdx`** (line 496): Remove "Requires Graphviz (`dot`) to be installed" from the `graph` command description. Update "SVG or PNG" to "SVG". -- **`docs/api-reference/overview.mdx`** (line 117): Remove the row about `502 Bad Gateway` / "Graphviz `dot` not installed" from the status code table. -- **`docs/changelog/2026-03-11.mdx`** (line 37): Remove "Requires Graphviz `dot` to be installed" and update "SVG or PNG" to "SVG". -- **`docs/administration/troubleshooting.mdx`** (line 21): Remove "Server-side Graphviz availability" from the doctor checks list. - -## 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 `render_dot_svg_or_png_if_graphviz_is_available` in `fabro-graphviz/src/render.rs`: -- Rename to `render_dot_produces_svg` -- Remove `if !dot_is_available() { return; }` guard -- Remove the `dot_is_available()` function (line 138-145) and its `#[expect(clippy::disallowed_methods)]` attribute (lines 134-137) -- Remove the PNG assertion (line 183-184) -- PNG is no longer supported -- Change `render_dot("digraph { a -> b }", GraphFormat::Svg)` to `render_dot("digraph { a -> b }")` (no format param) -- Test always runs and succeeds -- Optionally 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 `get_graph_returns_svg` (line ~7792): -- Remove `BAD_GATEWAY` early-exit guard (line ~7834) -- Assert response is always 200 OK with SVG content - -Update `render_graph_from_manifest_returns_svg` (line ~7858): -- Remove `BAD_GATEWAY` early-exit guard (line ~7889) -- 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` (the `spec()` helper in tests uses `&DOT_RE` -- replace with a test-local regex) -- Verify diagnostics report no longer includes "dot" check -- Verify `graph_json_with_output_reports_file` in `fabro-cli/tests/it/cmd/json_global.rs` no longer skips (the `dot_is_available()` guard is removed) - -### 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, update `json_global.rs` and `graph.rs` test snapshots -10. Update OpenAPI spec + regenerate clients -11. Update diagnostics/doctor/install -- remove dot checks -12. Update documentation (cli.mdx, overview.mdx, troubleshooting.mdx, changelog) -13. Full workspace build + test + clippy + fmt + `cargo insta accept` for snapshot updates - -## 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, string interning in `agstrfree`), and `gvContext()`/`gvFreeContext()` does not fully isolate all of it. The concurrent test (Test 9) will verify whether this causes data races. Mitigation: if races appear, wrap the FFI call in a `std::sync::Mutex` -- this is acceptable since `render_dot_to_svg` is already called via `spawn_blocking` and the rendering latency dwarfs lock contention. diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index 741f76edc..b8654eef2 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -34,7 +34,7 @@ fabro-retro = { path = "../fabro-retro" } fabro-sandbox = { path = "../fabro-sandbox", features = ["daytona"] } fabro-checkpoint = { path = "../fabro-checkpoint" } fabro-graphviz = { path = "../fabro-graphviz" } -fabro-graphviz-sys = { path = "../fabro-graphviz-sys" } +graphviz-sys.workspace = true fabro-validate = { path = "../fabro-validate" } fabro-workflow = { path = "../fabro-workflow" } fabro-server = { path = "../fabro-server" } diff --git a/lib/crates/fabro-cli/src/commands/render_graph.rs b/lib/crates/fabro-cli/src/commands/render_graph.rs index ce1ec4e16..6e8ef81c3 100644 --- a/lib/crates/fabro-cli/src/commands/render_graph.rs +++ b/lib/crates/fabro-cli/src/commands/render_graph.rs @@ -8,7 +8,7 @@ pub(crate) fn execute() -> i32 { return 1; } - match fabro_graphviz_sys::render_dot_to_svg(&dot_source) { + match graphviz_sys::render_dot_to_svg(&dot_source) { Ok(svg) => { if std::io::stdout().write_all(&svg).is_err() { return 1; diff --git a/lib/crates/fabro-graphviz/Cargo.toml b/lib/crates/fabro-graphviz/Cargo.toml index c4203551d..8cd252242 100644 --- a/lib/crates/fabro-graphviz/Cargo.toml +++ b/lib/crates/fabro-graphviz/Cargo.toml @@ -14,7 +14,7 @@ workspace = true [dependencies] anyhow.workspace = true -fabro-graphviz-sys = { path = "../fabro-graphviz-sys" } +graphviz-sys.workspace = true fabro-types = { path = "../fabro-types" } nom = "7" regex = { workspace = true } diff --git a/lib/crates/fabro-graphviz/src/render.rs b/lib/crates/fabro-graphviz/src/render.rs index 1c871f9a2..9f7bb518f 100644 --- a/lib/crates/fabro-graphviz/src/render.rs +++ b/lib/crates/fabro-graphviz/src/render.rs @@ -67,7 +67,7 @@ pub fn postprocess_svg(raw: Vec) -> Vec { /// Render styled DOT source into SVG via the vendored Graphviz library. 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) + let raw = graphviz_sys::render_dot_to_svg(&styled_source) .map_err(|e| anyhow::anyhow!("Graphviz rendering failed: {e}"))?; Ok(postprocess_svg(raw)) }