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

Use dep as a dependency management tool #369

Closed
wants to merge 8 commits into from

Conversation

adambabik
Copy link
Contributor

@adambabik adambabik commented Sep 27, 2017

This PR changes all dependencies management to dep.

Done

Note

This change will depend on geth 1.7.0 as we removed a significant amount of code in there and go-ethereum unit tests work. With geth 1.6.7 I would need to fix quite a few of them.

Todo

  • consider creating a similar PR for status-im/go-ethereum and ethereum/go-ethereum. It would allow removing a great number of constraints from Gopkg.toml in this project (check out https://github.com/status-im/go-ethereum/tree/status/1.7.0-unstable-dep),
  • fix unit tests as still some packages are missing,
  • decide if we want to use dep prune and a bash script to get rid of unnecessary packages or we want to rely only on dep ensure and include ~1.2M additional LOC.

Makefile Outdated
go get -u github.com/golang/dep/cmd/dep
dep ensure

mkdir _vendor_keep
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just leave it without dep prune (until it works as we want it)? If dep prune is broken for now, introducing this obscure dirs manipulations will introduce technical debt.

It works fine without pruning, 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.

Right, but it turned out that we will add around 1.2M LOC because of one package that contains translation files... I'd like to try a different approach and remove only those translation packages and compare numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a huge fan of pruning myself - in many cases we need only a few files from dependency. But it should be completely automatic and reliable process, which dep at some point will be able to accomplish. So between "no pruning at all" (which takes more space, but that's still ok) and "full pruning" I'd choose second. But to "half-manual pruning using hack in Makefile" I'd prefer first one.

This script is undoubtedly doing its job now, but:

  • it's easy to forger about (I rarely use make command, for example, and always assume that standard tooling go build/go test/dep should work).
  • it's not cross-platform (imagine we have tomorrow some strange dev on Windows) - we'll need to rewrite this script to support Windows
  • it's error prone (each line potentially can fail and we don't handle errors in this script)
  • etc/etc.

So my suggestion is to wait until your issue in dep is resolved, perhaps create technical debt issue for prune. Moreover prune command will be eliminated from dep in the next releases, which only adds complexity in maintaining this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without pruning it will add 5029 files changed, 1145282 insertions(+), 1 deletion(-) for our fork of go-ethereum and 2631 files changed, 1018238 insertions(+) for status-go. I suppose we can handle that as it's a one-time operation.

This issue is blocked, for the time being, so no rush. We should wait for a comment from a team building dep and then decide what to do.

Regarding your comments:

  1. Most people do :) Makefile is understood by a broader group of developers, not only ones who are fluent in Go. Even if dep prune would work correctly, how many people would know that they need to run dep ensure && dep prune to properly set up dependencies? make install nicely hides this implementation detail.
  2. How is it not cross-platform?
  3. If any line fails, the script will just stop. No need to handle anything.

I don't like this hack either but adding so many unnecessary files just because a tool is broken also does not seem like a great idea. That's why I wanted to see if it's doable to make pruning work with a little help of bash.

Let's get back to this when we rebase on geth 1.7.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Just want to emphasize that we already have non-pruned dependencies, and most of the projects do as well. And unless deps are really huge (>100M), that amount of files doesn't really cause any harm. At least, comparing to the technical debt of a workaround.

As for the comments:

  1. Using Makefile and default tools aren't mutually exclusive things. Also, dep ensure && dep prune would be "standard" go dep ensure soon :)
  2. I don't think Windows is POSIX-compatible and commands like cp -R will work.
  3. It will likely leave _vendor_keep dir.

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 don't think Windows is POSIX-compatible and commands like cp -R will work.

I wouldn't worry about that. Without a proper shell like cygwin, one won't be able to run this Makefile anyway.

It will likely leave _vendor_keep dir.

Correct, it would need to be removed manually.

I got a response from the team building dep and including prune into ensure won't happen soon. We're talking about months.

Copy link
Contributor

@influx6 influx6 left a comment

Choose a reason for hiding this comment

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

It's good in my view to use more consistent vendoring bits, we should have the instructions on the install/setup readme. Doubt alot of people will immediately think of make and look for vendor commands in there.

"enode://ce6854c2c77a8800fcc12600206c344b8053bb90ee3ba280e6c4f18f3141cdc5ee80bcc3bdb24cbc0e96dffd4b38d7b57546ed528c00af6cd604ab65c4d528f6@163.172.153.124:30303",
"enode://00ae60771d9815daba35766d463a82a7b360b3a80e35ab2e0daa25bdc6ca6213ff4c8348025e7e1a908a8f58411a364fe02a0fb3c2aa32008304f063d8aaf1a2@163.172.132.85:30303",
"enode://86ebc843aa51669e08e27400e435f957918e39dc540b021a2f3291ab776c88bbda3d97631639219b6e77e375ab7944222c47713bdeb3251b25779ce743a39d70@212.47.254.155:30303"
"enode://b2e71ac32168cc263a507125d11e0d34e1d530cbe8c9ae81c8bc76d01c94616d4beec41160885577222ac547fcc5f03db31c55e1a56c4351d5f47a6ec62a41cc@51.15.218.125:30303"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are other hash nodes not required anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just because I branched from #353 as this PR depends on that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, guess someone will have to sort that out when we merge into develop, to ensure to add back the missing nodes.

@adambabik
Copy link
Contributor Author

It's good in my view to use more consistent vendoring bits, we should have the instructions on the install/setup readme.

I agree. We can definitely improve our Readme page.

@influx6
Copy link
Contributor

influx6 commented Oct 16, 2017

@adambabik A few merge conflicts, once fixed should should be alright for merged, am not sure if @tiabc has thoughts on this changed, but am equally fine having dep used for vendoring.

@tiabc
Copy link
Contributor

tiabc commented Oct 16, 2017

@adambabik is this really in progress? If so, please, finish it before starting any new issue.

@adambabik
Copy link
Contributor Author

@tiabc it's kind of blocked. Also, as we upgraded to geth 1.7.1 and 1.7.2 now, some work needs to be repeated.

And one more thing:

decide if we want to use dep prune and a bash script to get rid of unnecessary packages or we want to rely only on dep ensure and include ~1.2M additional LOC.

Any opinion on that?

@tiabc
Copy link
Contributor

tiabc commented Oct 17, 2017

I don't see any mentioned blockers here. What blocks it?

@adambabik
Copy link
Contributor Author

The question I posted in my previous comment is still not answered. The case is that dep has two commands now: dep ensure and dep prune. The latter one removes unused packages, however, it also removes packages with used C files if they don't contain any Go files.

We can rely only on dep ensure but then we will add around 1.2M LOC in the repo or we can hack it with some bash script that I crafted.

I talked only to @divan and he is for using only dep ensure. I am also ok with it as in the future, dep will incorporate dep prune into dep ensure. The cost of adding 1.2M LOC is a one-time thing so it should not be a problem.

@tiabc
Copy link
Contributor

tiabc commented Oct 22, 2017

@adambabik please, confirm I understand the problem correctly.

Currently, dep prune removes everything but Go files and it makes problems for go-ethereum compilation. And the options are:

  1. Use dep ensure and "custom" dep prune and not include 1M extra LoC to our repo.
  2. Use only dep ensure, include 1M extra LoC to our repo, wait until dep prune is merged into dep ensure with a non-go flag which we can utilize not to delete C files.

If so, I'd choose the 2nd option. 1M lines is about 45 Mb (if we suppose 50 as an average line length) and I don't really care about 45 Mb. At the same time, I agree with @divan that it'd simplify dependencies management and avoid adding technical debt.

So, let's just do dep ensure?

@adambabik
Copy link
Contributor Author

@tiabc yes, it's correct.

Ok, let's proceed with only dep ensure. I will update the issue and close this PR as it's outdated.

@divan
Copy link
Contributor

divan commented Oct 28, 2017

@adambabik @tiabc why this PR is outdated? I think it's better to choose any of the proposed options and tune/optimize it on the go. We would benefit already from having vendoring tool in place.

If PR is relatively orthogonal to most of the changes in develop, so I guess it's easy to merge/rebase it.

@adambabik
Copy link
Contributor Author

@divan

why this PR is outdated?

Because it was based on the old geth (1.7.0). #223 was updated to reflect decisions discussed here.

@adambabik adambabik deleted the feature/vendor-library-223 branch January 11, 2018 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants