Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bst graph command #1949

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ion232
Copy link

@ion232 ion232 commented Aug 12, 2024

Closes #1915

  • Adds a new command to bst called 'graph' which reimplements the same API and functionality as contrib/bst-graph.
  • The implementation details are within _stream.py as another function called 'graph'.
  • Removes contrib/bst-graph.

EDIT: I've added another commit that changes the functionality to makes two separate graphs for buildtime and runtime, as this is clearer imo.

@ion232 ion232 marked this pull request as draft August 13, 2024 06:43
@ion232 ion232 changed the title WIP: Add bst graph command Add bst graph command Aug 13, 2024
Adds a new command to bst called 'graph' which reimplements the same API
and functionality as contrib/bst-graph. The implementation details are
within _stream.py as another function called 'graph'.

Part of apache#1915.
@ion232 ion232 force-pushed the ion232/reimplement-bst-graph-into-buildstream branch from 24179c8 to 9faeca8 Compare August 13, 2024 07:20
@ion232 ion232 marked this pull request as ready for review August 13, 2024 07:23
It can already be difficult to tell what's going on in large graphs.
This commit helps make the graphs clearer and smaller.
However this is no longer the same functionality as in the original
contrib/bst-graph.

Part of apache#1915.
This is no longer needed when reimplemented into buildstream itself.

Part of apache#1915.
@ion232 ion232 changed the title Add bst graph command Draft: Add bst graph command Aug 20, 2024
@ion232 ion232 marked this pull request as draft August 20, 2024 12:39
@ion232 ion232 changed the title Draft: Add bst graph command Add bst graph command Aug 20, 2024
@jjardon
Copy link
Contributor

jjardon commented Sep 25, 2024

@ion232 is there anything else pending to mark this a non-draft?

@gtristan
Copy link
Contributor

gtristan commented Sep 26, 2024

I'm not really in favor of having bst-graph at all.

I think we should either:

  • Enhance bst show with features like --depth or such, allowing some scripting to construct the graph and display it however it wants
  • Open up some very limited Stream / App APIs from the buildstream module and make a limited set of APIs public
    • Allowing at least to load the build graph once and have access to the Element and Source data model, eliminating the need to load the build graph multiple times (as would be the case with bst show scripting) and allowing users to do whatever introspective operations they might want

Either of these approaches don't add a first class graphing feature but allow users to do more powerful things without requiring special buildstream features to do so... specifically this should be interesting if we add APIs for Source objects to report information about their URLs etc, so that we can do SBoMs in a way that is stable and does not abuse internal APIs (like fdsdk's collect_manifest thing does).

EDIT: It looks like this script already gets the work done with a single bst show, so that has already been thought out pretty well.

I find the output to be particularly horrible to look at for sufficiently big graphs (I've finally looked at one today, after all these years), I suspect that whatever useful things one might want to achieve by looking at this, could be better achieved with some scripting (e.g. perhaps a combination of bst show commands with bst artifact list-contents could get you as far as finding out "Why does this file I don't want end up in my image ?" and this would be more reliable than looking at this huge graph IMHO).

Side note, the script does not work with junctions, it seems to be a simple quoting error that shouldn't be hard to fix..

That said I think we made the right choice keeping this in contrib/. This avoids scope creep and avoids the addition of dependencies which are unrelated to the core mission and APIs - separate projects can do nice things with our APIs and use more elaborate dependencies, and ask us to open up some APIs as needed.

@ion232
Copy link
Author

ion232 commented Sep 26, 2024

@ion232 is there anything else pending to mark this a non-draft?

Not really. I was possibly going add some other features but didn't get around to it. I was waiting for feedback.

@harrysarson
Copy link
Contributor

harrysarson commented Oct 24, 2024

Thanks for this script! I hit a small issue when using it on a complex buildstream project containing junctions. The generated graphviz ends up containing colons in the wrong place making it invalid. See small patch below that fixes the script base on a recommendation I found here https://graphviz.readthedocs.io/en/stable/manual.html#node-ports-compass

diff --git a/contrib/bst-graph b/contrib/bst-graph
index 3fe93e1ff..5c05c6b4b 100755
--- a/contrib/bst-graph
+++ b/contrib/bst-graph
@@ -29,6 +29,7 @@ installed.
 import argparse
 import subprocess
 import re
+import urllib.parse
 
 from graphviz import Digraph
 from ruamel.yaml import YAML
@@ -55,6 +56,10 @@ def parse_args():
     return parser.parse_args()
 
 
+def escape(s):
+    return urllib.parse.quote_plus(s.encode())
+
+
 def parse_graph(lines):
     '''Return nodes and edges of the parsed grpah.
 
@@ -80,12 +85,13 @@ def parse_graph(lines):
         build_dep = parser.load(build_dep)
         runtime_dep = parser.load(runtime_dep)
 
-        nodes.add(name)
-        [build_deps.add((name, dep)) for dep in build_dep if dep]
-        [runtime_deps.add((name, dep)) for dep in runtime_dep if dep]
+        safe_name = escape(name)
 
-    return nodes, build_deps, runtime_deps
+        nodes.add((safe_name, name))
+        [build_deps.add((safe_name, escape(dep))) for dep in build_dep if dep]
+        [runtime_deps.add((safe_name, escape(dep))) for dep in runtime_dep if dep]
 
+    return nodes, build_deps, runtime_deps
 
 def generate_graph(nodes, build_deps, runtime_deps):
     '''Generate graph from given nodes and edges.
@@ -99,8 +105,8 @@ def generate_graph(nodes, build_deps, runtime_deps):
        A graphviz.Digraph object
     '''
     graph = Digraph()
-    for node in nodes:
-        graph.node(node)
+    for tag, name in nodes:
+        graph.node(tag, label=name)
     for source, target in build_deps:
         graph.edge(source, target, label='build-dep')
     for source, target in runtime_deps:

@abderrahim
Copy link
Contributor

@harrysarson This PR is about integrating the bst-graph script into buildstream proper. For fixes to the existing contrib/bst-graph script, please open a PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-implement contrib/bst-graph as part of BuildStream
5 participants