[WIP] Import transitive constraints from glide

[chriswhelix]

When calculating constraints, we want to include any constraints specified by our dependencies, including if those constraints are specified in the dependency using another tool (e.g. glide). This is crucial to facilitate piecewise migration to dep. Full use case documented in #821.

Status:

• Add test cases for dep init
• Get dep init working
• Add test cases for dep ensure
• Get dep ensure working
• Check that Lock mismatch immediately after dep init #917 is fixed

fixes #821
fixes #917

[chriswhelix]

@sdboyer @carolynvs This test case fails to fail. What I'm trying to do here is import two direct dependencies each of which, in their respective glide.yaml, require incompatible versions of github.com/sdboyer/deptest -- ^1.0.0 and 0.8.1 respectively.

With some extra debug output from importManifestAndLock, I can see that correctly conflicting constraints are being read from the two glide.yaml files:

$ rm Gopkg.* ; 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/go/src/foo/glide.yaml
  Loading /tmp/go/src/foo/glide.lock
Converting from glide.yaml and glide.lock...
  Using gt100 as initial constraint for imported dep github.com/chriswhelix/deptestglide1
  Using eq081 as initial constraint for imported dep github.com/chriswhelix/deptestglide2
  Trying gt100 (d2925e7) as initial lock for imported dep github.com/chriswhelix/deptestglide1
  Trying eq081 (fff62a8) as initial lock for imported dep github.com/chriswhelix/deptestglide2
  Trying v1.0.0 (ff2948a) as initial lock for imported dep github.com/sdboyer/deptest
&dep.Manifest{Constraints:gps.ProjectConstraints{"github.com/chriswhelix/deptestglide1":gps.ProjectProperties{Source:"", Constraint:"gt100"}, "github.com/chriswhelix/deptestglide2":gps.ProjectProperties{Source:"", Constraint:"eq081"}}, Ovr:gps.ProjectConstraints(nil), Ignored:[]string(nil), Required:[]string(nil)}
&dep.Lock{SolveMeta:dep.SolveMeta{InputsDigest:[]uint8(nil), AnalyzerName:"", AnalyzerVersion:0, SolverName:"", SolverVersion:0}, P:[]gps.LockedProject{gps.LockedProject{pi:gps.ProjectIdentifier{ProjectRoot:"github.com/chriswhelix/deptestglide1", Source:""}, v:"gt100", r:"d2925e73d1fc12ac1f61ba86bb1983452bfe5b49", pkgs:[]string(nil)}, gps.LockedProject{pi:gps.ProjectIdentifier{ProjectRoot:"github.com/chriswhelix/deptestglide2", Source:""}, v:"eq081", r:"fff62a89d33d0ae0bfb23da76698212aadea45c9", pkgs:[]string(nil)}, gps.LockedProject{pi:gps.ProjectIdentifier{ProjectRoot:"github.com/sdboyer/deptest", Source:""}, v:gps.semVersion{sv:semver.Version{major:0x1, minor:0x0, patch:0x0, pre:"", metadata:"", original:"v1.0.0", special:0x0}}, r:"ff2948a2ac8f538c4ecd55962e919d1e13e74baf", pkgs:[]string(nil)}}}
Root project is "foo"
 13 transitively valid internal packages
 2 external packages imported from 2 projects
(0)   ✓ select (root)
(1)	? attempt github.com/chriswhelix/deptestglide1 with 1 pkgs; at least 1 versions to try
(1)	    try github.com/chriswhelix/deptestglide1@gt100
Importing configuration from glide. These are only initial constraints, and are further refined during the solve process.
&dep.Manifest{Constraints:gps.ProjectConstraints{"github.com/sdboyer/deptest":gps.ProjectProperties{Source:"", Constraint:gps.semverConstraint{c:semver.rangeConstraint{min:semver.Version{major:0x1, minor:0x0, patch:0x0, pre:"", metadata:"", original:"", special:0x0}, max:semver.Version{major:0x2, minor:0x0, patch:0x0, pre:"", metadata:"", original:"", special:0x0}, includeMin:true, includeMax:false, excl:[]semver.Version(nil)}}}}, Ovr:gps.ProjectConstraints(nil), Ignored:[]string(nil), Required:[]string(nil)}
&dep.Lock{SolveMeta:dep.SolveMeta{InputsDigest:[]uint8(nil), AnalyzerName:"", AnalyzerVersion:0, SolverName:"", SolverVersion:0}, P:[]gps.LockedProject{gps.LockedProject{pi:gps.ProjectIdentifier{ProjectRoot:"github.com/sdboyer/deptest", Source:""}, v:gps.semVersion{sv:semver.Version{major:0x1, minor:0x0, patch:0x0, pre:"", metadata:"", original:"v1.0.0", special:0x0}}, r:"ff2948a2ac8f538c4ecd55962e919d1e13e74baf", pkgs:[]string(nil)}}}
(1)	✓ select github.com/chriswhelix/deptestglide1@gt100 w/1 pkgs
(2)	? attempt github.com/chriswhelix/deptestglide2 with 1 pkgs; at least 1 versions to try
(2)	    try github.com/chriswhelix/deptestglide2@eq081
Importing configuration from glide. These are only initial constraints, and are further refined during the solve process.
&dep.Manifest{Constraints:gps.ProjectConstraints{"github.com/sdboyer/deptest":gps.ProjectProperties{Source:"", Constraint:gps.semverConstraint{c:semver.rangeConstraint{min:semver.Version{major:0x0, minor:0x8, patch:0x1, pre:"", metadata:"", original:"", special:0x0}, max:semver.Version{major:0x0, minor:0x9, patch:0x0, pre:"", metadata:"", original:"", special:0x0}, includeMin:true, includeMax:false, excl:[]semver.Version(nil)}}}}, Ovr:gps.ProjectConstraints(nil), Ignored:[]string(nil), Required:[]string(nil)}
&dep.Lock{SolveMeta:dep.SolveMeta{InputsDigest:[]uint8(nil), AnalyzerName:"", AnalyzerVersion:0, SolverName:"", SolverVersion:0}, P:[]gps.LockedProject{gps.LockedProject{pi:gps.ProjectIdentifier{ProjectRoot:"github.com/sdboyer/deptest", Source:""}, v:gps.semVersion{sv:semver.Version{major:0x0, minor:0x8, patch:0x1, pre:"", metadata:"", original:"v0.8.1", special:0x0}}, r:"3f4c3bea144e112a69bbe5d8d01c1b09a544253f", pkgs:[]string(nil)}}}
(2)	✓ select github.com/chriswhelix/deptestglide2@eq081 w/1 pkgs
(3)	? attempt github.com/sdboyer/deptest with 1 pkgs; at least 2 versions to try
(3)	    try github.com/sdboyer/[email protected]
(3)	✓ select github.com/sdboyer/[email protected] w/1 pkgs
  ✓ found solution with 3 packages from 3 projects

However, it still goes ahead and picks [email protected] without complaint, despite that violating the 2nd constraint.

Is this behavior intentional? If so, would it be worth emitting a warning about the conflict? If not, any suggestions for where to pursue fixing it?

[carolynvs]

I'll leave it to @sdboyer to say how "preferred versions" should work. I don't know enough about them to comment. 😀

[sdboyer]

sorry, i'm just seeing this now. unfortunately, i don't have a lot of guidance - assuming i'm reading that dump correctly, it does appear that those constraints are disjoint, which should cause a failure. (gps has scads of tests verifying that failures do occur when faced with disjoint constraints).

i might instrument something a little higher up, e.g. in bridge.go, to see what constraints are actually coming back from sm.GetManifestAndLock()

[carolynvs]

Don't worry about #917, that should be addressed in a separate PR.

[carolynvs]

I suggest that we split this PR into more manageable chunks:

• Have dep call the importers during DeriveManifestAndLock (i.e. use external config during solve).
• Add tests (and possible warnings/etc) for conflicting "preferred versions" from external config.
• Fix for Lock mismatch immediately after dep init #917.

This will make it easier for less difficult items to get into master, and hopefully shorten the review cycle.

That said, FYI I'm out the rest of this week and will only be checking in periodically.

[chriswhelix]

Getting back to this today. Unfortunately, it's hard to test against my more complex real-world projects because rejection of #830 means that with this change, not only can your glide.yaml not use a short hash, none of your dependencies' glide.yaml can use a short hash. Almost all our projects use libraries that break this rule and therefore cause the overall import to fail.

I've started looking into whether short hashes can be supported for import-only; however, it doesn't appear that SourceManager currently provides a way to translate a short hash into a long hash. The options I can see are:

1. Add a public function (GetRevisionForRevision?) to the SourceManager interface that accepts a short hash and returns a long hash. Then this could be called by the glide importer before InferConstraint.
2. Have InferConstraint take a flag that tells it whether to accept short hashes or not. This seems ugly but would be expedient.
3. Implement tool-specific versions of InferConstraint in each importer, so that InferConstraint doesn't need to deal with different revision semantics in different contexts. This seems like a lot of work and hard to make very DRY. Also, I'm not sure if that would then imply there should then be tool-specific SourceManagers as well.

@sdboyer @carolynvs would love your thoughts on how to proceed.

[sdboyer]

Unfortunately, it's hard to test against my more complex real-world projects because rejection of #830 means that with this change, not only can your glide.yaml not use a short hash, none of your dependencies' glide.yaml can use a short hash.

#830 is not a blocker for this. it was rejected as the proposal requested that we allow the use of shortened SHA1s anywhere. i was, however, explicit that autoconverting from short hashes in glide.yaml (or other systems) would be fine: #830 (comment)

Have InferConstraint take a flag that tells it whether to accept short hashes or not. This seems ugly but would be expedient.

@carolynvs made a suggestion along these lines the other day in slack, and it seems fine to me. not a hack at all, really. at least, not any more than InferConstraint() already is: it takes a string, and relies on interrogating the source, rather than pure symbolic logic (which is what the rest of the system does), to determine what well-formed constraint the string should corresponds to.

[carolynvs]

Yup! We have an issue on the backlog (#987) to gracefully handle dealing with short hashes found in external config.

[carolynvs]

FYI, #1027 was just merged, which handles expanding short revisions into full ones automatically in the importers.

@chriswhelix Let me know if you are still blocked one this one now that we have that.

[chriswhelix]

@carolynvs thanks, I'd been keeping an eye out for that, I'll pick this up again tomorrow probably.

[chriswhelix]

Current status: "dep init" seems to work well to import an initial config, but "dep ensure -update" won't continue to honor the transitive constraints used during init. The ensure solver will need to start using the import logic currently in rootAnalyzer. Putting that work on hold until @carolynvs finishes #1100.

[uberbits]

@chriswhelix do you have any update on this pull request? @carolynvs has merged #1100 into master.

[chriswhelix]

@uberbits I missed that that had merged, I'll take another look at this.

[uberbits]

@chriswhelix many thanks! The ability to read glide.yaml constraints is critical for our rollout of dep. It is critical for any other company with a large existing project base and complex dependency graph. With this change, we can start migrating leaf projects vs migrating entire clusters which include external (open-source) dependencies which are out of our control.

[uberbits]

@chriswhelix is there any way I can help with feature?

[chriswhelix]

@uberbits next step is making the solver use the importers during ensure. @carolynvs can probably provide some guidance on approach. It's looking like I may not be able to get back to this for a couple weeks, so feel free to pick it up.

[uberbits]

Thanks @chriswhelix ! Alex @alsamylkin would you please pick this up and follow up with @carolynvs ?

[uberbits]

@ChinmayR picking up on behalf of @alsamylkin

[carolynvs]

@uberbits Please ping me with any questions that come up here. I'm finally back online regularly and have time to help.

[carolynvs]

Closing this PR because it is being continued in PR #1277.