List out of date deps #1553

zkry · 2018-01-23T16:18:14Z

What does this do / why do we need it?

This is a continuation of #1402
This pull request implements the -old flag which displays the projects that could be upgraded given the current circumstance.

I made slight chagnes to the runStatusAll function to display the out of date projects.

What should your reviewer look out for in this PR?

Heres a summary of what I did. Is there a better way I could have done this?

Pass the cmd *statusCommand to the runStatusAll function so that this function and change it's behavior based on various flags.
If the -old flag was passed, run the solver.
Using the result of the solver, only call the BasicLine function on the projects whos revison is not the same as the revision the solver came up with.

Do you need help or clarification on anything?

I was wondering if there is a good way to test this functionality besides testing the helper function I created.

Which issue(s) does this PR fix?

fixes #1383

@darkowlzz

googlebot · 2018-01-23T16:18:19Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

zkry · 2018-01-23T16:19:42Z

@azbshiri Here's the new PR.

darkowlzz

Sorry for such a long delay, I was AFK.

I've added some inline comments for changes. Also, you can refer to #1402 (comment) for what is to be done next.

darkowlzz · 2018-02-05T16:04:17Z

cmd/dep/status.go

@@ -424,7 +432,7 @@ type MissingStatus struct {
 	MissingPackages []string
 }

-func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (hasMissingPkgs bool, errCount int, err error) {
+func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager, cmd *statusCommand) (hasMissingPkgs bool, errCount int, err error) {


Let's not do this and go back to a separate function runOld. This is the default status run function. Maybe we would rename it to "default" in the future. You can refer ensure.go to see how we have different functions for different run modes.

darkowlzz · 2018-02-05T16:09:22Z

cmd/dep/status.go

+				}
+			}
+
+			if err := out.BasicLine(bsMap[string(pr)]); err != nil {


Let's not touch this function at all. This whole function would be refactored into small functions in #1498 , and that would make things a lot better :) . So, let's move all these changes into a separate function runOld.

darkowlzz · 2018-02-05T16:10:21Z

cmd/dep/status.go

+	}
+	// If we're here, the solution was missing a project. Should never get here but just incase
+	return false, errors.New("solution missing project information for " + string(p.Ident().ProjectRoot))
+}


This is good. Lets add test cases for this.

darkowlzz · 2018-02-05T16:16:56Z

Also, if you can, please squash the 6 commits from previous PR to 1. Most of them are arbitrary small changes which can fit in a single commit.

zkry · 2018-02-06T16:06:49Z

Sounds good. I did the squashing for this and the other status issue one. I'll rearrange the code and add the tests then.

zkry · 2018-02-09T07:08:07Z

So I got the harness and unit test in as well as put the -old flag's functionality in it's own routine. A lot of the logic for the new routine is copied from the main one making it pretty long. Eventually it should be rewritten. Once therunStatusAll function is broken up, I can rewrite runOld. If you want I can even work on breaking up these routines and testing them.

darkowlzz

err... there seems to be some misunderstanding?
I've added some inline comments but in general, whatever was in #1402 looked exact and correct to me. The only thing that was left was to present the obtained result.

But this PR seems to be doing a lot of different things, which I don't think is needed for showing out dated projects. #1383 contains discussion about how this should have been.

Tell me if I'm overlooking something 🤔

Also, after squashing, azbshiri's commits seems to have gone away 😅 I meant to keep his changes in a single commit and build on top of it, because those changes were correct.

darkowlzz · 2018-02-10T17:14:27Z

cmd/dep/status.go

+
+	// Errors while collecting constraints should not fail the whole status run.
+	// It should count the error and tell the user about incomplete results.
+	cm, ccerrs := collectConstraints(ctx, p, sm)


This should be done only in the main status runStatusAll() for a complete picture of the dependency graph. In this case, we are just trying to find packages for which there's a new version available under the specified constraint.

darkowlzz · 2018-02-10T17:16:12Z

cmd/dep/status.go

+		return false, 0, errors.Wrapf(err, "could not set up solver for input hashing")
+	}
+
+	if err := out.BasicHeader(); err != nil {


We need a different struct for old status. BasicHeader contains too much details that we don't need.
If you see #1383 (comment) , that's kind of what we need.

darkowlzz · 2018-02-10T17:18:24Z

cmd/dep/status.go

+
+	slp := p.Lock.Projects()
+
+	if !bytes.Equal(s.HashInputs(), p.Lock.SolveMeta.InputsDigest) {


We need not check this as well. We are just checking for new versions of what we have in the lock, under the set constraints.

darkowlzz · 2018-02-10T17:33:43Z

So I got the harness and unit test in as well as put the -old flag's functionality in it's own routine. A lot of the logic for the new routine is copied from the main one making it pretty long. Eventually it should be rewritten. Once therunStatusAll function is broken up, I can rewrite runOld. If you want I can even work on breaking up these routines and testing them.

@zkry great that you were able to write the tests. But, as mentioned in the above review, there are too many things happening that we don't necessarily need for showing outdated projects. Let's change the implementation as mentioned above and modify the tests accordingly.

Hope the comments help. I can explain more if required :)

zkry · 2018-02-10T17:43:30Z

Sorry about that 🙈. I guess I misunderstood. For some reason I thought the scope of the runOld to be the same with the runStatusAll. I thought that the cases that runStatusAll had to check was also required for the runOld (like with all the different status types and what not).

So I'll cut down runOld to how it was at the beginning, create a new status type for it, address the other comments, and fix my squashing mistake.

Thanks for the help.

zkry · 2018-02-11T12:23:16Z

Ok, so I think I'm close. There was just a few implementation questions that I had when making these changes.

I created a new interface called oldOutputter becuase I didn't want to make all of the other outputters implement the old methods. I could just add the old output methods to outputter and put stubs in for template, dot, etc.
-old displays a * when there is no constraint specified which is different from what normal status displays. Is this ok?

darkowlzz

Sorry for another delay.

Looks good.

I've added some inline comments for checking the updates against projects in manifest. That would solve the issue of * you mentioned.

darkowlzz · 2018-02-17T18:25:45Z

cmd/dep/status.go

+	var oldStatuses []OldStatus
+	solutionProjects := solution.Projects()
+
+	for _, proj := range p.Lock.Projects() {


I think checking against manifest projects would make more sense, because constraints are defined in manifest. If a project has no constraint in manifest, we don't know the constraint at which to check for latest. Locked projects would contain transitive dependencies too.

Update: I just realized that this is fine. Below this, we should check if there's an entry in manifest for the solution project before comparing their lock and solution versions.

Ok, so if there isn't an entry in it's manifest it shouldn't go to the -old output as well right?

darkowlzz · 2018-02-17T18:28:02Z

cmd/dep/status.go

@@ -81,6 +82,13 @@ type outputter interface {
 	MissingFooter() error
 }

+// Only a subset of the outputters should be able to output old statuses


Let's end all the comments with a period (.). We have been doing that for new code comments :)

darkowlzz · 2018-02-17T18:36:56Z

cmd/dep/testdata/harness_tests/status/old_constraints/initial/main.go

@@ -0,0 +1,14 @@
+// Copyright 2016 The Go Authors. All rights reserved.


al3rez · 2018-02-11T13:36:25Z

cmd/dep/status.go

@@ -269,6 +309,15 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error {
 		return errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file")
 	}

+	if cmd.old {
+		if _, ok := out.(oldOutputter); !ok {
+			return errors.Errorf("invalid output command usesed")


There's a typo here. "usesed" -> "used"

And I think it would be an output format and not a command?

darkowlzz · 2018-02-18T10:17:03Z

cmd/dep/status.go

+	solutionProjects := solution.Projects()
+
+	for _, proj := range p.Lock.Projects() {
+		for i := range solutionProjects {


any reason for using an index number here and not the object directly?
for _, solnProj := range solutionProjects {

I see i is used below only to get the object at ith position.

To avoid iterating through lockProjects.
7a9180f

@azbshiri Right, part of an optimization. Makes sense. Thanks.

@zkry check if you can adopt that optimization and make it fast. Can be done separately once the logic is corrected.

Ok, I can do that. The reason I changed it to a nested loop was because I found the order for the lock and solution projects to be different. I'll have to check where/why they are not in the same order. Whatever the case, sorting the list that is out of order and then comparing them with one iteration would be faster(?).

So it turns out that solutionProjects is not sorted. I can sort it like it's done in runStatusAll (https://github.com/golang/dep/pull/1553/files#diff-cf328f431a592d0b360d509c6c5862c7R640).

On a side note, since locked projects is being sorted for good measures here, do you think I should also sort it runOld?

Benefit of sorting would be a consistent output, which is required for the tests to pass always. So yes, sort them.

@zkry can you change the loop to use the array element and not the index as shown in the first comment in this thread? I don't see the need of index, as discussed above, it came from some old code which we don't need any more.
Also, sorry for so much delay. I'll be able to get this merged as soon as you make the change. Or if you are busy, I can make the changes myself :)

No problem 🙂. I'll get this one in right away.

zkry · 2018-02-20T16:25:49Z

I fixed the lock-solution comparison loop, the typos, as well as change it so it doesn't show the * dependencies. 👍

darkowlzz

This is looking good. Almost there.

Made some inline suggestions about supporting template output, verbose mode, and possible issue with the optimization.

darkowlzz · 2018-02-25T16:20:26Z

cmd/dep/status.go

@@ -304,6 +354,9 @@ func (cmd *statusCommand) validateFlags() error {
 	opModes := []string{}

 	if cmd.old {
+		if cmd.template != "" {
+			return errors.New("cannot pass template string with -old")
+		}


Since -old has json output support, you might as well add template support :)

darkowlzz · 2018-02-25T16:29:03Z

cmd/dep/status.go

+		RootPackageTree: ptree,
+		Manifest:        p.Manifest,
+		// Locks aren't a part of the input hash check, so we can omit it.
+	}


Let's add support for log output when -v is passed, similar to https://github.com/golang/dep/blob/v0.4.1/cmd/dep/status.go#L445-L449 .
Since the solving takes some time, a verbose mode would be good to have to see what's happening.
Also, I think we should add a line telling the user that's it's solving and checking for updates. Right now, there's no message and it waits for long, which could be confusing to the user.

darkowlzz · 2018-02-25T16:35:36Z

cmd/dep/status.go

+
+	for i := range solutionProjects {
+		lProj := lockProjects[i]
+		sProj := solutionProjects[i]


Thinking about it now, I feel doubtful about this optimization. What if an update includes a new dependency,(a transitive dependency), in that case, the solution lock would have the new project and would be different from the existing lock, resulting failing this optimization and wrong results. What do you think?

darkowlzz · 2018-02-25T16:39:25Z

cmd/dep/status.go

+		if lProj.Ident().ProjectRoot != sProj.Ident().ProjectRoot {
+			// We should always be comparing the same projects and should enter be here.
+			return errors.New("Projects from the solution and lock are not in the same order")
+		}


I see this is ensuring that the project in lock and solution be the same at same index, but we can do better here, than just failing. Maybe avoiding the optimization and going back to more iterative comparison was the right thing to do?

That makes sense. I am thinking that way too. I can change it to the iterative way no problem 👍

zkry · 2018-02-26T19:12:11Z

I got those changes in. The -old flag should now work with -f and -v. I also got the comparison loop back to the way it was.

Merged with branch: azbshiri/feature/list-out-of-date-dependencies

darkowlzz · 2018-03-25T17:28:15Z

Thanks a lot.

zkry requested a review from darkowlzz as a code owner January 23, 2018 16:18

googlebot added the cla: no label Jan 23, 2018

zkry mentioned this pull request Jan 23, 2018

[WIP] Add runOld method to statusCommand #1402

Closed

darkowlzz reviewed Feb 5, 2018

View reviewed changes

zkry force-pushed the list-out-of-date-deps branch from f6ecc19 to 0db8cb2 Compare February 6, 2018 06:29

darkowlzz reviewed Feb 10, 2018

View reviewed changes

zkry force-pushed the list-out-of-date-deps branch from 1ad042b to 1ce6fdb Compare February 11, 2018 12:20

darkowlzz reviewed Feb 17, 2018

View reviewed changes

al3rez reviewed Feb 18, 2018

View reviewed changes

darkowlzz reviewed Feb 18, 2018

View reviewed changes

darkowlzz reviewed Feb 25, 2018

View reviewed changes

zkry force-pushed the list-out-of-date-deps branch from 680a7d8 to 90bf357 Compare February 26, 2018 19:08

zkry added 5 commits March 19, 2018 15:34

Squash merge with previous work done

9faf04b

Merged with branch: azbshiri/feature/list-out-of-date-dependencies

Perform fixes, add tests, add display methods

3596a82

Improve solution-lock comparison and other minor fixes

55c928d

Make -f and -v compatible with -old

7fc577b

Chage the for loop style

2935827

zkry force-pushed the list-out-of-date-deps branch from 90bf357 to 2935827 Compare March 19, 2018 12:45

darkowlzz removed the cla: no label Mar 25, 2018

darkowlzz merged commit dbbe73b into golang:master Mar 25, 2018

darkowlzz added a commit that referenced this pull request Mar 25, 2018

Update Changelog for #1553

4c86f27


		slp := p.Lock.Projects()

		if !bytes.Equal(s.HashInputs(), p.Lock.SolveMeta.InputsDigest) {

		@@ -0,0 +1,14 @@
		// Copyright 2016 The Go Authors. All rights reserved.

List out of date deps #1553

List out of date deps #1553

Conversation

zkry commented Jan 23, 2018

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

googlebot commented Jan 23, 2018

zkry commented Jan 23, 2018 • edited Loading

darkowlzz 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

darkowlzz commented Feb 5, 2018

zkry commented Feb 6, 2018

zkry commented Feb 9, 2018 • edited Loading

darkowlzz 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

darkowlzz commented Feb 10, 2018

zkry commented Feb 10, 2018 • edited Loading

zkry commented Feb 11, 2018

darkowlzz left a comment • edited Loading

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

darkowlzz Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

al3rez Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkry Feb 18, 2018 • edited Loading

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

zkry commented Feb 20, 2018

darkowlzz left a comment • edited Loading

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

zkry Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

zkry commented Feb 26, 2018 • edited Loading

darkowlzz commented Mar 25, 2018

zkry commented Jan 23, 2018 •

edited

Loading

zkry commented Feb 9, 2018 •

edited

Loading

zkry commented Feb 10, 2018 •

edited

Loading

darkowlzz left a comment •

edited

Loading

darkowlzz Feb 18, 2018 •

edited

Loading

al3rez Feb 18, 2018 •

edited

Loading

zkry Feb 18, 2018 •

edited

Loading

darkowlzz left a comment •

edited

Loading

zkry Feb 26, 2018 •

edited

Loading

zkry commented Feb 26, 2018 •

edited

Loading