adds dependency analysis #18543

juanvallejo · 2018-02-09T07:28:34Z

Depends on #18466
Depends on #18606

Adds dependency graph analysis.
Outputs "yours", "mine", "ours" dependencies.

Usage:

$ ./depcheck analyze --root=github.com/openshift/origin --entry=cmd/... --entry=pkg/... --entry=tools/... --entry=test/... --openshift --dep=github.com/openshift/origin/vendor/k8s.io/kubernetes

Output of the command above

cc @deads2k

deads2k · 2018-02-09T13:07:55Z

graph still looks like you're cutting at github/org not github/org/repo

juanvallejo · 2018-02-10T00:32:06Z

@deads2k updated graph to reflect latest changes in #18466
Added "mine" and "ours" packages

deads2k · 2018-02-12T13:42:28Z

remove all gihtub.com/openshift/origin/... packages from the yours/mine/ours output

deads2k · 2018-02-12T14:00:14Z

tools/depcheck/pkg/cmd/trace/pkg.go

@@ -91,7 +91,10 @@ func BuildGraph(packages *PackageList, excludes []string) (*depgraph.MutableDire

 			to, exists := g.NodeByName(dependency)
 			if !exists {
-				return nil, fmt.Errorf("expected child node for dependency %q was not found in graph", dependency)
+				// if a package imports a dependency that we did not visit


this was supposed to end up in a separate pull. did I miss it?

Opened a pull here to begin addressing this #18606

Opened a pull here to begin addressing this #18606

whoa, that pull is big. I was expecting it to be 5 lines long.

deads2k · 2018-02-12T15:00:38Z

tools/depcheck/pkg/cmd/trace/pkg.go

@@ -108,6 +111,55 @@ func BuildGraph(packages *PackageList, excludes []string) (*depgraph.MutableDire
 	return g, nil
 }

+func copyGraph(g *depgraph.MutableDirectedGraph) (*depgraph.MutableDirectedGraph, error) {


deads2k · 2018-02-12T15:02:54Z

tools/depcheck/pkg/cmd/trace/trace.go

+	return nil
+}
+
+func printYours(g *depgraph.MutableDirectedGraph, roots, nodes []graph.Node, you *depgraph.Node) ([]graph.Node, error) {


You have a print method that is mutating its input?? Don't do that.

deads2k · 2018-02-12T15:04:37Z

tools/depcheck/pkg/cmd/trace/trace.go

+		from := g.From(n)
+
+		// found an existing orphaned node to exclude
+		if len(to) == 0 && len(from) == 0 {


When does this happen? I would expect these to snipped before you call this method.

deads2k · 2018-02-12T15:05:08Z

tools/depcheck/pkg/cmd/trace/trace.go

+	g.RemoveNode(you)
+
+	// find nodes not reachable from our given roots
+	yours := findOrphans(g, roots, nodes)


Given a clean starting graph, the idea of "roots" doesn't exist.

deads2k · 2018-02-12T15:06:09Z

tools/depcheck/pkg/cmd/trace/trace.go

+	for _, n := range nodes {
+		isOrphan := true
+		for _, root := range roots {
+			if hasPathFromTo(g, root, n) {


This doesn't look right. If you started your graph without orphans, then any orphans you're finding later on the result of the removing the "from" on an inbound link.

deads2k · 2018-02-12T15:08:43Z

tools/depcheck/pkg/cmd/trace/trace.go

+	return nil
+}
+
+func printYours(g *depgraph.MutableDirectedGraph, roots, nodes []graph.Node, you *depgraph.Node) ([]graph.Node, error) {


The logic in here looks wrong. I expected you to produce a new graph by filtering out a selected set of nodes. Then after you've built your new graph, you simply scan all nodes to see if they don't have any inbound links. You track those, then built a new graph using the nodes without any inbound links as your input. Repeat until you have no changes. Then union all the nodes you removed.

Notice that at no point do we discuss "roots". I'm still trying to figure out why that concept is needed.

deads2k · 2018-02-14T14:05:01Z

tools/depcheck/pkg/graph/graph.go

@@ -55,7 +55,64 @@ func (g *MutableDirectedGraph) AddNode(n *Node) error {
 	return nil
 }

+func (g *MutableDirectedGraph) RemoveNode(n *Node) {


godoc whether this leaves dangling edges

deads2k · 2018-02-14T14:05:39Z

tools/depcheck/pkg/graph/graph.go

 func (g *MutableDirectedGraph) NodeByName(name string) (graph.Node, bool) {
 	n, exists := g.nodesByName[name]
 	return n, exists && g.DirectedGraph.Has(n)
 }
+
+// remove nodes with no inbound edges
+func (g *MutableDirectedGraph) PruneOrphans() {


Return a list of the nodes you removed.

deads2k · 2018-02-14T14:09:13Z

tools/depcheck/pkg/cmd/trace/trace.go

+// - "Yours": a list of every node in the set unique to the dependency tree of a given target node.
+// - "Mine": a list of every node in the set unique to the dependency tree of the root nodes (set non-overlapping with given target node)
+// - "Ours": a list of every node in the overlapping set between the dependency tree of the root nodes and a given target node
+func (o *TraceImportsOpts) analyzeGraph(g *depgraph.MutableDirectedGraph) error {


This is really hard to read. Separate your library methods (things operating on input data), from your command wiring (things combining a set of library methods). I should have a small method for identifying "which repos are only present because this set of nodes is present".

deads2k · 2018-02-14T14:10:04Z

tools/depcheck/pkg/cmd/trace/trace.go

+// This operation is done recursively until we no longer cause
+// any nodes to become orphaned after pruning the removed set.
+// The resulting set of total nodes recursively removed is returned.
+func calculateOrphans(g *depgraph.MutableDirectedGraph, targetNodes []*depgraph.Node) []*depgraph.Node {


I think this is the graph method I want. Peer to the other graph methods I'd think. Unit tests located there to check it.

deads2k · 2018-02-14T14:11:37Z

tools/depcheck/pkg/cmd/trace/trace.go

+				continue
+			}
+
+			orphans = append(orphans, n.(*depgraph.Node))


Isn't this exactly what your PruneOrphans method does? I didn't look, but for that method to work it would have to be recursive. It is recursive, right? If not, write the test that will cause that method to break and then fix it.

deads2k · 2018-02-14T14:12:15Z

tools/depcheck/pkg/cmd/trace/trace.go

+// This operation is done recursively until we no longer cause
+// any nodes to become orphaned after pruning the removed set.
+// The resulting set of total nodes recursively removed is returned.
+func calculateOrphans(g *depgraph.MutableDirectedGraph, targetNodes []*depgraph.Node) []*depgraph.Node {


Bad name. This is FindExclusiveDependencies(g, targetNodes)

deads2k · 2018-02-14T14:12:37Z

tools/depcheck/pkg/cmd/trace/trace.go

+}
+
+type TraceImportsFlags struct {
+	Roots        []string


this should be dead, rigth?

deads2k · 2018-02-14T14:12:54Z

tools/depcheck/pkg/cmd/trace/trace.go

+	excludes []string
+	filters  []string
+
+	Analyze          string


bad name, what is this?

deads2k · 2018-02-14T14:13:10Z

tools/depcheck/pkg/cmd/trace/trace.go

+`
+
+type TraceImportsOpts struct {
+	Roots   []string


dead I hope

deads2k · 2018-02-14T14:14:01Z

tools/depcheck/pkg/cmd/trace/trace.go

+	fmt.Printf("Analyzing a total of %v packages\n", len(g.Nodes()))
+	fmt.Println()
+
+	yours := calculateOrphans(g, []*depgraph.Node{you})


Why are you messing with a single repo/node. That doesn't make sense for what this needs to do.

deads2k · 2018-02-14T14:15:43Z

tools/depcheck/pkg/cmd/trace/trace.go

+	}
+
+	cmd.Flags().StringSliceVar(&flags.Roots, "root", flags.Roots, "set of entrypoints for dependency trees used to generate a depedency graph.")
+	cmd.Flags().StringVarP(&flags.Exclude, "exclude", "e", "", "json file containing a list of import-paths of packages to recursively exclude when traversing the set of given entrypoints specified through --root.")


description sounds bad. I'd want to choose recursive exclusion or not based on ... in golang, right?

deads2k · 2018-02-14T14:16:26Z

tools/depcheck/pkg/cmd/trace/trace.go

+	cmd.Flags().StringVarP(&flags.Exclude, "exclude", "e", "", "json file containing a list of import-paths of packages to recursively exclude when traversing the set of given entrypoints specified through --root.")
+	cmd.Flags().StringVarP(&flags.OutputFormat, "output", "o", "", "output generated dependency graph in specified format. One of: dot.")
+	cmd.Flags().StringVarP(&flags.Collapse, "collapse", "c", "", "json file containing a list of import-paths of packages to collapse sub-packages into.")
+	cmd.Flags().StringVarP(&flags.Analyze, "analyze", "a", "", "output a summary report on the dependency set of a given repository. Mutually exclusive with --output")


if its mutually exclusive, why do we have this lever? Is this just a different output mode or is it (more likely) a different command.

deads2k · 2018-02-14T14:17:06Z

tools/depcheck/pkg/cmd/trace/trace.go

+		},
+	}
+
+	cmd.Flags().StringSliceVar(&flags.Roots, "root", flags.Roots, "set of entrypoints for dependency trees used to generate a depedency graph.")


It really seems like you ought to have a set of common flags for building the graph, then a separate set of flags for analyzing it and that you're re-use the flags for building the graph in multiple different commands.

juanvallejo · 2018-03-06T23:15:02Z

@deads2k Went ahead and rebased with approved changes from #18466.

Added the analysis bits on top.
Switched to using gonum Dijkstra's instead of home-grown breath-first path traversal :)
Command output is exactly the same as the updated one posted in #18543 (comment)

juanvallejo · 2018-03-07T07:22:00Z

/retest

juanvallejo · 2018-03-07T21:58:14Z

/test extended_networking_minimal

deads2k · 2018-03-08T13:38:04Z

prereq merged. rebase for smaller diff

deads2k · 2018-03-08T17:27:15Z

tools/depcheck/pkg/cmd/analyze.go

+`
+
+type AnalyzeOptions struct {
+	*graph.GraphOptions


no anonymous inclusion

deads2k · 2018-03-08T17:27:22Z

tools/depcheck/pkg/cmd/analyze.go

+type AnalyzeOptions struct {
+	*graph.GraphOptions
+
+	// Packages to analyze against


deads2k · 2018-03-08T17:27:31Z

tools/depcheck/pkg/cmd/analyze.go

+}
+
+type AnalyzeFlags struct {
+	*graph.GraphFlags


no anonymous inclusion

deads2k · 2018-03-08T17:33:05Z

tools/depcheck/pkg/cmd/analyze.go

+// - "Yours": a list of every node in the set unique to the dependency tree of a given target node.
+// - "Mine": a list of every node in the set unique to the dependency tree of the root nodes (set non-overlapping with given target node)
+// - "Ours": a list of every node in the overlapping set between the dependency tree of the root nodes and a given target node
+func (o *AnalyzeOptions) analyzeGraph(g *graph.MutableDirectedGraph) error {


needs unit tests

deads2k · 2018-03-08T17:33:41Z

Add some tests. Looks pretty good.

juanvallejo · 2018-03-12T18:30:08Z

@deads2k thanks, added tests

juanvallejo · 2018-03-19T14:16:09Z

@deads2k addressed in-person feedback regarding calculateYours, calculateMineOurs methods. Updated tests as well. Commits squashed.

Approving per our conversation.

juanvallejo · 2018-03-19T17:23:33Z

/test unit
/test gcp

juanvallejo · 2018-03-19T19:03:23Z

/test gcp

juanvallejo · 2018-03-19T20:34:53Z

@deads2k tests passing, adding /lgtm
@soltysh fyi

/lgtm

openshift-ci-robot · 2018-03-19T20:34:54Z

@juanvallejo: you cannot LGTM your own PR.

In response to this:

@deads2k tests passing, adding /lgtm
@soltysh fyi

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2018-03-19T20:35:02Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: juanvallejo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

tools/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-robot · 2018-03-19T23:43:50Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2018-03-20T01:09:56Z

@juanvallejo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/gcp	`4792756`	link	`/test gcp`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2018-03-20T02:55:51Z

Automatic merge from submit-queue (batch tested with PRs 18999, 18543).

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 9, 2018

openshift-ci-robot requested review from deads2k and smarterclayton February 9, 2018 07:29

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch from 2437936 to 400edcd Compare February 10, 2018 00:26

deads2k reviewed Feb 12, 2018

View reviewed changes

deads2k mentioned this pull request Feb 13, 2018

Enable osin internal error logging #18505

Merged

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch 2 times, most recently from c6fadf7 to 45b85cd Compare February 14, 2018 08:09

deads2k reviewed Feb 14, 2018

View reviewed changes

tools/depcheck/pkg/cmd/trace/trace.go Outdated

}

type TraceImportsFlags struct {

Roots []string

Copy link

Contributor

deads2k Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be dead, rigth?

deads2k reviewed Feb 14, 2018

View reviewed changes

tools/depcheck/pkg/cmd/trace/trace.go Outdated

excludes []string

filters []string

Analyze string

Copy link

Contributor

deads2k Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad name, what is this?

deads2k reviewed Feb 14, 2018

View reviewed changes

tools/depcheck/pkg/cmd/trace/trace.go Outdated

`

type TraceImportsOpts struct {

Roots []string

Copy link

Contributor

deads2k Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead I hope

deads2k reviewed Feb 14, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch from 36b30fa to 039582b Compare March 8, 2018 15:30

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2018

deads2k reviewed Mar 8, 2018

View reviewed changes

tools/depcheck/pkg/cmd/analyze.go Outdated

`

type AnalyzeOptions struct {

*graph.GraphOptions

Copy link

Contributor

deads2k Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no anonymous inclusion

deads2k reviewed Mar 8, 2018

View reviewed changes

tools/depcheck/pkg/cmd/analyze.go Outdated

}

type AnalyzeFlags struct {

*graph.GraphFlags

Copy link

Contributor

deads2k Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no anonymous inclusion

deads2k reviewed Mar 8, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch 2 times, most recently from d84f0e7 to 98802b7 Compare March 12, 2018 18:27

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch from 98802b7 to f389dcf Compare March 19, 2018 14:03

add graph analysis

4792756

juanvallejo force-pushed the jvallejo/analysis-deps-yours branch from f389dcf to 4792756 Compare March 19, 2018 14:15

juanvallejo added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2018

juanvallejo added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2018

openshift-merge-robot merged commit 08506eb into openshift:master Mar 20, 2018

adds dependency analysis #18543

adds dependency analysis #18543

Conversation

juanvallejo commented Feb 9, 2018 • edited Loading

deads2k commented Feb 9, 2018

juanvallejo commented Feb 10, 2018

deads2k commented Feb 12, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Mar 6, 2018 • edited Loading

juanvallejo commented Mar 7, 2018

juanvallejo commented Mar 7, 2018

deads2k commented Mar 8, 2018

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

deads2k commented Mar 8, 2018

juanvallejo commented Mar 12, 2018

juanvallejo commented Mar 19, 2018 • edited Loading

juanvallejo commented Mar 19, 2018

juanvallejo commented Mar 19, 2018

juanvallejo commented Mar 19, 2018

openshift-ci-robot commented Mar 19, 2018

openshift-ci-robot commented Mar 19, 2018

openshift-merge-robot commented Mar 19, 2018

openshift-ci-robot commented Mar 20, 2018 • edited Loading

openshift-merge-robot commented Mar 20, 2018

juanvallejo commented Feb 9, 2018 •

edited

Loading

juanvallejo commented Mar 6, 2018 •

edited

Loading

juanvallejo commented Mar 19, 2018 •

edited

Loading

openshift-ci-robot commented Mar 20, 2018 •

edited

Loading