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

Implement new spec for init #470

Merged
merged 7 commits into from May 1, 2017

Conversation

Projects
None yet
3 participants
@darkowlzz
Copy link
Collaborator

darkowlzz commented Apr 27, 2017

  • Adds assuming project revision to be master when VersionInWorkspace
    fails for not on disk projects. Failed projects were being skipped from
    manifest before this change.
  • Adds running solver once at init. This ensures creating an initial
    lock with memo hash in lock file.

Fixes #356 and #149

@googlebot googlebot added the cla: yes label Apr 27, 2017

@darkowlzz darkowlzz changed the title Add constraints for not on disk projects. [WIP] Add constraints for not on disk projects. Apr 27, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 27, 2017

I would like to discuss if this is a good solution.

After debugging init, found out that the manifest variable, m, created in init refers to constraints of ProjectData here. And getProjectData() was omitting the projects from manifest when it fails to get version of projects not on disk here.

Instead of omitting the projects, I thought assuming the remote project's version to be master would be better. This ensured that all the direct, not on disk, dependencies are added to manifest.

Also, enabled running the solver to ensure that a proper lock is generated and memo is not left empty after init is run.

Need feedback.

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 27, 2017

Integration tests are failing due to change in the expected behavior/results. Will fix them once we decide to go ahead with this solution.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 27, 2017

Instead of omitting the projects, I thought assuming the remote project's version to be master would be better. This ensured that all the direct, not on disk, dependencies are added to manifest.

There'd be a lot of pitfalls from this. Much better is to run a solve without constraints applied, then pick the versions to put back into the manifest. Please have a look at the WIP spec doc - it describes how such an algorithm should work for picking a constraint.

Because it's out of scope for this issue to change init's flags, it'd be best to just use that algorithm for constraint selection - but (as it is now) only for projects for which we haven't gleaned a constraint from searching GOPATH.

Also, enabled running the solver to ensure that a proper lock is generated and memo is not left empty after init is run.

Excellent 😄

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 28, 2017

There'd be a lot of pitfalls from this.

oops! yeah, right. Not every project has the default branch named "master" 😅

if k == x.Ident().ProjectRoot {
m.Dependencies[k] = getProjectPropertiesFromVersion(x.Version())
break
}

This comment has been minimized.

@darkowlzz

darkowlzz Apr 28, 2017

Collaborator

Is there a better way to pick the constraints and add to manifests?

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

Not one that's wrapped up in a nice function, but there is something sorta analogous - I'll make more detailed comments down on the impl. But really, the algorithm you'll want to follow for this is bulleted in the spec doc. 😄

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 28, 2017

Another attempt and a little bit closer, I guess :)

Removed false version assumption for not on disk projects. Instead, running solver to get the proper version and then picking the missing constraints of not on disk projects from the solution.

Since the lock from the previous solver run had missing constraints, running solver again with appropriate project version(manifest) to obtain a final lock. Sounds good?

Also, with the above changes, manifest, lock and vendor are generated properly, but the lock Memo has a little issue. Here's an example scenario.

My project has 2 direct dependencies:

  • github.com/darkowlzz/openurl (not on disk)
  • github.com/fsnotify/fsnotify (on disk)

When init is run, fsnotify gets added to manifest first because it's on disk, hence becomes the first element of manifest map. After running solver and obtaining version of openurl, it is added to manifest map.

{map[github.com/fsnotify/fsnotify:{ master} github.com/darkowlzz/openurl:{ master}] map[] [] []}

This manifest map is then passed to the 2nd solver run to obtain the final lock, which is then written to manifest and lock files.

Now, if I run status, it fails to show expected result (list of packages) because status runs solver and compares it's solution's lock Memo with the memo in the lock file, and they don't match. On checking the inputs to the solver in status, I found that manifest map value is

&{map[github.com/darkowlzz/openurl:{ master} github.com/fsnotify/fsnotify:{ master}] map[] [] []}

openurl first and fsnotify second. Due to this the generated memo hash differs.

Any idea how to handle this?

@sdboyer
Copy link
Member

sdboyer left a comment

Great progress!

if k == x.Ident().ProjectRoot {
m.Dependencies[k] = getProjectPropertiesFromVersion(x.Version())
break
}

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

Not one that's wrapped up in a nice function, but there is something sorta analogous - I'll make more detailed comments down on the impl. But really, the algorithm you'll want to follow for this is bulleted in the spec doc. 😄

return err
}
l = dep.LockFromInterface(soln)
// Run solver again with appropriate constraint solutions from previous run

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

I don't think it's necessary to solve twice. It should be sufficient to gps.Prepare() a new solver with the returned solution and the updated manifest, then call solver.HashInputs() and use that as the Memo value instead of what was returned from the initial solve.

More details about how this hash works, and what it means, are here.

case gps.IsBranch, gps.IsVersion, gps.IsRevision:
pp.Constraint = v
case gps.IsSemver:
c, _ := gps.NewSemverConstraint("^" + v.String())

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

Let's add a TODO here to note that this will need to change when #225 is ready.

// gps.ProjectProperties with Constraint value based on the version type.
func getProjectPropertiesFromVersion(v gps.Version) gps.ProjectProperties {
pp := gps.ProjectProperties{}
switch v.Type() {

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

So, we actually need to do a little more work here up front. It's not a guarantee of the interface (yet - this may change), but gps has a concept of "version pairing" - basically, combining a revision with a branch or tag. This information is not encoded in the gps.Version.Type() method, so we need to tease it out first, because - per the spec - we never want to put revisions in Gopkg.toml, if we can avoid it.

There's a rough skeleton to follow here in status.go.

}

// getSolverSolution runs gps solver and returns a solution.
func getSolverSolution(root string, pkgT pkgtree.PackageTree, m *dep.Manifest, l *dep.Lock, sm *gps.SourceMgr) (gps.Solution, error) {

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

I don't object to having this abstraction in principle, but let's leave it for a later PR where it can be a dedicated refactor that affects the other commands as well.

func getProjectPropertiesFromVersion(v gps.Version) gps.ProjectProperties {
pp := gps.ProjectProperties{}
switch v.Type() {
case gps.IsBranch, gps.IsVersion, gps.IsRevision:

This comment has been minimized.

@sdboyer

sdboyer Apr 29, 2017

Member

Per the spec & the above note about revisions, if the actual type is gps.IsRevision, then we just want to ignore it.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 29, 2017

(for those things that weren't addressed inline...)

Another attempt and a little bit closer, I guess :)

Yep! 🥅

Removed false version assumption for not on disk projects. Instead, running solver to get the proper version and then picking the missing constraints of not on disk projects from the solution.

👍

Also, with the above changes, manifest, lock and vendor are generated properly, but the lock Memo has a little issue. Here's an example scenario.
...
openurl first and fsnotify second. Due to this the generated memo hash differs.

Ordering should not be an issue here. The input hasher is very, very careful about minimizing superfluous conflicts. It even removes irrelevant output (e.g. if a package is both imported and required, then the require value is not included in the hash, because it would be redundant and cause unnecessary hash conflicts). If gps' code is correct, it should be impossible for the caller to order inputs in a way that generate a superfluous conflict.

To get a better picture of why this conflict is happening, rather than printing the manifest itself, run gps.HashingInputsAsString() at various stages. That'll give a precise picture of what the problems are.

@darkowlzz darkowlzz force-pushed the darkowlzz:356-fix-init-solver-and-versioninworkspace branch from 92740cb to a4fd873 Apr 29, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 29, 2017

Another attempt with all my understandings 😅

  • Stopped running solver twice, using gps.Prepare() to obtain memo.
  • Removed solver solution abstraction created in the previous commit, because we are running solver just once now.
  • Modified getProjectPropertiesFromVersion() to comply more with the manifest constraints population algo as per the spec.

Also, the issue with different memo got resolved. I used gps.HashingInputsAsString() to check the hashing inputs at init:

-CONSTRAINTS-
github.com/darkowlzz/openurl
b-master-r-673a04d14da017f965f0b97f87ab369ec4a1fd08
github.com/fsnotify/fsnotify
b-master-r-fd9ec7deca8bf46ecd2a795baaacf2b3a9be1197
-IMPORTS/REQS-
github.com/darkowlzz/openurl
github.com/fsnotify/fsnotify
-IGNORES-
-OVERRIDES-
-ANALYZER-
dep
1

While on running status, hashing inputs were:

-CONSTRAINTS-
github.com/darkowlzz/openurl
b-master
github.com/fsnotify/fsnotify
b-master
-IMPORTS/REQS-
github.com/darkowlzz/openurl
github.com/fsnotify/fsnotify
-IGNORES-
-OVERRIDES-
-ANALYZER-
dep
1

Revision value was causing the issue. With the modified constraint selections algo implementation, it got fixed.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 30, 2017

(travis should be failing too, but we introduced a bug - i just opened #486 for that)

@sdboyer
Copy link
Member

sdboyer left a comment

idk about limited understanding, this is looking pretty good to me 😄 next step is probably to move on to adapting the tests.

@@ -209,6 +227,45 @@ func hasImportPathPrefix(s, prefix string) bool {
return strings.HasPrefix(s, prefix+"/")
}

// getVersionConstituents extracts version constituents
func getVersionConstituents(v gps.Version) (version, revision string) {

This comment has been minimized.

@sdboyer

sdboyer Apr 30, 2017

Member

This is actually really close to the VersionComponentStrings() func in gps. If we were to go this route, I think I'd rather see that function reused. But I think another route might be better, as I'll describe below...

pp := gps.ProjectProperties{}

// constituent version and revision. Ignoring revison for manifest.
cv, _ := getVersionConstituents(v)

This comment has been minimized.

@sdboyer

sdboyer Apr 30, 2017

Member

Rather than converting to and from strings, we can do it all with the types - in getProjectPropertiesFromVersion, we can do something like this:

switch tv := v.(type) {
case gps.PairedVersion:
	v = tv.Unpair()
case gps.Revision:
	return pp
}

Now the v is exactly what we'll want to assign into pp.Constraint, unless we're in the semver case.

pp.Constraint = gps.NewVersion(cv)
case gps.IsSemver:
// TODO: remove "^" when https://github.com/golang/dep/issues/225 is ready.
c, _ := gps.NewSemverConstraint("^" + cv)

This comment has been minimized.

@sdboyer

sdboyer Apr 30, 2017

Member

Let's check the error here and panic if we get one. That shouldn't be possible - any valid plain semver version should be able to have the caret operand prepended to it - but if we somehow missed something, then program behavior is undefined; panicking is the safe thing to do.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 30, 2017

Revision value was causing the issue. With the modified constraint selections algo implementation, it got fixed.

Ahh, of course! Also, awesome, really glad we're going to be able to knock that one out.

@darkowlzz darkowlzz force-pushed the darkowlzz:356-fix-init-solver-and-versioninworkspace branch from a4fd873 to 14c8cc6 Apr 30, 2017

if outsemver.Constraint.String() != expectedSemver.String() {
t.Fatalf("Expected %q to be equal to %q", outsemver, expectedSemver)
}
}

This comment has been minimized.

@darkowlzz

darkowlzz Apr 30, 2017

Collaborator

Can we do something about these tests? Especially the semver part. And if there's a way to improve the way they are being tested?

This comment has been minimized.

@sdboyer

sdboyer May 1, 2017

Member

Yeah, the issue here is that semver.rangeConstraint contains a slice, which means it's not directly comparable, even though that slice is almost always empty, including in this case.

The simple solution here is to use reflect.DeepEqual() to compare them instead.

Apart from that, I think the tests are actually fine as-is; they focus in on the key issue, which is stripping out the underlying revision, and making sure that we default to the caret. The only ones I might add, just for completeness, is a duplicate set where there isn't a revision attached (the Version is actually a gps.UnpairedVersion). This case shouldn't occur in the wild, but it's to test for and we should know if we ever regress in handling it, for some reason.

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented Apr 30, 2017

And we are almost there ☘️ 😁

  • Removed getVersionConstituents() and instead using a simplified inline implementation of the same.
  • Added error checking with panic for gps.NewSemverConstraint().
  • Added test for getProjectPropertiesFromVersion().
  • Fixed the failing integration tests due to new init behavior.
@sdboyer
Copy link
Member

sdboyer left a comment

good progress!

func TestGetProjectPropertiesFromVersion(t *testing.T) {
cases := []struct {
version gps.Version
expected gps.Version

This comment has been minimized.

@sdboyer

sdboyer May 1, 2017

Member

supernit: prefer want to expected

if outsemver.Constraint.String() != expectedSemver.String() {
t.Fatalf("Expected %q to be equal to %q", outsemver, expectedSemver)
}
}

This comment has been minimized.

@sdboyer

sdboyer May 1, 2017

Member

Yeah, the issue here is that semver.rangeConstraint contains a slice, which means it's not directly comparable, even though that slice is almost always empty, including in this case.

The simple solution here is to use reflect.DeepEqual() to compare them instead.

Apart from that, I think the tests are actually fine as-is; they focus in on the key issue, which is stripping out the underlying revision, and making sure that we default to the caret. The only ones I might add, just for completeness, is a duplicate set where there isn't a revision attached (the Version is actually a gps.UnpairedVersion). This case shouldn't occur in the wild, but it's to test for and we should know if we ever regress in handling it, for some reason.

darkowlzz added some commits Apr 27, 2017

Add constraints for not on disk projects.
- Adds assuming project revision to be master when VersionInWorkspace
fails for not on disk projects. Failed projects were being skipped from
manifest before this change.
- Adds running solver once at init. This ensures creating an initial
lock with memo hash in lock file.
Partially implements new init algorithm
- Adds running gps solver with project versions found on disk.
- Picks constraints from the solver's solution and adds them to
manifest projects with missing constraints.
- Runs solver again to obtain final lock with appropriate project constraints.
Fix issues with the new init algo implementation
- Avoid running solver twice and use gps.Prepare to get the final lock.
- Improve manifest constraints population based on the algo in the new spec doc.
Nit fixes to init algo and add test
- Removes getVersionConstituents(), inline its simplified implementation.
- Adds test for getProjectPropertiesFromVersion() function.
Nit fixes to tests
- Added some more test cases: version without any revision attached.
- Fixed breaking ensure/pkg-errors/case1 integration test.

@darkowlzz darkowlzz force-pushed the darkowlzz:356-fix-init-solver-and-versioninworkspace branch from 14c8cc6 to 0a0f777 May 1, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 1, 2017

  • Renamed expected to want in test cases.
  • Added test cases for versions without revision attached.
  • Fixed breaking ensure/pkg-errors/case1 integration test. init in an empty project (empty manifest) should also have a memo hash.

reflect.DeepEqual for comparing semvar constraints didn't work for some reason :(
For reflect.DeepEqual(outsemver.Constraint, wantSemver), got:

Expected semver to be ^1.0.0, got ^1.0.0

Ended up comparing their strings.

@darkowlzz darkowlzz changed the title [WIP] Add constraints for not on disk projects. Implement new spec for init May 1, 2017


// Test to have caret in semver version
outsemver := getProjectPropertiesFromVersion(gps.NewVersion("v1.0.0"))
wantSemver, _ := gps.NewSemverConstraint("^1.0.0")

This comment has been minimized.

@sdboyer

sdboyer May 1, 2017

Member

The actual problem here is an annoying implementation detail - it should work without needing to fall back on string comparison if you use ^v1.0.0 instead of ^1.0.0. I believe this kind of issue is improved in newer versions of the semver lib, but we're stuck with what we've got for now.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 1, 2017

Now all we need is to update the CLI helptext 😄

@sdboyer

sdboyer approved these changes May 1, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 1, 2017

LGTM! I'll tweak the helptext a bit on my own, directly - no sense holding up the functionality for that 😄

@sdboyer sdboyer merged commit aef755e into golang:master May 1, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
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#470 from darkowlzz/356-fix-init-solver-and-…
…versioninworkspace

Implement new spec for init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment