Initial integration of gps into glide

[sdboyer]

This pull request swaps in gps for much of the core glide engine.

This checklist is sorta starting in media res of a long-running pull request...but hey, better late than never.

This checklist represents what I think should be necessary to at least merge this PR, though not necessarily be a complete, finished conversion. It might be preferable to merge this into a separate branch, so we can then iterate on other issues in smaller PRs.

• Get Kubernetes working. This is a nice torture test, and so a pretty thorough exercise for gps.
• Store versions alongside revisions in the lock file.
• Add general support (a method? and then, ofc, an underlying field) for reporting the version of a manifest or lock file. (EDIT: there's no explicit version field, b/c that would be a shitshow for users. Instead, we use variations in the format to clearly delineate between legacy and current file formats)
• Create legacy versions of both cfg.Config and cfg.Lock, and have the unmarshaler detect and report accordingly.
• Fully convert:
  • glide install
  • glide get
  • glide update
• Remove dead code - the old resolver, etc.

[sdboyer]

Yeah, so...the addition of logrus, which in turns pulls in golang/x/sys for one tiny little thing, is a really gross thing to have to add to a committed vendor dir - it's almost 100k loc. Will try to deal with that later.

[sdboyer]

@technosophos @mattfarina feedback on the general approach taken in Analyzer, or even just specifically with the godep support I added in c708bf7, would be great.

Would also help keep the review down to a more manageable size later :)

[mattfarina]

@sdboyer first, wow... that's a huge changeset. It's going to take a little bit to look at that.

Second, in your quick review request of c708bf7, I'll need to look further at why you're asking for the manifest / lock pairing to understand what's going on.

I would prefer that the mechanism that looks for external imports be pluggable rather than hard coded. So, it would iterate over a list of the things rather than go one by one. Though, this is an area the current importer could really improve on.

[sdboyer]

@mattfarina the large changeset is almost entirely under vendor; as I noted above, it's mostly golang.org/x/sys, which is brought in by one tiny point in Sirupsen/logrus - and I'm gonna see if I can duck around that by using an earlier version.

If you look at diffs restricted to real, local code additions, there's very little as of now:

git diff master --stat -- *.go */*.go
  action/create.go       |   2 +-
  action/ensure.go       |   4 ++--
  action/name.go         |   2 +-
  cfg/config.go          |  60 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
  cfg/config_test.go     |  12 ++++++------
  cfg/lock.go            |  36 ++++++++++++++++++++++++++++++++++++
  dependency/analyzer.go | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  godep/godep.go         |  46 ++++++++++++++++++++++++++++++++++++++++++++++
  repo/installer.go      |  16 ++++++++--------
  repo/vcs.go            |   2 +-
  10 files changed, 260 insertions(+), 25 deletions(-)

I'll need to look further at why you're asking for the manifest / lock pairing to understand what's going on.

Right, this is the crux of what there is to grok/review. As currently designed, vsolver expects pretty much just one thing to be injected by an implementing tool: an instance of vsolver.ProjectAnalyzer. That's what I've started building so far, in dependency/analyzer.go.

The sole method analyzers currently have, GetInfo(), is responsible for returning both manifest-type data, and lock-type data, from what vsolver has determined to be the root of a project. Right now, that project root determination is very locked down - repo roots only. I expect that will change, at which point analyzers may also need to provide logic for, "given this path, find the closest project root." I've seen the logic glide has for that, so I'm not worried there - it's just, I'm not quite there yet with the core engine, as supporting it involves more work with the project/package level distinction.

vsolver needs both the manifest and lock as both are relevant to the solving process:

1. Manifests are used from both the root and non-root projects (obviously), as they provide the foundational dep lists and version constraints for which the solver is trying to find a solution.
2. In root projects, manifests also provide three additional things:
   1. Dev dependencies (implemented)
   2. Aliases, which are pretty much what they sound like (not yet implemented)
   3. Overrides, which allow the root package to define global rules that can supersede the constraints given by other projects (not yet implemented)
3. Locks are primarily used from the root project, as they provide us the version that we should try to end up with, unless a) something new in the solving process necessitates it change or b) the user explicitly requested an upgrade.
4. However, we also use locks from non-root projects to try to achieve more stability in deeper parts of the depgraph where no root lock has guidance. (not yet implemented)

This last idea is one that, as far as I know, will be unique to glide, as I first suggested in my article. @technosophos and I have discussed it several times as possibly being very helpful in dealing with the go project ecosystem "as-is" - full of locked deps from godep, etc.

I would prefer that the mechanism that looks for external imports be pluggable rather than hard coded.

That's seems doable. I don't think it's really either/or, though - we can have the hardcoded analysis for godep, gom, etc., and if those don't work, let a plugin have a go? Or let the plugin have a go first, and then fall back on those. Either way, my concern is that the use case for this does not immediately spring to mind (though glide obviously has a history with this, so I suspect you have something concrete), but the complications for caching the returned data (which vsolver will/does) do.

So, it would iterate over a list of the things rather than go one by one.

I'm confused, "iterating over a list" and "go one by one" seem like synonyms to me. Unless you're suggesting that they all run unconditionally, even if one indicates a successful analysis?

I guess that's possible. Seems to me you'd have a mess, though, and I'm not clear on what case that's trying to cover. Some project simultaneously using, say, godep and glide, maybe? If anyone is doing that, then erroring out seems like the saner thing to do than trying to make sense of a fundamentally nonsensical situation. (The user could always be encouraged to fork that bizarre project and fix it - which really seems saner to me than trying to have tooling figure out what manner of custom crazy happens from the combination of two systems).

[sdboyer]

Just rebased in order to (at least temporarily) get rid of golang/x/sys - it's only needed for solaris, so we can deal with it a little later. By rebasing, it's not clogging up the history, and it makes reading the diff more feasible.

[mattfarina]

@sdboyer Two pieces of feedback so far...

1. The use of github.com/Sirupsen/logrus is going to create an odd UX if debugging is enabled (which it should be). We should talk this part through.
2. Need to deal with dependencies in test files. This is a big ask and someone already started a PR. It's currently the most requested feature.

I'm going to keep poking at this but I wanted to record these before I switched off reviewing for the day.

[sdboyer]

The use of github.com/Sirupsen/logrus is going to create an odd UX if debugging is enabled (which it should be). We should talk this part through.

right, so, logrus is definitely an issue. ...sort of.

i wrote that logging layer while i was still getting the core solver algorithm right. initially, i imagined that it might be helpful for users, but immediately found (once i started using it to debug the solver) that it was difficult even for ME to use. so, i opened sdboyer/gps#5, the outcome of which will specifically be a trace logger, with output looking like the linked file. it will be, dare i say, 🎉 🎉 🎉

that said, i don't think there's actually a problem here. the reason i'm using logrus in the first place is because i needed a logging framework that the implementing tool could fully control, and inject as a dependency into the solver. and logrus meets those requirements, even though its current output is mostly useless. so, just set the log level to something below Info, and you won't hear a peep. this isn't an acceptable long-term solution, of course - but that's what the aforementioned issue is for.

Need to deal with dependencies in test files. This is a big ask and someone already started a PR. It's currently the most requested feature.

(we discussed a bit in gitter after you posted this, so reiterating a bit)

yes, this is crucial. we already do half of it now: reading the dev/test deps out of the manifest. the other half is ensuring we incorporate test files in static analysis. the (untested) code is already (mostly) written for that; i ultimately put it down because the requirement was still a bit murky for me, and i felt that the work required to integrate with glide would clarify it.

so, yeah, the static analysis part is high on the priority list. my plan was to incorporate it if not in this PR, then in the next one.

[sdboyer]

Small progress update:

The latest set of changes takes one approach at solving the issue raised in #391. It's provisional, hacky code, but it does the job well enough for now, and it can give some basis for discussion, at least.

In any case, having at least a temporary solution in place for that, I'm now up against the next barrier, which has turned out to be package-vs-project-level analysis. That is, since I'm working against glide itself here, and that annoying golang.org/x/sys/unix package is being brought in by logrus (as previously discussed)...well, that's not a repo root - golang.org/x/sys is.

Tackling this properly means refactoring to include at least some of the static analysis I mentioned in my last comment. There won't be much visible on this side, but there'll be a fair few changes on the vsolver side.

[mattfarina]

@sdboyer can you use the normal logger interface for your application and pass in logrus for your test runs? Then anything implementing the logger interface can be passed into vsolver. I can even make a logger interface for the msg package so it can work fluidly with Glide. I'm happy to discuss the details.

[sdboyer]

Note that #565 is now open to discuss the major changes that introducing gps into glide will entail.

[sdboyer]

The latest set of changes introduce a conversion to the new config and lock file formats. I still have some tweaks to make (upon further, post-merge reflection last night), but it's pretty close.

[mattfarina]

@sdboyer Thanks for the updates on this. I just pulled the latest and ran into another problem.

First, I ran go run glide.go up and it proceeded to successfully go through an update and generate a new glide.lock file exiting with a 0 status. This was great to see.

I next tried to run go run glide.go up again. I wanted to test if the new versions worked to generate new version (which should really come to the same conclusion that just happened). What I got was an error.

$ go run glide.go up
# github.com/Masterminds/glide/vendor/gopkg.in/yaml.v2
vendor/gopkg.in/yaml.v2/error.go:4: "ERROR: the correct import path is gopkg.in/yaml.v2 ... " evaluated but not used
# github.com/Masterminds/glide/vendor/github.com/sdboyer/gps
vendor/github.com/sdboyer/gps/constraints.go:53: undefined: semver.Constraint

The new glide.lock file reads:

hash: 3975bda2fdb43b0cccab2b038f89cbe099f5b6f25b77084b4769d7e15a1f62a8
updated: 2016-09-15T09:25:57.859680773-04:00
imports:
- name: github.com/armon/go-radix
  branch: master
  revision: 4239b77079c7b5d1243b7b4736304ce8ddb6f0f2
- name: github.com/codegangsta/cli
  version: v1.14.0
  revision: 71f57d300dd6a780ac1856c005c4b518cfd498ec
- name: github.com/kr/pretty
  version: go.r60
  revision: a6f3cf47d962d7c6eae13b363e2b860eb561e528
- name: github.com/Masterminds/semver
  version: v1.1.1
  revision: 8d0431362b544d1a3536cca26684828866a7de09
- name: github.com/Masterminds/vcs
  version: v1.8.0
  revision: fbe9fb6ad5b5f35b3e82a7c21123cfc526cbf895
- name: github.com/sdboyer/gps
  version: v0.10.0
  revision: 278a227dfc3d595a33a77ff3f841fd8ca1bc8cd0
- name: github.com/termie/go-shutil
  revision: bcacb06fecaeec8dc42af03c87c6949f4a05c74c
- name: gopkg.in/yaml.v2
  branch: master
  revision: bec87e4332aede01fb63a4ab299d8af28480cd96
testImports: []

Notice the version for semver here isn't right. You can see the update run at https://gist.github.com/mattfarina/4171e5f18db51a3ee8110a27e779854c. You'll see the message about the 2.x conflict.

What's going on?

[sdboyer]

The pushed commits should fix those issues. I've run glide up a few times now, with no error - hopefully it works the same for you :)

The single biggest issue was that the glide.yaml was only halfway converted. Like, it used the new branch key for some constraints, but all of the constraints were still under the old key name, imports, instead of under dependencies. There there was still this line in the output in your gist:

[WARN]  glide.yaml was in a legacy format. An attempt will be made to automatically update it.

which was an indication that it was using the legacy logic to interpret glide.yaml, causing all branch constraint keys to be silently ignored - the legacy handler just drops keys it doesn't know about. Now, while that's a nasty situation, I also think it's not a huge concern for actual users, because a half-converted glide.yaml is something that really only seems likely to happen in bootstrappy-type situations - that is, literally only for us, in the process of converting glide to the new system. (Not to say we couldn't/shouldn't necessarily think of ways to improve that autoconversion process - e.g., we could make it fail if the legacy importer sees a branch key)

Now, initially I was more concerned by something else: even if the root manifest was halfway converted (and thus it didn't pick up the branch: 2.x constraint on github.com/Masterminds/semver), gps has always relied on that version of semver, and included it in its manifest since the beginning. So, why wasn't glide picking up that constraint when visiting gps' manifest and applying it to the depgraph, causing us to get the right version of semver anyway?

Turns out, the answer's in the trace:

| | | | | | ? attempt github.com/sdboyer/gps with 1 pkgs; 30 versions to try
| | | | | | | try github.com/sdboyer/gps@v0.10.1
| | | | | | | ✗ github.com/sdboyer/gps@v0.10.1 depends on github.com/Masterminds/semver with 2.x, but that's already selected at v1.1.1
| | | | | | | try github.com/sdboyer/gps@v0.10.0
| | | | | | ✓ select github.com/sdboyer/gps@v0.10.0 w/1 pkgs

The halfway converted root glide.yaml specified branch: master for gps, but for the same reasons above with semver, that constraint got silently dropped. With no explicit constraint, we instead accept anything, which means the solver started working from the most recent semver downwards, beginning with v0.10.1. And indeed, the glide.yaml in gps@v0.10.1 is fully converted and correct, declaring that constraint on semver to be 2.x. But, because we'd already selected v1.1.1 earlier on, that was a conflict, so the solver did what the solver does: tried the next version of gps, v0.10.0, to see if that might work.

And that's where things got fun, because v0.10.0 ALSO had a halfway-converted manifest:

package: github.com/sdboyer/gps
owners:
- name: Sam Boyer
  email: tech@samboyer.org
import: # legacy name
- package: github.com/Masterminds/semver
  branch: 2.x # new key
  vcs: git
- package: github.com/Masterminds/vcs
  vcs: git
- package: github.com/termie/go-shutil
  version: bcacb06fecaeec8dc42af03c87c6949f4a05c74c
  vcs: git

So, we had the same problem repeating itself a layer down - glide's analyzer autoconverted that glide.yaml, silently dropped the branch constraint, which made it look like v0.10.0 was cool with any version of semver, which of course meant that there was no longer a conflict with the already-selected semver@v1.1.1 - so the algo picked gps@v0.10.0, and off we went.

Again, I think this is mostly a problem that's unique to the bootstrapping situation. However, this particular incident does change some of my feelings on how glide's analyzer is currently written. Right now, it tries really hard not to error on DeriveManifestAndLock(), because my thought was that it's better to include versions that might be right than to exclude them. This experience, however, even if it is mostly a product of bootstrapping, demonstrates just how fast those problems can become harmful and impossible for an end-user to debug. So, I think it's probably better to be conservative in those cases - if the metadata looks at all bad, just throw the version out.

Also, just to explain - the reason these half-conversions exist at all is because I had to deal with the ambiguity of semver-or-branch discussed in #391 early on, because of the 2.x branch name in github.com/Masterminds/semver. So, I introduced logic to allow that branch key long prior to formally creating the new/legacy format distinction. When those manifests were written and committed, they actually did work. They just don't now, because glide's logic has changed.

It's a nice reminder of how profoundly important "backwards" compatibility is in this domain - we kinda have to deal with all projects, across all time. Old forms don't vanish into the background in the way they often do with other software.

[mattfarina]

This has been merged into the gps-integration branch. We can continue the development there. Smaller PRs and incremental work.