feat(gnovm): tool for generating dependency graphs #2635

bmilojkovic · 2024-07-27T06:04:02Z

This is related to issue #2478. For now, this is just an implementation of the graph generation, no CI/CD yet. The tool creates one or more .dot files that can be used to generate an SVG via graphviz. To use the tool, just do the following:

cd gnovm
make build
./build/gno depgraph -o depgraph.dot ../examples

DOT is a graphviz language for describing graphs. It includes layout information as well. For the final step which makes an SVG you need to have graphviz installed.

dot -Tsvg -o depgraph.svg depgraph.dot

The graph below will be produced. I tried several different layout schemes provided by graphviz, and I'm pretty sure that the sheer number of packages and connections is probably always going to make a bit of a mess. This particular layout doesn't clearly show what exactly is being referenced, however it DOES clearly show how often packages are being referenced (blue nodes are p/ and red nodes are r/).

If you would like to more clearly see requires for a specific package, the tool can also be invoked with the -m flag ("multiple graphs") which produces a hierarchy of directories mimicking the input structure with a .dot file at the bottom for each package. For now you would need to manually convert those to SVG files. Example usage:

./build/gno depgraph -m -o depgraph ../examples

The next steps from here could be:

Develop CI that would automatically produce this SVG and include it in a README so we can easily track examples.
Add a flag to skip .dot and output directly to SVG (this would require graphviz to be installed ofc)
Add some layout and visual customization options? This might be overengineering it. I'm not sure if this is something we realistically need down the line. Maybe we can identify several useful layouts and just support those. For now, the layout is hardcoded.

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-07-29T09:52:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.11%. Comparing base (bbcb2f6) to head (5e4bb45).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
- Coverage   60.11%   60.11%   -0.01%     
==========================================
  Files         560      560              
  Lines       74918    74918              
==========================================
- Hits        45039    45037       -2     
- Misses      26504    26505       +1     
- Partials     3375     3376       +1

Flag	Coverage Δ
contribs/gnodev	`61.40% <ø> (ø)`
contribs/gnofaucet	`14.46% <ø> (-0.86%)`	⬇️
gno.land	`64.17% <ø> (ø)`
gnovm	`64.13% <ø> (ø)`
misc/genstd	`80.54% <ø> (ø)`
misc/logos	`19.88% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thehowl

cc @gfanton for good taste in tools and a first review :)

thehowl · 2024-07-29T17:35:02Z

gnovm/cmd/gno/depgraph.go

Can you put the generating source code in gnovm/pkg/depgraph, so we can hope to reuse it?

I'm also thinking how to have this on the CLI; having it as a subcommand of gno doesn't feel right. I feel like in go this could be go tool xxx; idk.

Yea i agree, having it in gno tool could actually be a good idea. We could also decide to put this kind of tools inside the contribs folder, at last for now, an move it in the core code when it's mature enough and/or we find out that we use it really often. wdyt ?

gfanton

Super cool! (check my comments for requested changes)

As Morgan says, I'm not sure about keeping this in the root of gno cli. I think we should either move this to contribs or a tool subcommand. cc @moul, wdyt?

gfanton · 2024-07-30T08:06:03Z

gnovm/cmd/gno/depgraph.go

+		file.Close()
+	} else { // useful for testing - makes a separate graph for each found package


return early here to avoid enclosing the rest of the method in an else statement.

gfanton · 2024-07-30T08:43:43Z

gnovm/cmd/gno/depgraph.go

Yea i agree, having it in gno tool could actually be a good idea. We could also decide to put this kind of tools inside the contribs folder, at last for now, an move it in the core code when it's mature enough and/or we find out that we use it really often. wdyt ?

gfanton · 2024-07-30T09:20:39Z

gnovm/cmd/gno/depgraph.go

+		for _, pkg := range allPkgs {
+			err := buildGraphData(pkg, allPkgs, make(map[string]bool), make(map[string]bool), &graphData)
+
+			if err != nil {
+				return fmt.Errorf("error in building graph: %w", err)
+			}
+		}


The whole purpose of passing the visited map to this method is to keep track of already visited packages. You need to move the declaration of the visited map above the for loop to ensure there will be no duplicates while iterating between packages.

Suggested change

for _, pkg := range allPkgs {

err := buildGraphData(pkg, allPkgs, make(map[string]bool), make(map[string]bool), &graphData)

if err != nil {

return fmt.Errorf("error in building graph: %w", err)

}

}

visited := make(map[string]bool)

for _, pkg := range allPkgs {

err := buildGraphData(pkg, allPkgs, visited, make(map[string]bool), &graphData)

if err != nil {

return fmt.Errorf("error in building graph: %w", err)

}

}

Doh! Yeah, I was thinking about solving that with a different structure, but this is simpler.

gfanton · 2024-07-30T09:22:20Z

gnovm/cmd/gno/depgraph.go

+func buildGraphData(pkg gnomod.Pkg, allPkgs []gnomod.Pkg, visited map[string]bool, onStack map[string]bool, graphData *string) error {
+	if onStack[pkg.Name] {
+		return fmt.Errorf("cycle detected: %s", pkg.Name)
+	}
+	if visited[pkg.Name] {
+		return nil
+	}
+
+	visited[pkg.Name] = true
+	onStack[pkg.Name] = true
+
+	for _, req := range pkg.Requires {
+		found := false
+
+		for _, candidate := range allPkgs {
+			if candidate.Name != req {
+				continue
+			}
+			if err := buildGraphData(candidate, allPkgs, visited, onStack, graphData); err != nil {
+				return err
+			}
+			found = true
+			// this check is wildly inefficient - change this to not work with strings directly
+			if !strings.Contains(*graphData, "\""+pkg.Name+"\" -> \""+req+"\"\n") {
+				*graphData += "\"" + pkg.Name + "\" -> \"" + req + "\"\n"
+			}
+		}
+		if !found {
+			return fmt.Errorf("couldn't find dependency %q for package %q", req, pkg.Name)
+		}
+	}
+
+	onStack[pkg.Name] = false
+
+	return nil
+}


You don't need to check if the line already exists because you are already ensuring that you visit the package and requirements only once.

Suggested change

func buildGraphData(pkg gnomod.Pkg, allPkgs []gnomod.Pkg, visited map[string]bool, onStack map[string]bool, graphData *string) error {

if onStack[pkg.Name] {

return fmt.Errorf("cycle detected: %s", pkg.Name)

}

if visited[pkg.Name] {

return nil

}

visited[pkg.Name] = true

onStack[pkg.Name] = true

for _, req := range pkg.Requires {

found := false

for _, candidate := range allPkgs {

if candidate.Name != req {

continue

}

if err := buildGraphData(candidate, allPkgs, visited, onStack, graphData); err != nil {

return err

}

found = true

// this check is wildly inefficient - change this to not work with strings directly

if !strings.Contains(*graphData, "\""+pkg.Name+"\" -> \""+req+"\"\n") {

*graphData += "\"" + pkg.Name + "\" -> \"" + req + "\"\n"

}

}

if !found {

return fmt.Errorf("couldn't find dependency %q for package %q", req, pkg.Name)

}

}

onStack[pkg.Name] = false

return nil

}

func buildGraphData(pkg gnomod.Pkg, allPkgs []gnomod.Pkg, visited map[string]bool, onStack map[string]bool, graphData *string) error {

if onStack[pkg.Name] {

return fmt.Errorf("cycle detected: %s", pkg.Name)

}

if visited[pkg.Name] {

return nil

}

visited[pkg.Name] = true

onStack[pkg.Name] = true

for _, req := range pkg.Requires {

var candidate gnomod.Pkg

var found bool

for _, candidate = range allPkgs {

if candidate.Name != req {

continue

}

found = true

break

}

if !found {

return fmt.Errorf("couldn't find dependency %q for package %q", req, pkg.Name)

}

if err := buildGraphData(candidate, allPkgs, visited, onStack, graphData); err != nil {

return err

}

*graphData += "\"" + pkg.Name + "\" -> \"" + req + "\"\n"

}

onStack[pkg.Name] = false

return nil

}

gfanton · 2024-08-08T15:33:53Z

contribs/gnodepgraph/depgraph.go

+		nodeData := ""  // nodes
+		graphData := "" // edges


nit: Using a strings.Builder or bytes.Buffer along with fmt.Fprintf and passing an io.Writer to buildGraphData would have been a cleaner approach than directly writing the output to a string directly.

moul · 2024-08-31T17:14:23Z

In my opinion, a suitable location for this tool would be in the 'gno mod graph'. It does not appear to import any new dependencies and is closely integrated with gnomod.

moul · 2024-08-31T17:16:16Z

contribs/gnodepgraph/depgraph.go

+	fs.BoolVar(&c.verbose, "v", false, "verbose output when lintning")
+	fs.StringVar(&c.rootDir, "root-dir", rootDir, "clone location of github.com/gnolang/gno (gno tries to guess it)")
+	fs.StringVar(&c.output, "o", "depgraph", "output (file if single graph, dir if multiple graphs)")
+	fs.BoolVar(&c.multipleGraphs, "m", false, "make a separate graph for each package")


remove this flag, you should always generate a single graph.

moul · 2024-08-31T17:16:28Z

contribs/gnodepgraph/depgraph.go

+	rootDir := gnoenv.RootDir()
+	fs.BoolVar(&c.verbose, "v", false, "verbose output when lintning")
+	fs.StringVar(&c.rootDir, "root-dir", rootDir, "clone location of github.com/gnolang/gno (gno tries to guess it)")
+	fs.StringVar(&c.output, "o", "depgraph", "output (file if single graph, dir if multiple graphs)")


remove this flag, generate to stdout.

moul · 2024-08-31T17:17:08Z

contribs/gnodepgraph/depgraph_test.go

+// test for big graph
+
+// test for fail on cyclical dependencies
+
+// test for fail on missing dependencies
+
+// test for not duplicating dependencies


we need more tests.

moul · 2024-08-31T17:18:40Z

contribs/gnodepgraph/depgraph.go

+
+func (c *depGraphCfg) RegisterFlags(fs *flag.FlagSet) {
+	rootDir := gnoenv.RootDir()
+	fs.BoolVar(&c.verbose, "v", false, "verbose output when lintning")


would be nice to have a non-graphviz format by default, could be just lines with:

A -> B A -> C C -> D

something we can easily read and grep.

then, the graphviz format would be a flag.

moul · 2024-08-31T17:19:12Z

contribs/gnodepgraph/depgraph.go

+		if err != nil {
+			return fmt.Errorf("couldn't open output file: %w", err)
+		}
+		graphFileData := fmt.Sprintf("Digraph G {\nrankdir=\"LR\"\nranksep=20\n%s\n%s\n}", nodeData.String(), graphData.String())


this code is hard to read.

moul · 2024-08-31T17:19:34Z

contribs/gnodepgraph/depgraph.go

+		nodeData.WriteString("}\nsubgraph {\nrank=same\n")
+		for _, pkg := range allPkgs {
+			if strings.Contains(pkg.Name, "gno.land/r") {
+				fmt.Fprintf(&nodeData, "\"%s\" [color=\"red\"]\n", pkg.Name)


you should use backticks when you generate things with double quotes.

Branislav Milojkovic added 2 commits July 26, 2024 10:02

Initial implementation of dependency graph

fc25fb8

Adding tests and a few comments

0818b7c

bmilojkovic requested review from moul and thehowl as code owners July 27, 2024 06:04

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 27, 2024

github-actions bot assigned bmilojkovic Jul 27, 2024

zivkovicmilos self-requested a review July 29, 2024 09:51

thehowl requested a review from gfanton July 29, 2024 17:31

thehowl reviewed Jul 29, 2024

View reviewed changes

gfanton requested changes Jul 30, 2024

View reviewed changes

Moved the tool to contribs. Fixed some inefficiencies.

0e1973e

bmilojkovic requested a review from a team as a code owner August 4, 2024 10:31

Fixing .mod file

9cfa494

leohhhn linked an issue Aug 4, 2024 that may be closed by this pull request

tool to display a graph of dependencies #2478

Open

bmilojkovic and others added 2 commits August 5, 2024 07:03

Merge branch 'master' into pr_graph

026814b

Merge branch 'master' into pr_graph

5e4bb45

gfanton reviewed Aug 8, 2024

View reviewed changes

bmilojkovic added 2 commits August 9, 2024 15:49

Fixing errors. Added string buffers.

77d7e0a

Merge branch 'pr_graph' of github.com:bmilojkovic/gno into pr_graph

738c489

moul reviewed Aug 31, 2024

View reviewed changes

Kouteki added review/triage-pending PRs opened by external contributors that are waiting for the 1st review and removed review/triage-pending PRs opened by external contributors that are waiting for the 1st review labels Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): tool for generating dependency graphs #2635

feat(gnovm): tool for generating dependency graphs #2635

bmilojkovic commented Jul 27, 2024

codecov bot commented Jul 29, 2024 •

edited

Loading

thehowl left a comment

thehowl Jul 29, 2024

gfanton Jul 30, 2024

thehowl Jul 30, 2024

gfanton left a comment

gfanton Jul 30, 2024

gfanton Jul 30, 2024

gfanton Jul 30, 2024

bmilojkovic Aug 1, 2024

gfanton Jul 30, 2024

gfanton Aug 8, 2024

moul commented Aug 31, 2024

moul Aug 31, 2024

moul Aug 31, 2024

moul Aug 31, 2024

moul Aug 31, 2024

moul Aug 31, 2024 •

edited

Loading

moul Aug 31, 2024

		file.Close()
		} else { // useful for testing - makes a separate graph for each found package

feat(gnovm): tool for generating dependency graphs #2635

Are you sure you want to change the base?

feat(gnovm): tool for generating dependency graphs #2635

Conversation

bmilojkovic commented Jul 27, 2024

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfanton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moul commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moul Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2024 •

edited

Loading

moul Aug 31, 2024 •

edited

Loading