fix(status): add constraint for locked projects #962

darkowlzz · 2017-08-06T18:19:23Z

What does this do / why do we need it?

Uses project lock to get the constraints of projects not in manifest.

What should your reviewer look out for in this PR?

Correctness of implementation.

Do you need help or clarification on anything?

Before:

PROJECT                             CONSTRAINT     VERSION        REVISION  LATEST   PKGS USED
github.com/Masterminds/semver       branch 2.x     branch 2.x     139cc09   c2e7f6c  1
github.com/Masterminds/vcs          ^1.11.0        v1.11.1        3084677   3084677  1
github.com/armon/go-radix           *              branch master  4239b77   1fca145  1
github.com/go-yaml/yaml             branch v2      branch v2      cd8b52f   25c4ec8  1
github.com/nightlyone/lockfile      *              branch master  e83dc5e   6a197d5  1
github.com/pelletier/go-buffruneio  *              v0.2.0         c37440a   c37440a  1
github.com/pelletier/go-toml        branch master  branch master  fe206ef   69d355d  1
github.com/pkg/errors               ^0.8.0         v0.8.0         645ef00   645ef00  1
github.com/sdboyer/constext         *              branch master  836a144   836a144  1

With this change:

PROJECT                             CONSTRAINT     VERSION        REVISION  LATEST   PKGS USED
github.com/Masterminds/semver       branch 2.x     branch 2.x     139cc09   c2e7f6c  1
github.com/Masterminds/vcs          ^1.11.0        v1.11.1        3084677   3084677  1
github.com/armon/go-radix           branch master  branch master  4239b77            1
github.com/go-yaml/yaml             branch v2      branch v2      cd8b52f   25c4ec8  1
github.com/nightlyone/lockfile      branch master  branch master  e83dc5e            1
github.com/pelletier/go-buffruneio  v0.2.0         v0.2.0         c37440a   c37440a  1
github.com/pelletier/go-toml        branch master  branch master  fe206ef   69d355d  1
github.com/pkg/errors               ^0.8.0         v0.8.0         645ef00   645ef00  1
github.com/sdboyer/constext         branch master  branch master  836a144   836a144  1

For some reason, LATEST of go-radix and lockfile aren't showing up. Constraint.Matches() seems to be failing. Some help in figuring out the reason would be great.

Update: The missing LATEST above is because those projects don't have any release.

Which issue(s) does this PR fix?

Related to #893

sdboyer · 2017-08-06T18:29:19Z

cmd/dep/status.go

+					// Get constraint for locked project
+					for _, lockedP := range p.Lock.P {
+						if lockedP.Ident().ProjectRoot == proj.Ident().ProjectRoot {
+							c.Constraint = lockedP.Version()


We can't make this jump, unfortunately - the version in the lock doesn't really tell us anything about what the constraints were that got us there. For any possible version that would be in a lock, there are at least two possible constraints that would allow it:

An exact match for that version

An Any constraint

so yeah, we've gotta do the CollectConstraints thing in order to be able to fill this in accurately 😢

darkowlzz · 2017-08-08T13:12:28Z

@sdboyer I have implemented collectConstraints() from what I understood. Can you verify if this is how it should be done?

sdboyer

sorry i dawdled so much on this 😢

collectConstraints implementation looks good, though let's make sure to add some tests for it. after that, shouldn't be too much more work to get this squared away!

sdboyer · 2017-10-07T00:44:54Z

cmd/dep/status.go

+	// Create a root analyzer
+	rootAnalyzer := newRootAnalyzer(false, ctx, directDeps, sm)
+
+	// Iterate through the locked projects and collect constrains of all the projects


nit: s/constrains/constraints/

also, missing period

sdboyer · 2017-10-07T01:49:52Z

cmd/dep/status.go

+		for pr, pp := range pc {
+			// Check if a collection exists for a project root and append to
+			// existing one or create a new constraint collection.
+			if cc, exists := constraintCollection[string(pr)]; exists {


i think this can be safely shortened to a one-liner, as lookups to nonexistent keys on maps will return the zero value for the map's value type:

constraintCollection[string(pr)] = append(constraintCollection[string(pr)], pp.Constraint)

darkowlzz · 2017-10-29T12:19:55Z

@sdboyer I'm not sure how to write a test for this without creating new projects with the manifest file in them. Can we reuse some of your test repos for this? Or should I create repos of my own?

darkowlzz · 2017-11-02T14:47:27Z

I created https://github.com/darkowlzz/deptest-project-1 and https://github.com/darkowlzz/deptest-project-2 for testing collectConstraints() with different version of same project.

darkowlzz · 2017-11-12T13:37:23Z

Added projectConstraint and constraintsCollection types to store information about which project sets a constraint on a given dependency project. This would be required in #1367 to create PROJECT IMPORTERS list.

needs new review

Use project lock to get the constraints of projects not in manifest.

The resultant collection obtained from collectConstraints() should contain data about which project set a particular constraint on a dependency project. `projectConstraint` consists of the project and the constraint it sets on a given dependency. And `constraintsCollection` is a map to store the pair of dependency project and a collection of `projectConstraint` for different constraints. This would be used by project argument status (status for a single project), to show the importers of the project.

sdboyer · 2017-11-27T17:36:22Z

cmd/dep/status.go

+		// Iterate through the project constraints to get individual dependency
+		// project and constraint values.
+		for pr, pp := range pc {
+			constraintCollection[string(pr)] = append(


so this is overzealous right now - it pulls in constraints regardless of whether they're actually applicable or not. it's unlikely to create problems right now, but it will need to be updated in the future (please open an issue!) to do the equivalent of something like intersectConstraintsWithImports() or getApplicableConstraints()

googlebot added the cla: yes label Aug 6, 2017

sdboyer suggested changes Aug 6, 2017

View reviewed changes

darkowlzz force-pushed the status-transitive-constraint branch from 7cdb8f6 to ea8b946 Compare August 8, 2017 13:07

sdboyer previously requested changes Oct 7, 2017

View reviewed changes

darkowlzz force-pushed the status-transitive-constraint branch 2 times, most recently from 439d7f9 to 0d33290 Compare October 29, 2017 12:14

darkowlzz force-pushed the status-transitive-constraint branch from 0d33290 to 7bef6fc Compare November 2, 2017 14:44

darkowlzz force-pushed the status-transitive-constraint branch 2 times, most recently from 3632c8c to 8d2d1cd Compare November 12, 2017 13:34

darkowlzz mentioned this pull request Nov 14, 2017

status: add project status #1367

Closed

darkowlzz added 5 commits November 19, 2017 01:52

fix(status): add constraint for locked projects

b51249c

Use project lock to get the constraints of projects not in manifest.

fix(status): implement collectConstraints()

99f6de7

test(status): fix failing tests due to constraints from lock

62b077c

test(status): Add TestCollectConstraints

df694ee

darkowlzz force-pushed the status-transitive-constraint branch from 8d2d1cd to 1814fc3 Compare November 18, 2017 20:29

sdboyer approved these changes Nov 27, 2017

View reviewed changes

sdboyer merged commit 26f91c7 into golang:master Nov 27, 2017

darkowlzz mentioned this pull request Nov 28, 2017

collectConstraints() should collect applicable constraints only #1409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(status): add constraint for locked projects #962

fix(status): add constraint for locked projects #962

darkowlzz commented Aug 6, 2017 •

edited

Loading

sdboyer Aug 6, 2017

darkowlzz commented Aug 8, 2017

sdboyer left a comment

sdboyer Oct 7, 2017

sdboyer Oct 7, 2017

darkowlzz commented Oct 29, 2017 •

edited

Loading

darkowlzz commented Nov 2, 2017

darkowlzz commented Nov 12, 2017

sdboyer Nov 27, 2017

fix(status): add constraint for locked projects #962

fix(status): add constraint for locked projects #962

Conversation

darkowlzz commented Aug 6, 2017 • edited Loading

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?

sdboyer Aug 6, 2017

Choose a reason for hiding this comment

darkowlzz commented Aug 8, 2017

sdboyer left a comment

Choose a reason for hiding this comment

sdboyer Oct 7, 2017

Choose a reason for hiding this comment

sdboyer Oct 7, 2017

Choose a reason for hiding this comment

darkowlzz commented Oct 29, 2017 • edited Loading

darkowlzz commented Nov 2, 2017

darkowlzz commented Nov 12, 2017

sdboyer Nov 27, 2017

Choose a reason for hiding this comment

darkowlzz commented Aug 6, 2017 •

edited

Loading

darkowlzz commented Oct 29, 2017 •

edited

Loading