Interpret strings as branches > semver constraints #715

carolynvs · 2017-06-03T17:43:26Z

Fixes #710. When a user supplied string in an imported config file, or specified to dep ensure, can be interpreted multiple ways, prefer the branch over a semver constraint.

In #710, glide.yaml specified v2 for https://github.com/go-mgo/mgo. When we assume that is a semver constraint, solve fails because the hinted revision in the lock (a commit on the v2 branch) doesn't satisfy the assumed constraint of ^2.0.0.

The new preferred match order for the user string is:

revision
branch
semver constraint
tag

I am giving preference of a semver constraint over a tag so that a bare version, 1.0.0, is interpreted more loosely with an implied caret, ^1.0.0, instead of the stricter exact match.

carolynvs · 2017-06-03T18:08:35Z

This isn't quite ready yet because doesn't work with the testdata from https://gist.github.com/ellotheth/e96f8f2c5fdd1b9e09e183970cfb276b.

Feedback doesn't look right when the constraint is a branch.
~~My test repo isn't quite right, getting a subpackage error for mikespook/gearman-go which is causing the wrong rev to be locked.~~ Nevermind, this is a problem with that repo, so I've removed it from the testcase.

$ dep init -v
Importing configuration from glide. These are only initial constraints, and are further refined during the solve process.
Detected glide configuration files...
  Loading /tmp/src/foo/glide.yaml
  Loading /tmp/src/foo/glide.lock
Converting from glide.yaml and glide.lock...
  Using ^1.1.0 as initial constraint for imported dep github.com/DataDog/datadog-go
  Trying 1.1.0 (0ddda6b) as initial lock for imported dep github.com/DataDog/datadog-go
  Using b79fee2 as initial hint for imported dep github.com/mikespook/gearman-go
  Using 29cc868 as initial hint for imported dep gopkg.in/mgo.v2
Searching GOPATH for projects...
Root project is "foo"
 1 transitively valid internal packages
 3 external packages imported from 3 projects
(0)   ✓ select (root)
(1)	? attempt github.com/DataDog/datadog-go with 1 pkgs; at least 1 versions to try
(1)	    try github.com/DataDog/[email protected]
(1)	✓ select github.com/DataDog/[email protected] w/1 pkgs
(2)	? attempt github.com/mikespook/gearman-go with 1 pkgs; at least 1 versions to try
(2)	    try github.com/mikespook/gearman-go@b79fee29655dcdce7ba4a738f8388f9862b6779e
(3)	✗   github.com/mikespook/gearman-go at b79fee29655dcdce7ba4a738f8388f9862b6779e has problem subpkg(s):
(3)	  	github.com/mikespook/gearman-go/worker has err (*pkgtree.LocalImportsError); required by (root).
(2)	    try github.com/mikespook/[email protected]
(2)	✓ select github.com/mikespook/[email protected] w/2 pkgs
(3)	? attempt gopkg.in/mgo.v2 with 1 pkgs; at least 1 versions to try
(3)	    try gopkg.in/mgo.v2@29cc868a5ca65f401ff318143f9408d02f4799cc
(3)	✓ select gopkg.in/mgo.v2@29cc868a5ca65f401ff318143f9408d02f4799cc w/1 pkgs
  ✓ found solution with 4 packages from 3 projects

carolynvs · 2017-06-11T18:18:39Z

@sdboyer This is ready for review, though beware that it is currently pulling into two other PRs, #729 and #744. I recommend limiting the github diff to just the last commit so you don't see the changes from those other PRs.

I needed another test case for deduceConstraint where a branch name could be interpreted as a semver. I added that to my deptest fork but let me know if it belongs elsewhere.

carolynvs · 2017-06-14T16:41:44Z

Rebased now that the other PRs are now merged.

carolynvs · 2017-07-03T14:05:16Z

@sdboyer @darkowlzz @ibrasho Bumping for great glory. 🇺🇸 🎇

ibrasho

Naming nits.

ibrasho · 2017-07-03T16:40:56Z

cmd/dep/root_analyzer.go

+// project based on the locked revision and the constraint from the manifest.
+// First try matching the revision to a version, then try the constraint from the
+// manifest, then finally the revision.
+func lookupVersionForLockedProject(pi gps.ProjectIdentifier, c gps.Constraint, rev gps.Revision, sm gps.SourceManager) (version gps.Version, warning error) {


What's the benefit of named return values here?

Also, naming the error warning seems a bit misleading.

For lookupVersionForLockedProject, it's an error that prevents returning the expected results. Consumers cannot rely on the version returned since it will be nil.

A consumer can call this value warning if it doesn't depend on getting a valid gps.Version value. But I would only call it warning here if consumers could, somehow, use the returned value if they didn't care about the warning.

I would only call it warning here if consumers could, somehow, use the returned value if they didn't care about the warning.

Thanks! That is a good way of thinking about err/warn, very helpful.

ibrasho · 2017-07-03T16:45:12Z

cmd/dep/godep_importer.go

-				warn := errors.Wrapf(err, "Unable to lookup the version represented by %s in %s. Falling back to locking the revision only.", pkg.Rev, pi.ProjectRoot)
-				g.logger.Printf(warn.Error())
-				version = revision
+			version, warning := lookupVersionForLockedProject(pi, nil, revision, g.sm)


I'd even call this variable err since the comment below explains why it was logged and ignored (same thing in glide importer).

carolynvs · 2017-07-05T15:05:39Z

I've renamed from warning -> err and removed the named return values.

darkowlzz · 2017-07-05T17:53:31Z

I went through it. LGTM 👍
cmd/dep/testdata/godep/expected_import_output.txt should have been changed too right? Refer: https://github.com/golang/dep/pull/742/files#r121284440

Just wondering why were the files renamed?
cmd/dep/testdata/glide/glide.lock → cmd/dep/testdata/glide/glide0.lock, etc

ibrasho

LGTM. 👍

carolynvs · 2017-07-05T22:29:16Z

Just wondering why were the files renamed?

I added a second testdata file, e.g. glide1.lock, then didn't need it later on, but the filename change persisted after I squashed all my commits. Fixing.... 😊

carolynvs · 2017-07-06T01:36:50Z

@darkowlzz reminded me that I promised that this PR would also fix the godep importer.
Improved which version is picked when a rev has multiple tags. When the manifest has a semver constraint, dep uses the constraint to filter the tagged versions. Since this only impacts the preferred version in the lock, the solver may still pick something different, but at least dep no longer suggests (and prints) something that is impossible. This stems from case in github.com/sdboyer/deptest, where rev ff2948a is tagged with both v0.8.0 and v1.0.0.

darkowlzz · 2017-07-06T10:12:46Z

cmd/dep/root_analyzer_test.go

+	wantV := "v1.0.0"
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected '%s', got '%s'", wantV, gotV)


How about adding a little context to the failure message, like we try to do everywhere and in alignment with the existing test in the file?
Same for the tests below 🙂

googlebot · 2017-07-06T14:14:17Z

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 they're okay 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.

carolynvs · 2017-07-06T14:15:29Z

@darkowlzz Thanks for calling me out on that! 😀 I've added context to the test failure messages and squashed.

googlebot · 2017-07-06T14:24:49Z

CLAs look good, thanks!

carolynvs · 2017-07-06T14:48:53Z

Derp, my rebase went awry. All fixed (hopefully). 🤞

When a user supplied string in an imported config file, or specified to dep ensure, can be interpreted multiple ways, prefer the branch over a semver constraint. In golang#710, glide.yaml specified v2 for https://github.com/go-mgo/mgo. When we assume that is a semver constraint, solve fails because the hinted revision in the lock (a commit on the v2 branch) doesn't satisfy the assumed constraint of ^2.0.0. The new preferred match order for the user string is: * revision * branch * semver constraint * tag I am giving preference of a semver constraint over a tag so that a bare version, 1.0.0, is interpreted more loosely with an implied caret, ^1.0.0, instead of the stricter exact match.

When selecting a preferred version for a lock (before solve) if the constraint is a semver range, ensure the version selected is compatible.

sdboyer · 2017-07-07T02:47:09Z

This seems good to me - can merge once we deal with those conflicts 😄

carolynvs · 2017-07-07T04:18:17Z

I've fixed the merge conflict.

googlebot added the cla: yes label Jun 3, 2017

carolynvs force-pushed the detect-branch-vs-tag branch from ac944f7 to 6e55120 Compare June 3, 2017 17:45

carolynvs changed the title ~~Interpret strings as branches > semver constraints~~ WIP: Interpret strings as branches > semver constraints Jun 3, 2017

carolynvs force-pushed the detect-branch-vs-tag branch from 6e55120 to f2d0d8c Compare June 7, 2017 16:35

This was referenced Jun 11, 2017

Godep importer #742

Merged

Add constructors for feedback logs #744

Merged

carolynvs force-pushed the detect-branch-vs-tag branch from f2d0d8c to 74056c0 Compare June 11, 2017 17:03

carolynvs changed the title ~~WIP: Interpret strings as branches > semver constraints~~ Interpret strings as branches > semver constraints Jun 11, 2017

carolynvs force-pushed the detect-branch-vs-tag branch 3 times, most recently from 74a0a26 to 62b206f Compare June 11, 2017 18:08

carolynvs force-pushed the detect-branch-vs-tag branch from 62b206f to 1570f17 Compare June 14, 2017 16:40

This was referenced Jun 17, 2017

move deduceConstraint to SourceManager #761

Merged

Using dep with a project depending on "gomail" results in unbuildable package #777

Closed

ibrasho reviewed Jul 3, 2017

View reviewed changes

ibrasho approved these changes Jul 5, 2017

View reviewed changes

darkowlzz approved these changes Jul 6, 2017

View reviewed changes

carolynvs force-pushed the detect-branch-vs-tag branch from a1f4e09 to e6c5754 Compare July 6, 2017 14:14

googlebot added cla: no and removed cla: yes labels Jul 6, 2017

carolynvs force-pushed the detect-branch-vs-tag branch 2 times, most recently from 1b181b9 to b77f0c1 Compare July 6, 2017 14:24

googlebot added cla: yes and removed cla: no labels Jul 6, 2017

carolynvs mentioned this pull request Jul 6, 2017

Improve revision detection in DeduceConstraint #800

Closed

carolynvs added 2 commits July 6, 2017 18:29

Select locked version compatible with the constraint

db62987

When selecting a preferred version for a lock (before solve) if the constraint is a semver range, ensure the version selected is compatible.

sdboyer approved these changes Jul 7, 2017

View reviewed changes

carolynvs force-pushed the detect-branch-vs-tag branch from b77f0c1 to db62987 Compare July 7, 2017 04:11

darkowlzz merged commit ddf5680 into golang:master Jul 7, 2017

carolynvs mentioned this pull request Jul 21, 2017

Cannot import gopkg.in package from glide #799

Closed

carolynvs deleted the detect-branch-vs-tag branch October 6, 2017 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interpret strings as branches > semver constraints #715

Interpret strings as branches > semver constraints #715

carolynvs commented Jun 3, 2017 •

edited

Loading

carolynvs commented Jun 3, 2017 •

edited

Loading

carolynvs commented Jun 11, 2017

carolynvs commented Jun 14, 2017

carolynvs commented Jul 3, 2017

ibrasho left a comment

ibrasho Jul 3, 2017 •

edited

Loading

carolynvs Jul 3, 2017

ibrasho Jul 3, 2017

carolynvs commented Jul 5, 2017

darkowlzz commented Jul 5, 2017 •

edited

Loading

ibrasho left a comment

carolynvs commented Jul 5, 2017

carolynvs commented Jul 6, 2017

darkowlzz Jul 6, 2017

googlebot commented Jul 6, 2017

carolynvs commented Jul 6, 2017

googlebot commented Jul 6, 2017

carolynvs commented Jul 6, 2017

sdboyer commented Jul 7, 2017

carolynvs commented Jul 7, 2017

Interpret strings as branches > semver constraints #715

Interpret strings as branches > semver constraints #715

Conversation

carolynvs commented Jun 3, 2017 • edited Loading

carolynvs commented Jun 3, 2017 • edited Loading

carolynvs commented Jun 11, 2017

carolynvs commented Jun 14, 2017

carolynvs commented Jul 3, 2017

ibrasho left a comment

Choose a reason for hiding this comment

ibrasho Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

carolynvs Jul 3, 2017

Choose a reason for hiding this comment

ibrasho Jul 3, 2017

Choose a reason for hiding this comment

carolynvs commented Jul 5, 2017

darkowlzz commented Jul 5, 2017 • edited Loading

ibrasho left a comment

Choose a reason for hiding this comment

carolynvs commented Jul 5, 2017

carolynvs commented Jul 6, 2017

darkowlzz Jul 6, 2017

Choose a reason for hiding this comment

googlebot commented Jul 6, 2017

carolynvs commented Jul 6, 2017

googlebot commented Jul 6, 2017

carolynvs commented Jul 6, 2017

sdboyer commented Jul 7, 2017

carolynvs commented Jul 7, 2017

carolynvs commented Jun 3, 2017 •

edited

Loading

carolynvs commented Jun 3, 2017 •

edited

Loading

ibrasho Jul 3, 2017 •

edited

Loading

darkowlzz commented Jul 5, 2017 •

edited

Loading