Skip to content
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

Refactoring: Introduce vendoring library #223

Closed
tiabc opened this issue Aug 1, 2017 · 24 comments · Fixed by #551
Closed

Refactoring: Introduce vendoring library #223

tiabc opened this issue Aug 1, 2017 · 24 comments · Fixed by #551

Comments

@tiabc
Copy link
Contributor

tiabc commented Aug 1, 2017

Problem

Currently our vendor folder is filled with a lot of uncontrolled dependencies and:

  1. We don't know which versions of our dependencies we use.
  2. We can't control which of them are really used and which aren't.
  3. They're hard to control.

However, it's not as straightforward as we have our own changes to some libraries under the vendor folder, for example, for go-ethereum. The solution here is to fork go-ethereum and support our version separately from status-go.

Implementation

We decided to use dep as a dependency management tool.

The recommended way is to first specify dependencies in our fork of go-ethereum and then using our fork as a dependency in status-go. Dependencies between these two projects will be properly split and easier to manage.

Acceptance Criteria

  1. dep is the only tool to manage dependencies in both status-go and our go-ethereum fork,
  2. After running dep ensure the project compiles properly and passes all tests,
  3. dep status returns no issues.

Notes

Note that we already have https://github.com/status-im/go-ethereum library forked and you'll need to find out the difference between it and that under vendor/ethereum/go-ethereum.

The first implementation was done #369. Read a discussion there before implementing. dep prune will remove some files which are necessary to build go-ethereum and status-go, thus it should not be used. The cost will be additional ~1M LOC which is not a big deal as it's a one-time operation.

@tiabc tiabc added this to the Beta milestone 2 of 6 milestone Aug 1, 2017
@adambabik
Copy link
Contributor

I am not sure we need #198 to be blocking this issue. Most of the mentioned vendor tools can figure out a list of dependencies from the source files. The challenge might be to sync dependencies versions...

From my experience:

  1. glide is the most complex but also provides the best feedback. I was constantly hitting small issues though.
  2. govendor is in the middle in terms of complexity.
  3. godep is very basic but also little things can go wrong with it. Currently, it's my favourite tool.

@tiabc tiabc removed this from the Beta milestone 2 of 6 milestone Aug 13, 2017
@adambabik
Copy link
Contributor

There is one more that might be an official one https://github.com/golang/dep

@tiabc
Copy link
Contributor Author

tiabc commented Aug 23, 2017

I think this is the first option we should consider.

@tiabc tiabc mentioned this issue Sep 17, 2017
@tiabc tiabc added this to the 0.9.12 milestone Sep 17, 2017
@b00ris b00ris self-assigned this Oct 25, 2017
@b00ris b00ris removed their assignment Nov 27, 2017
@divan divan self-assigned this Dec 1, 2017
@divan divan added this to To Do in Maintaining code quality via automation Dec 1, 2017
@divan divan moved this from To Do to In Progress in Maintaining code quality Dec 1, 2017
@divan
Copy link
Contributor

divan commented Dec 1, 2017

I'm going to do a second try on it. Here are the challenges we have with dep:

  • ideally, we want to keep 100% the same versions that currently in the vendor/ folder (i.e., dep init will download latest stable versions by default, but we want to minimize risks incurred by changing vendored libs now)
  • the way we use go-ethereum is a bit weird: repo status-im/go-ethereum actually contains all imports to ethereum/go-ethereum (so it's dependency) and our own patches, then it's used as dependency by import name github.com/ethereum/go-ethereum.

@adambabik
Copy link
Contributor

@divan regarding the second point, it's actually useful to do it this way as we don't need to change all github.com/ethereum/go-ethereum/... imports. Also, dep supports such a case: https://github.com/status-im/status-go/pull/369/files#diff-836546cc53507f6b2d581088903b1785R8. As we should strive for reducing the number of patches in go-ethereum, I would stay with this approach.

@mandrigin
Copy link
Contributor

@divan @adambabik I'll take this one over. Chances are, by the time I'll finish it, 1.7.3 would be rebased.

@mandrigin mandrigin self-assigned this Jan 9, 2018
@adambabik
Copy link
Contributor

I wonder if we should actually start working on this issue before go-ethereum decides to adopt dep. Here is an issue and what they think about it now.

What problem do we want to address with dep? The most crucial thing for us is to have the same version of dependencies as go-ethereum in order to stay compatible and avoid unexpected behavior.

We need to figure out a way to sync our and go-ethereum dependencies wherever we decide to upgrade go-ethereum. Does dep will solve that? Or maybe we should stick to govendor, i.e. the tool used by go-ethereum?

When I experimented with dep, it was not easy to keep the same versions (without writing git SHA manually into a configuration file) and it introduced a lot of additional (sub)packages that were not used.

@mandrigin
Copy link
Contributor

Sticking to govendor might make sense because we should be able to re-use their vendor/vendor.json file in that case.
I can start investigating that first.
Does anyone have a strong opinion on that?

@adambabik
Copy link
Contributor

If govendor can read vendor.json recursively that would be great. We could have very minimalistic vendor.json and all dependencies would be loaded from go-ethereum's vendor.json. (dep can do that, i.e. it reads its config files recursively.)

I don't have a strong opinion regarding the tool, however, I hope dep will be a standard soon. Having said that, if govendor is a better solution, for the time being, I would go for it.

dep is capable of migrating dependencies from popular dependency managers so even if govendor is better now, it will be trivial to migrate to dep.

@JekaMas
Copy link
Contributor

JekaMas commented Jan 9, 2018

I agree that govendor is a better solution for now and in our conditions. Could we made migration fast? If not may be we dont need any new package manager?

@adambabik
Copy link
Contributor

I think it depends if govendor can recursively download dependencies. @mandrigin will research that. If yes, we will just need to specify go-ethereum as a dependency plus a few others so it should be quick. If it does not work recursively, it may be more problematic.

@mandrigin
Copy link
Contributor

mandrigin commented Jan 10, 2018

I did a short research of govendor.
It has its own caveats: it ignores the remote vendor.json when fetching dependencies. There are workarounds for that though.

I will try to do a short investigation of dep and see how it compares to govendor.
I'll post a longer version of my investigation later.

It also turns out that we have quite a few dependencies in our tree that are either never used or duplicates with different versions.

Hence, I am convinced that we need a dependency manager.
The question is now which one to use.

@divan
Copy link
Contributor

divan commented Jan 10, 2018

@mandrigin make sure you've seen this (closed) PR: #369

We agreed on using dep (it's going to be an official go subcommand soon anyways), so the challenge is to make it work nicely with our forked go-ethereum clone. Using status-im/go-ethereum with patches (#492) should facilitate it.

Also (I don't remember where discussion is, but it's somewhere here in issues on GH), part of the problem is that for existing dependencies we currently use there is no .git folder and exact version information. So it should be either upgraded to latest (and verified that it's ok), or guessed the current versions and specified manually in Gopkg.toml.

@adambabik
Copy link
Contributor

@mandrigin if govendor does not support remote vendor.json then I would not spend much time on it. dep is the tool that will be used in the future by the whole community.

BTW, there is an update on dep golang/dep#944 (improvements to pruning that did not work well for us when we tried dep for the first time).

@mandrigin First, I would try to research migration from govendor to dep in our fork of go-ethereum. Our objective for dep must be to use the same version of dependencies as go-ethereum does. dep has some automatic way to do that and it worked well. The only problem was pruning. Maybe this branch of dep handles it better.

Also, I think we can wait until pruning in dep is fixed. I don't think we have any burning issues with dependencies now.

@mandrigin
Copy link
Contributor

mandrigin commented Jan 11, 2018

@adambabik yeah, the workaround I mentioned is as follows.
We have vendor.json in our /vendor directory. It has go-ethereum as a dependency.
Then we do the following sequence.

govendor sync # downloads go-ethereum and it's vendor.json
pushd vendor/github.com/ethereum/go-ethereum
govendor sync # downloads the deps of go-ethereum
popd

That relies on a fact, that go supports nested vendor folders and chooses the longest path.

When there are multiple possible resolutions, the most specific (longest) path wins.

(from Go Vendor Experiment)

I'll not spend more time on govendor today, I'll take a look what dep can help us with.

UPD: a better example of nested vendoring

@mandrigin
Copy link
Contributor

Ok, I manage to make it build and the tests to pass using dep and an external fork of go-ethereum.

Unfortunately, our current 1.7.2-unstable-dep doesn't work with this scheme, so I will create 1.7.2-stable-dep in out fork, convert the dependencies from govendor and apply a missing patch there.

After that, I can prepare dependencies for 1.7.3 in the dep format.

@divan what is the best place to document the process of adding a new version? Converting dependencies format would be one of the steps there.

@dnephin
Copy link
Contributor

dnephin commented Jan 15, 2018

I would suggesting adding a "vendor check" to CI. The vendor check would run dep ensure and then git diff to validate that any changes to the vendor directory are a result of changing the Gopkg.* files, and not manually applied diffs. check-git-diff is a good example of this.

The failure message from that check can provide the documentation about how to update dependencies, or link to relevant docs (if it's more involved than https://github.com/golang/dep#adding-a-dependency or https://github.com/golang/dep#changing-dependencies, but it ideally shouldn't be).

@mandrigin
Copy link
Contributor

That makes sense if we will continue to keep vendor under git.
I'm leaning towards not having it there, forcing CI to do dep ensure and download the dependencies every time.

@sdboyer
Copy link

sdboyer commented Jan 16, 2018

noticed this on a backlink from the dep repo - hi! just a quick note:

I would suggesting adding a "vendor check" to CI. The vendor check would run dep ensure and then git diff to validate that any changes to the vendor directory are a result of changing the Gopkg.* files, and not manually applied diffs.

yes, definitely important to do. you could base it off of what we do in dep itself: https://github.com/golang/dep/blob/master/hack/validate-vendor.bash

note that, as outlined here, there'll be a first-class dep check command at some point in the coming months that will perform this check without the need to duct-tape anything together.

@mandrigin
Copy link
Contributor

@sdboyer thanks a lot, this script seems very interesting indeed!

@adambabik @divan I think we can skip CI checks and dep prune for the first PR. Getting dependencies under control is a big enough issue in itself, and we can create a few more separate issues to introduce pruning and a CI check.

@mandrigin
Copy link
Contributor

@sdboyer btw, a side question. If I want to change an commit for a dependency (for triaging bugs), is it enough to just change Gopkg.lock and do dep ensure?

@adambabik
Copy link
Contributor

@mandrigin

I think we can skip CI checks and dep prune for the first PR.

We need to skip it because dep prune will remove some packages that have only C files and status-go won't compile. At least that was the problem before golang/dep#1223. I suppose that we can't use prune until golang/dep#944 is merged.

If I want to change an commit for a dependency (for triaging bugs), is it enough to just change Gopkg.lock and do dep ensure?

I guess you should never change Gopkg.lock by yourself. I believe you can use constraint revision to use a specific commit.

@mandrigin
Copy link
Contributor

@adambabik I tried putting specific revision constraints to go-ethereum's Gopkg.toml. We hit the following issue with it.
master: Could not introduce <dependency>@master, as it has a dependency on github.com/mandrigin/go-ethereum with constraint <sha>, which does not allow the currently selected version of master

The preferred way of pinning dependencies is a lock file as I understand(1). This file is preserved until dep ensure --update is called.

Also, this question was more out of curiosity, I'm relying on auto-conversion to generate the toml/lock pair.

@sdboyer
Copy link

sdboyer commented Jan 23, 2018

btw, a side question. If I want to change an commit for a dependency (for triaging bugs), is it enough to just change Gopkg.lock and do dep ensure?

in general, directly editing Gopkg.lock is an antipattern, and you can accomplish the same via dep ensure -update <name of thing>, depending on the constraint you have expressed in Gopkg.toml.

but yes, in a pinch, you can directly update Gopkg.lock, then run dep ensure -vendor-only.

the next release of dep - i'm trying for this week - comes with a big docs drop.

The preferred way of pinning dependencies is a lock file as I understand(1). This file is preserved until dep ensure --update is called.

yes, strongly preferred. revisions in your Gopkg.toml is generally an antipattern. this'll also be in the new docs :)

Maintaining code quality automation moved this from In Progress to Done Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

8 participants