From bd38d8190a7f71294486c0ffa291628114218fae Mon Sep 17 00:00:00 2001 From: Harold Martin Date: Thu, 9 Jul 2026 08:50:25 -0700 Subject: [PATCH] Only clusters with a non-None rect can become active parents during membership walking. No-rect clusters are now transparent, so descendants fall back to the nearest rendered ancestor. Implemented a CI hang guard. --- graphviz2drawio/graphviz2drawio.py | 20 ++++---- scripts/drawio_export.sh | 66 ++++++++++++++++++++++++--- test/test_graphs.py | 73 ++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 15 deletions(-) diff --git a/graphviz2drawio/graphviz2drawio.py b/graphviz2drawio/graphviz2drawio.py index 77c00d8..b0c8a17 100644 --- a/graphviz2drawio/graphviz2drawio.py +++ b/graphviz2drawio/graphviz2drawio.py @@ -84,12 +84,14 @@ def _cluster_membership( DOT allows a node to be referenced by several clusters even though Graphviz renders it inside only one of them, so a membership candidate only counts when the member's rendered rect actually lies inside the cluster's rect. - Names that fail to round-trip through the SVG titles simply leave the - member parented further up (ultimately at the page root), which keeps its - rendered position intact because child geometry is relativized against - whichever parent is chosen. + Names that fail to round-trip through the SVG titles, and clusters without + rendered rectangles, simply leave the member parented further up (ultimately + at the page root), which keeps its rendered position intact because child + geometry is relativized against whichever parent is chosen. """ - rendered_cluster_names = set(clusters) + parentable_cluster_names = { + name for name, cluster in clusters.items() if cluster.rect is not None + } rendered_node_names = set(nodes) node_parents: dict[str, str] = {} cluster_parents: dict[str, str] = {} @@ -100,7 +102,7 @@ def _cluster_membership( nodes, clusters, rendered_node_names, - rendered_cluster_names, + parentable_cluster_names, node_parents, cluster_parents, ) @@ -113,13 +115,13 @@ def _walk_cluster_membership( nodes: Mapping[str, Node], clusters: Mapping[str, Node], rendered_node_names: set[str], - rendered_cluster_names: set[str], + parentable_cluster_names: set[str], node_parents: dict[str, str], cluster_parents: dict[str, str], ) -> None: for subgraph in subgraphs: subgraph_name = str(subgraph.name) if subgraph.name is not None else None - if subgraph_name is not None and subgraph_name in rendered_cluster_names: + if subgraph_name is not None and subgraph_name in parentable_cluster_names: current_cluster = subgraph_name if parent_cluster is not None and _is_within_cluster( clusters[subgraph_name].rect, @@ -144,7 +146,7 @@ def _walk_cluster_membership( nodes, clusters, rendered_node_names, - rendered_cluster_names, + parentable_cluster_names, node_parents, cluster_parents, ) diff --git a/scripts/drawio_export.sh b/scripts/drawio_export.sh index 90882b5..dac8cc1 100644 --- a/scripts/drawio_export.sh +++ b/scripts/drawio_export.sh @@ -68,6 +68,26 @@ should_disable_chromium_sandbox() { esac } +render_timeout_command() { + case "${DRAWIO_RENDER_TIMEOUT:-300s}" in + 0 | false | no) + return 1 + ;; + esac + + if command -v timeout > /dev/null 2>&1; then + command -v timeout + return 0 + fi + + if command -v gtimeout > /dev/null 2>&1; then + command -v gtimeout + return 0 + fi + + return 1 +} + render_drawio_png() { local input_file="$1" local output_file="$2" @@ -76,8 +96,10 @@ render_drawio_png() { local sandbox_status local render_log local render_status + local timeout_bin local xvfb_status local -a drawio_cmd + local -a timeout_cmd drawio="$(resolve_drawio)" || return 1 drawio_cmd=("$drawio") @@ -89,9 +111,22 @@ render_drawio_png() { return 1 fi fi + if [[ "$(uname -s)" == "Linux" ]]; then + drawio_cmd+=("--disable-dev-shm-usage" "--disable-gpu") + fi + + timeout_cmd=() + if timeout_bin="$(render_timeout_command)"; then + timeout_cmd=( + "$timeout_bin" + --kill-after="${DRAWIO_RENDER_TIMEOUT_KILL_AFTER:-30s}" + "${DRAWIO_RENDER_TIMEOUT:-300s}" + ) + fi mkdir -p "$(dirname "$output_file")" render_log="$(mktemp)" + echo "Rendering: $label -> $output_file" if should_use_xvfb; then xvfb_status=0 @@ -105,16 +140,32 @@ render_drawio_png() { rm -f "$render_log" return 1 fi - if xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then - render_status=0 + if [[ "${#timeout_cmd[@]}" -gt 0 ]]; then + if "${timeout_cmd[@]}" xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then + render_status=0 + else + render_status="$?" + fi else - render_status="$?" + if xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then + render_status=0 + else + render_status="$?" + fi fi elif [[ "$xvfb_status" -eq 1 ]]; then - if "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then - render_status=0 + if [[ "${#timeout_cmd[@]}" -gt 0 ]]; then + if "${timeout_cmd[@]}" "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then + render_status=0 + else + render_status="$?" + fi else - render_status="$?" + if "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then + render_status=0 + else + render_status="$?" + fi fi else rm -f "$render_log" @@ -128,6 +179,9 @@ render_drawio_png() { fi echo "Error: draw.io render failed for $label" >&2 + if [[ "$render_status" -eq 124 || "$render_status" -eq 137 ]]; then + echo "draw.io render timed out after ${DRAWIO_RENDER_TIMEOUT:-300s}" >&2 + fi if [[ -s "$render_log" ]]; then sed -n '1,40p' "$render_log" >&2 fi diff --git a/test/test_graphs.py b/test/test_graphs.py index 8927da6..5413b0e 100644 --- a/test/test_graphs.py +++ b/test/test_graphs.py @@ -265,6 +265,79 @@ def test_cluster_membership_skips_cluster_without_rect() -> None: assert cluster_parents == {} +def test_cluster_membership_uses_rendered_ancestor_for_no_rect_cluster() -> None: + graph = AGraph( + string=( + "digraph G {" + " subgraph cluster_parent {" + " subgraph cluster_missing {" + " subgraph cluster_inner { a }" + " b" + " }" + " }" + "}" + ), + ) + nodes = { + "a": SimpleNamespace(rect=Rect(25, 25, 5, 5)), + "b": SimpleNamespace(rect=Rect(50, 50, 5, 5)), + } + clusters = { + "cluster_parent": SimpleNamespace(rect=Rect(0, 0, 100, 100)), + "cluster_missing": SimpleNamespace(rect=None), + "cluster_inner": SimpleNamespace(rect=Rect(20, 20, 20, 20)), + } + + node_parents, cluster_parents = graphviz2drawio._cluster_membership( # noqa: SLF001 + graph, + nodes, + clusters, + ) + + assert node_parents["a"] == "cluster_inner" + assert node_parents["b"] == "cluster_parent" + assert cluster_parents == {"cluster_inner": "cluster_parent"} + + +def test_cells_by_plain_label_collapses_duplicate_labels() -> None: + """Document a known limitation of the ``cells_by_plain_label`` test helper. + + The helper keys cells by their rendered plain-text label, so two distinct + node cells that render the same text collapse to a single dict entry + (last-wins on insertion). Membership assertions built on this helper are + therefore only reliable for graphs whose visible labels are unique; a graph + with duplicate labels would silently drop a node and could mask a + regression. This test pins that behavior so the assumption is explicit. + """ + model = ElementTree.fromstring( + """ + + + + + + """, + ) + elements = list(model) + + labels = cells_by_plain_label(elements) + + # Both "dup" cells are real, distinct vertices... + assert ( + len( + { + e.attrib["id"] + for e in elements + if e.attrib["value"].endswith("dup") + }, + ) + == 2 + ) + # ...but they collapse to one entry, and the last one inserted wins. + assert set(labels) == {"dup", "solo"} + assert labels["dup"].attrib["id"] == "node_second" + + def test_convnet() -> None: file = "test/directed/convnet.gv.txt" xml = graphviz2drawio.convert(file)