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

Introduce the dependency vendoring tool: dep. #551

Conversation

mandrigin
Copy link
Contributor

@mandrigin mandrigin commented Jan 16, 2018

Use commits from go-ethereum@release/1.7 for most of the dependencies.

TODO

Closes #223

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch from 107480d to 211bece Compare January 16, 2018 21:18
@mandrigin
Copy link
Contributor Author

Split changes into 2 commits to simplify reviewing:

  • actual changes in the configs;
  • dependencies update.

@adambabik
Copy link
Contributor

Have you created a Gopkg.toml in our go-ethereum fork already?

@mandrigin
Copy link
Contributor Author

@adambabik I did it in mandrigin/go-ethereum. There is a TODO for moving that to the status-im fork.
I just wanted to synchronize the effort with @divan about cleaning up status-im/go-ethereum.

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch 2 times, most recently from 04ee720 to 5813f1a Compare January 17, 2018 21:33
@mandrigin
Copy link
Contributor Author

rebased to the current develop, so make test-e2e works again.

@mandrigin
Copy link
Contributor Author

Double-checked difference between vendor directories in upstream go-ethereum@1.7 and this PR. In github.com there are no differences in source files. We just have a different set of packages here, but if the packages exists in both upstream and here the revision is the same.

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch 2 times, most recently from 823ee75 to 887468d Compare January 18, 2018 19:33
@mandrigin
Copy link
Contributor Author

@divan @adambabik I created develop branch in status-im/go-ethereum, pointed this PR to it.

@mandrigin
Copy link
Contributor Author

Hmm, is it by design, to lint vendor? Don't we need to ignore that?

@mandrigin mandrigin changed the title WIP: Introduce the dependency vendoring tool: dep. Introduce the dependency vendoring tool: dep. Jan 18, 2018
@mandrigin
Copy link
Contributor Author

Resolved WIP status. I'll take a look at the linter problem later.

@adambabik
Copy link
Contributor

Hmm, is it by design, to lint vendor? Don't we need to ignore that?

vendor/ is not linted (we specifically pass packages in lint.mk to gometalinter) but if there are files in linted packages that import some dependencies and types do not match, it can complain. In this case, it probably won't even compile.

@mandrigin
Copy link
Contributor Author

@adambabik, hmm, all the tests pass on my machine, both e2e and unit. Is that possible if it doesn’t compile?

@adambabik
Copy link
Contributor

Cache maybe in Travis? Have you checked that on Travis and your local machine there are the same files?

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch 2 times, most recently from 76fe80b to d8c66d3 Compare January 20, 2018 18:36
@mandrigin
Copy link
Contributor Author

@adambabik so it was an incompatibility of 2 libraries btcd and btcutil, but in the packages that we never use ourselves. Found a compatible version of btcutil (btcd is used by Geth).

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch from d8c66d3 to 5e93a5a Compare January 20, 2018 18:52
@mandrigin
Copy link
Contributor Author

@adambabik all the checks are green now.

@adambabik
Copy link
Contributor

@mandrigin awesome! Btw. these packages are used in extkeys which is used in geth/account.

@adambabik
Copy link
Contributor

@mandrigin are these constraints https://github.com/status-im/go-ethereum/blob/develop/vendor/vendor.json included in any Gopkg.* file?

@mandrigin
Copy link
Contributor Author

Yes, in the lock file.

@mandrigin
Copy link
Contributor Author

@divan @adambabik I added a short doc[1] on how to use dep and what are the ideas behind it. Please, review and share your opinion there.

@dshulyak
Copy link
Contributor

@mandrigin please do a push to status-go repository, it will run tests on public network

they are failing currently (#565) and it would be nice to see if this PR will affect them in any way

@mandrigin
Copy link
Contributor Author

@dshulyak done! https://travis-ci.org/status-im/status-go/builds/332265950 should have a test on a public network.

@dshulyak
Copy link
Contributor

thanks, looks like it doesn't, test hangs the same way :)

@adambabik
Copy link
Contributor

@mandrigin can you please post instruction how I can test it from scratch? Can I just delete vendor/ from status-go and dep ensure should do the trick or I need to do something more?

Copy link
Contributor

@dshulyak dshulyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works well, i have couple of questions/suggestions

  1. I am seeing this warning, shouldn't we use override for transitive deps?
However, these projects are not direct dependencies of the current project:
they are not imported in any .go files, nor are they in the 'required' list in
Gopkg.toml. Dep only applies [[constraint]] rules to direct dependencies, so
these rules will have no effect.

Either import/require packages from these projects so that they become direct
dependencies, or convert each [[constraint]] to an [[override]] to enforce rules
on these projects, if they happen to be transitive dependencies,
  1. Usually it is good practice to strip tests from vendor directory. Can we add find vendor -name '*_test.go' -delete until it is not support by dep ensure?

DEPENDENCIES.md Outdated
to be in-sync there. We want to reduce the regression scope.
Hence, we pin down all the dependencies of `go-ethereum` with SHAs in `Gopkg.toml` when
importing a new version of upstream. (This is considered a bad practice for
`dep` but we are willing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is missing here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in the latest push

2. Commit changes.


## Updating a dependency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it worth noting how to install dependencies from scratch, e.g dep ensure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever get a repo state when you need to install all deps from scratch? (aka with empty vendor)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess you are right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in the latest push

Gopkg.toml Outdated
@@ -0,0 +1,109 @@
[[constraint]] # Required to be compatible with `btcd`
Copy link
Contributor

@dshulyak dshulyak Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would like to separate our deps and go-ethereum, what do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, it doesn't seem to worth it.

  1. If we have SHAs in the lock file (as we should) it isn't propagated to the main dependency. That makes the versions we use in status-go slightly different from what go-ethereum is using. That is exactly what we want to avoid.

  2. If we have SHAs as constraints in go-ethereum/Gopkg.lock (via manual labour), we will have dependency resolution issues if a dependency is used both in status-go and go-ethereum (otto, for instance).

So the most stable method I found is to keep all the dependencies in Gopkg.toml of status-go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant just to put our own deps on top of the file, then go-ethereum and then all of the go-ethereum deps. but maybe thats not worth it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, that is a good idea, I'll do that.
One thing to note is that we won't have much of our dependencies in the toml file anyways.
The idea of this toml/lock split that the toml file contains only constrained dependencies. If we don't care about the version, we don't add this dep in there. So basically we will have:

  1. btcutils (constrained because btcd)
  2. go-ethereum (constrained because a custom source and a custom branch)
  3. geth dependencies.

dep ensures that SHAs are in the lock file and the components aren't updated until you specifically ask for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in the latest push

@dshulyak
Copy link
Contributor

Also our vendor became 112M with tests, 86M without tests. And before that it was 32M. Do you know what might be the reason for such big change?

@mandrigin
Copy link
Contributor Author

mandrigin commented Jan 24, 2018

@dshulyak that's because dep ensure w/o dep prune keeps all the sub-packages of a dependency. For instance, if we only use github.com/foo/package/subpackage1, it will download the whole github.com/foo/package. dep prune is supposed to fix that, but it will also kill all C sources that we want to keep.

@mandrigin
Copy link
Contributor Author

mandrigin commented Jan 24, 2018

@dshulyak

I am seeing this warning, shouldn't we use override for transitive deps?

How to reproduce that? Is it with clean vendor?

@dshulyak
Copy link
Contributor

@mandrigin at first i removed vendor and saw this warning, but after initial sync i can reproduce it as well

i suspect it might be related to dep version, i installed it in the morning)

dep:
 version     : devel
 build date  : 
 git hash    : 
 go version  : go1.9.2
 go compiler : gc
 platform    : linux/amd64

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch 2 times, most recently from fcf5ef7 to fe46ebb Compare January 24, 2018 11:49
@mandrigin
Copy link
Contributor Author

@dshulyak this warning... I can't reproduce it locally 😢

@dshulyak
Copy link
Contributor

dshulyak commented Jan 24, 2018

@mandrigin I just discovered that the code required for pruning unused packages and test files was merged and will be a part of next release. but in the documentation, we are recommending to use dep tool straight from GitHub, so maybe it makes sense to rely on those features and make our vendor folder smaller from the beginning. what do you think?

  1. Absorb prune into ensure golang/dep#944
  2. Add prune options to manifest golang/dep#1226

@dshulyak
Copy link
Contributor

Even if we can only remove tests it will reduce the size of vendor by ~30M

@mandrigin
Copy link
Contributor Author

@dshulyak I think it might make sense have a follow-up issue when the dep's functionality is released. Knowing which dependencies we use looks more important for me than the repo size.

Copy link
Contributor

@dshulyak dshulyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, sounds good to me

@dshulyak
Copy link
Contributor

regarding the warning, the result was identical anyway, so I assume we can figure out its meaning later

@mandrigin
Copy link
Contributor Author

@dshulyak #569

@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch 2 times, most recently from cbae543 to 4f748ce Compare January 25, 2018 10:53
Use commits from `go-ethereum@release/1.7` for most of the dependencies.
@mandrigin mandrigin force-pushed the igorm/investigate-vendoring-tool-223-dep-geth173 branch from 4f748ce to 9c50dbd Compare January 25, 2018 11:16
Copy link
Contributor

@themue themue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of dependencies is alway horrifying.

LGTM

@dshulyak
Copy link
Contributor

@mandrigin please rebase, i fixed mistake that blocked CI #572

@dshulyak
Copy link
Contributor

good news, you don't have to :) restart of the job helped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring: Introduce vendoring library
5 participants