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 to manage dependencies #465

Merged
merged 5 commits into from
Sep 19, 2017
Merged

Use dep to manage dependencies #465

merged 5 commits into from
Sep 19, 2017

Conversation

rfay
Copy link
Member

@rfay rfay commented Sep 17, 2017

The Problem/Issue/Bug:

We've been wanting to experiment with or move to godep for managing dependencies.

How this PR Solves The Problem:

godep with PR golang/dep#1079 actually seems to almost handle our dependencies.

It did get the branch wrong for cheggaaa/pb, and I had to manually change the constraint in Gopkg.toml and then dep ensure

[[constraint]]
  name = "github.com/cheggaaa/pb"
  version = "1.0.18"

I don't actually know about the vast changes it made to the vendor directory; I had expected it to import and use the versions in the vendor.json.

Edit:
dep from today should work fine with this as the PR was merged.
go get -u github.com/golang/dep

Manual Testing Instructions:

  • Review the changes made to vendor directory
  • Review Gopkg.toml
  • Review test results
  • Manually do a bit with ddev

My only real worry is that we're accepting something into vendor that we're not ready for, but I think it's probably OK.

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

@rfay
Copy link
Member Author

rfay commented Sep 17, 2017

Wow +1,190,292 −113,281 seems like an enormous change. 1.2M new loc?

@sdboyer
Copy link

sdboyer commented Sep 18, 2017

hi! you might want to try a dep prune - that should eliminate a lot of the excess weight in vendor.

dep's pruning still isn't quite as aggressive as some tools that're out there, but we're working on that: golang/dep#944. (that PR will also make dep prune go away, and just happen always as part of dep ensure)

@rfay
Copy link
Member Author

rfay commented Sep 18, 2017

Wow, that is an enormous difference @sdboyer, thanks: +209,226 −131,906 - We've long struggled with vendor bloat in this repo, we weren't eager to see it coming in fast.

@rfay
Copy link
Member Author

rfay commented Sep 18, 2017

golang/dep#1079 was merged yesterday, so we actually can use dep on this repo now. There's not yet a release that includes it, but I imagine that will be coming before long. But at this exact moment a go get -u github.com/golang/dep/cmd/dep should result in a usable dep.

I guess I'll remove "experimental" from the PR title and see where we go with this.

@rfay rfay changed the title Experimental use of dep to manage dependencies Use dep to manage dependencies Sep 18, 2017
@tannerjfco
Copy link
Contributor

I think we may still need to update the import references for sirupsen/logrus in ddev if we want to be able to borrow packages from it in other projects. I received the following error when attempting to import ddev's util package in another project:

dep ensure -add github.com/drud/ddev/pkg/util@^v0.9.1                                                              1 ↵
ensure Solve(): No versions of github.com/drud/ddev met constraints:
	v0.9.1: Could not introduce github.com/drud/ddev@v0.9.1 due to a case-only variation: it depends on "github.com/Sirupsen/logrus", but "github.com/sirupsen/logrus" was already established as the case variant for that project root by depender (root)

Granted, that's trying to import a tagged version, which is not using dep and would not have any changes present in this PR. However, based on the message provided, I don't think that would make a difference in this case.

@rfay
Copy link
Member Author

rfay commented Sep 18, 2017

After Tanner's 8d7913f we're at +241,817 −133,223

@tannerjfco
Copy link
Contributor

Should probably have review come from @beeradb at this point, since I have added some work to this PR.

@tannerjfco tannerjfco removed their request for review September 18, 2017 22:38
@sdboyer
Copy link

sdboyer commented Sep 19, 2017

I think we may still need to update the import references for sirupsen/logrus in ddev

without knowing anything about your specific situation, this is probably worth doing. while the new logic in dep allows any case variation so long as everything in the depgraph agrees on the particular variation, the preferred casing for sirupsen is the lowercase, so converging on that sooner rather than later (unless one of your current dependencies absolutely precludes it) is to your benefit.

@rfay
Copy link
Member Author

rfay commented Sep 19, 2017

Thanks @sdboyer - we definitely wanted to do that and did do that in this PR. Significant progress! Congrats on all the good things happening with dep.

@rfay
Copy link
Member Author

rfay commented Sep 19, 2017

  • the constraint I put for github.com/cheggaaa/pb is way too specific, I imagine it just needs to be 1.x and not 2. (Actually, 2 is a different branch I think, we should probably constrain to the branch 1 comes from, which is master IIRC)
  • go-pantheon should probably be limited to a release.
  • I'd say most of the constraints that currently just limit to "master" are not a good idea. Of course I contradict my own first bullet.

These don't likely make ddev vendor less stable than it was though.

version = "1.0.18"

[[constraint]]
branch = "master"
Copy link
Contributor

@beeradb beeradb Sep 19, 2017

Choose a reason for hiding this comment

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

If I read the constraints documentation on Gopkg.toml correctly, I believe our hierarchy for defining package constraints should be the following:

  • Version: Always, if a suitable one is available.
  • Revision: if a suitable version is not available
  • Branch: never.

If I understand the behavior of dep correctly, if we rebuild these dependencies we will have unintended code changes if we are pinned just to a branch here. We definitely don't want that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this dep should change unless we were to run an dep ensure -update. Provided the hash in the lock file is valid, I believe we should still get the same version checked out unless the hash is not valid for some reason.

That said, I'm not sure we gain anything by defining a constraint for these packages if we're defining master as the constraint. Perhaps we should remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, yes. I see that now. According to the updating dependencies section of the readme even running dep ensure -update is not recommended, as you would instead run it on an individual package, such as dep ensure -update github.com/drud/go-pantheon.

With that in mind, I'm probably fine with the branches. I'd still prefer versions when available, but it doesn't seem like this will cause the instability I was concerned about.

Copy link

Choose a reason for hiding this comment

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

i approve this line of thinking 😄

Copy link
Contributor

@beeradb beeradb left a comment

Choose a reason for hiding this comment

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

We're not doing anything here that can't be undone, and I think we'd benefit from the experience with dep.

I think dep is pretty clearly the way forward in the golang community, I think it's about time we got on board :)

@rfay rfay merged commit a0ee2e9 into ddev:master Sep 19, 2017
@rfay rfay deleted the 20170916_try_dep branch September 19, 2017 17:32
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.

None yet

4 participants