mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
refactor(graphviz): extract graphviz-sys into fabro-sh/graphviz-sys
Move the vendored Graphviz FFI crate to its own repo so it can be reused independently and reduce this repo's footprint (~250 C/H files). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp
parent
8c361afc51
commit
3e9683b6a0
7 changed files with 15 additions and 593 deletions
19
Cargo.lock
generated
19
Cargo.lock
generated
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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<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**: 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 <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. 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<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. 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<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** 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<RenderWorkflowGraphFormat>` 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<GraphOutputFormat> 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<Regex> = 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("<svg"));
|
||||
assert!(svg_str.contains("</svg>"));
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### 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.
|
||||
|
|
@ -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" }
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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 }
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@ pub fn postprocess_svg(raw: Vec<u8>) -> Vec<u8> {
|
|||
/// Render styled DOT source into SVG via the vendored Graphviz library.
|
||||
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)
|
||||
let raw = graphviz_sys::render_dot_to_svg(&styled_source)
|
||||
.map_err(|e| anyhow::anyhow!("Graphviz rendering failed: {e}"))?;
|
||||
Ok(postprocess_svg(raw))
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue