-
Notifications
You must be signed in to change notification settings - Fork 726
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 go mod to manage dependency #1269
Conversation
/run-all-tests |
@@ -32,23 +44,29 @@ ci: build check basic-test | |||
|
|||
build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to do build: export GO111MODULE=on
It could be useful to keep using a vendor/ directory with go modules, at least temporarily. |
go.mod
Outdated
google.golang.org/genproto v0.0.0-20180427144745-86e600f69ee4 // indirect | ||
google.golang.org/grpc v1.12.2 | ||
gopkg.in/airbrake/gobrake.v2 v2.0.9 // indirect | ||
gopkg.in/gemnasium/logrus-airbrake-hook.v2 v2.1.2 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did these airbrakes end up getting added? They weren't there before and shouldn't be PD dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From go mod why gopkg.in/gemnasium/logrus-airbrake-hook.v2
we can see it is imported by github.com/sirupsen/logrus.test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did run that, but I didn't know what logrus.test is. It isn't listed in our dependencies.
It seems that go modules adds all transitive testing dependencies.
- https://www.reddit.com/r/golang/comments/98hbvk/go_modules_how_to_deal_with_test_dependencies/
- cmd/go: add 'require test' section to go.mod golang/go#26913
- cmd/go: 'go mod -sync' adds test dependencies of dependencies, while 'go build' does not golang/go#26626
This makes me prefer dep to go modules for now until they get more feedback on go modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the logrus repo and I don't see how it can reference airbrake (testing or not): https://github.com/sirupsen/logrus/search?q=airbrake&unscoped_q=airbrake
This actually seems like an issue with how they defined their go.mod. Perhaps you can ask them about it?
I still don't understand why go mod tidy doesn't remove this since it isn't needed.
GO111MODULE=on go mod why github.com/sirupsen/logrus.test
go: finding github.com/golang/protobuf/proto/testdata latest
go: finding github.com/golang/protobuf/proto latest
# github.com/sirupsen/logrus.test
(main module does not need package github.com/sirupsen/logrus.test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is actually an open issue about testing dependencies here: golang/go#26955. It is milestoned for 1.12 although not necessarily accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks the best effort now is to keep the vendor/
directory, setup -mod=vendor
in Makefile, and only use go mod
when we need to update imports. The update procedure should include ./hack/clean-vendor
to cleanup test files.
I'll take a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 but I don't think clean-vendor will end up removing exteraneous deps like airbrake. And now go mod
is built-in automatically to go commands, so I don't know if it is possible to avoid it updating imports.
Hi @gregwebs Please take another look. I have reset the branch and |
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deps look good now! Looks like this is actually significantly decreasing the amount of code being dragged into vendor now.
I think keeping the vendor directory will help ease the transition go go modules now. In the long-term we may want to remove it, although I am not sure.
Yep, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What problem does this PR solve?
Use go mod to manage dependency
What is changed and how it works?
Changes:
go mod init
to generatego.mod
file from dep configuration.vendor/
and recreate it withmake vendor
Notes:
-mod=vendor
flag to force use the vendored code. Thus we can make sure different go will build use the same code base.GO111MODULE=on go get -u ...
to updatego.mod
thenmake vendor
to vendor source code.Check List
Tests