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

Dep status tree visualisation dot output #271

Merged
merged 14 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@Rhymond
Copy link
Contributor

Rhymond commented Feb 26, 2017

Couple of real life examples:

github.com/asciimoo/wuzz
output

github.com/wallix/awless
output 1

gopkg.in/kataras/iris.v6
output 2

  • BasicStatus was extended and Children field was added in order to get project child packages
  • -o was added to specify output file name (default: output.svg)
  • Default format is SVG

REF

@googlebot googlebot added the cla: yes label Feb 26, 2017

Rhymond added some commits Feb 26, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

Great first steps, thanks for doing this! A few things to work through before we can merge.

@@ -49,6 +55,7 @@ func (cmd *statusCommand) Register(fs *flag.FlagSet) {
fs.BoolVar(&cmd.detailed, "detailed", false, "report more detailed status")
fs.BoolVar(&cmd.json, "json", false, "output in JSON format")
fs.StringVar(&cmd.template, "f", "", "output in text/template format")
fs.StringVar(&cmd.output, "o", "output.svg", "output file")

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

Instead of -o, let's have -dot, then simply compute dot output as a string and print that to stdout. That'll be much more flexible for end-users.

This comment has been minimized.

@Rhymond

Rhymond Mar 10, 2017

Contributor

If we are only printing dot output to stdout this parameter is not necessary

This comment has been minimized.

@sdboyer

sdboyer Mar 10, 2017

Member

It is necessary - we're not printing dot output in the default case. The parameter is needed to indicate that dot output is desired. (So, a bool instead of a string)

This comment has been minimized.

@Rhymond

Rhymond Mar 10, 2017

Contributor

-dot flag was in the code before: https://github.com/golang/dep/blob/master/cmd/dep/status.go#L52

This is a newly created flag for saving generated dot string into the file which is not necessary if we're stdout string now.

This comment has been minimized.

@sdboyer

sdboyer Mar 10, 2017

Member

Hah, so it was. That was in our spec, but we never actually implemented it, so I figured it wasn't there. Didn't bother to check!

So then, yes, it's simply a matter of dropping the -o param you added.

@@ -150,6 +158,123 @@ func (out *jsonOutput) MissingFooter() {
json.NewEncoder(out.w).Encode(out.missing)
}

type dotProject struct {

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

I think everything related to generating graphviz output could be safely put into its own, separate file - graphviz.go

out.bsh[dp.project] = dp.hash()

// Create name boxes, and name them using hashes
// to avoid encoding name conflicts

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

nit: either use just a couple clarifying words, or a complete sentence (including a period) in comments, please

fmt.Printf("%v", err)
}

defer syscall.Unlink(tf.Name())

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

This should all go away when switching to just printing to stdout instead of a tmp file, but for future - os.Remove is the call to make here.

switch out.(type) {
case *dotOutput:
r := filepath.Join(p.AbsRoot, "vendor", string(proj.Ident().ProjectRoot))
ptr, err := gps.ListPackages(r, string(proj.Ident().ProjectRoot))

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

Instead of reading from vendor here, use a SourceManager's version of ListPackages(). That'll take care of ensuring the files are present, unmodified, and at the right version (which you'll have to pass in). At some point, it'll also get caching for free, too.

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

Also, side note - by placing this within the branch of the if where the memo matches, we create a potentially confusing situation for users where they ask for dot output, but just don't get if the memos don't match. I'm undecided if it's out of scope to handle in this PR, but if we don't, then we need a follow-up issue.

}

prm, _ := ptr.ToReachMap(true, false, false, nil)
bs.Children = prm.Flatten(false)

This comment has been minimized.

@sdboyer

sdboyer Mar 3, 2017

Member

There's some filtering that needs to be done here. Simply calling Flatten will get you the imports of ALL the packages in a ptree. We need only the subset of imports that come from the packages we're actually importing from a project.

The lock should contain the imported package list for each project, though, so this shouldn't be that bad to do.

This comment has been minimized.

@Rhymond

Rhymond Mar 12, 2017

Contributor

@sdboyer we are doing some sort of "filtering".
https://github.com/golang/dep/pull/271/files#diff-0d223b461ecb49bf384a2c75a3893558R51
When we are generating Graphviz nodes we are only working with packages which are presented in the lock in this way we're excluding other packages.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 9, 2017

bump

@Rhymond

This comment has been minimized.

Copy link
Contributor

Rhymond commented Mar 10, 2017

Hi @sdboyer,

Sorry for late response and Thanks for review.
I'm going to make changes and update PR this weekend.

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 10, 2017

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 10, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 10, 2017

Please rebase instead of merging when catching up with the mainline.

@Rhymond Rhymond force-pushed the Rhymond:dot_output branch from 272b812 to a8d5000 Mar 10, 2017

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 10, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Mar 10, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 10, 2017

(And please rewrite the history to remove that merge commit, 4275dd0, which should be unnecessary post-rebase)

@freeformz

This comment has been minimized.

Copy link
Contributor

freeformz commented Mar 11, 2017

@Rhymond let me know if I can help.

@Rhymond Rhymond force-pushed the Rhymond:dot_output branch from a8d5000 to 20fcfef Mar 12, 2017

@Rhymond

This comment has been minimized.

Copy link
Contributor

Rhymond commented Mar 14, 2017

@sdboyer merge commit was removed

@sdboyer
Copy link
Member

sdboyer left a comment

Sorry for slow review, very busy week. Almost there, I think - just one bug to fix!

for _, dp := range g.ps {
for _, bsc := range dp.children {
for pr, hsh := range g.h {
if strings.HasPrefix(bsc, pr) {

This comment has been minimized.

@sdboyer

sdboyer Mar 17, 2017

Member

There's a small bug here. We need to check not only that there's a prefix match, but that the next character in the bsc is a slash. This comes up a lot in this problem space; there's an (unexported) helper for it in gps. It'd probably be fine to just copy the func over to here - though if you do, please adjust comments accordingly.

This comment has been minimized.

@Rhymond

Rhymond Mar 20, 2017

Contributor

Thanks!
Is status.go correct place for isPathPrefixOrEqual or it should be in graphviz.go?

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Member

Let's keep it as a helper just in graphviz.go for now. If/when we see wider use for it, we can move it into a more general location.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 17, 2017

Oh, also looks like Travis is complaining about some gofmt issues. Please make sure you gomft before committing!

@Rhymond Rhymond force-pushed the Rhymond:dot_output branch from a9b959a to 5427779 Mar 20, 2017

@Rhymond

This comment has been minimized.

Copy link
Contributor

Rhymond commented Mar 21, 2017

Fixed gofmt issues and helper were moved to dep.
Could you please confirm that comment is correct. Otherwise, please help with corrections. Thanks!

@sdboyer
Copy link
Member

sdboyer left a comment

Couple nits, rearrangements.

Also, I think we need some tests for this. It'd be fine to limit the tests just to the graphviz type - no need to mock up a real project for it to analyze.

@@ -91,3 +91,24 @@ func (dp gvnode) label() string {

return strings.Join(label, "\n")
}

// Ensure that the literal string prefix is a path tree match and

This comment has been minimized.

@sdboyer

sdboyer Mar 21, 2017

Member

Even though this isn't exported, it should follow the Go standard of having the comment block begin with the func name.

// Verify that either the input is the same length as the match (in which
// case we know they're equal), or that the next character is a "/". (Import
// paths are defined to always use "/", not the OS-specific path separator.)
func isPathPrefixOrEqual(pre, path string) bool {

This comment has been minimized.

@sdboyer

sdboyer Mar 21, 2017

Member

Now that I'm looking at this here, I realize the change in parameter order is terrible. Let's flip them so that it's the same as strings.HasPrefix() - string first, prefix second.

@@ -48,7 +48,7 @@ func (g graphviz) output() bytes.Buffer {
for _, dp := range g.ps {
for _, bsc := range dp.children {
for pr, hsh := range g.h {
if strings.HasPrefix(bsc, pr) {
if strings.HasPrefix(bsc, pr) && isPathPrefixOrEqual(pr, bsc) {

This comment has been minimized.

@sdboyer

sdboyer Mar 21, 2017

Member

I was imagining this as a standalone implementation - move the strings.HasPrefix() call inside isPathPrefixOrEqual, rather than requiring two calls out here.

return false
}

// we assume something else (a trie) has done equality check up to the point

This comment has been minimized.

@sdboyer

sdboyer Mar 21, 2017

Member

This comment should go away when strings.HasPrefix() moves into the func.

@Rhymond

This comment has been minimized.

Copy link
Contributor

Rhymond commented Mar 27, 2017

Hi, sorry for late response.
Will do changes this week

Rhymond added some commits Apr 1, 2017

Test cases for graphviz
Removed strings.HasPrefix() usage
@Rhymond

This comment has been minimized.

Copy link
Contributor

Rhymond commented Apr 5, 2017

Hi @sdboyer,
I made tests and changed .HasPrefix, could you please check if it's alright?

@sdboyer
Copy link
Member

sdboyer left a comment

just two tiny things 😄

// path separator.)
func isPathPrefix(path, pre string) bool {
pathlen, prflen := len(path), len(pre)
if pathlen < prflen || path[0:prflen] != pre || pathlen == prflen+1 {

This comment has been minimized.

@sdboyer

sdboyer Apr 7, 2017

Member

That last check, pathlen == prflen+1, was useful because the use pattern in gps guaranteed that trailing slashes were not a possibility on the path input. Does the same apply here? If not, we should drop that check.

This comment has been minimized.

@Rhymond

Rhymond Apr 9, 2017

Contributor

Yes you're right, it's unnecessary check, because there is no possibilities to have trailing slashes in manifest either in imports.

defer h.Cleanup()

b := g.output()
expected := h.GetTestFileString("graphviz/empty.dot")

This comment has been minimized.

@sdboyer

sdboyer Apr 7, 2017

Member

nit: use want instead of expected

Rename expected to want
Remove unnecessary check IsPathPrefix()
@freeformz

This comment has been minimized.

Copy link
Contributor

freeformz commented Apr 10, 2017

looks like it needs a rebase.

@Rhymond Rhymond force-pushed the Rhymond:dot_output branch from d74d1cb to 7fa0203 Apr 10, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 13, 2017

OK, I think we're finally ready to merge this in. Thanks so much!

@sdboyer sdboyer merged commit e4f1f3e into golang:master Apr 13, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#271 from Rhymond/dot_output
Dep status tree visualisation dot output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment