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

build: add module support for ticker and queue packages #2602

Merged
merged 6 commits into from Feb 16, 2019

Conversation

Projects
None yet
4 participants
@wpaulino
Copy link
Collaborator

wpaulino commented Feb 7, 2019

In this PR, we begin our efforts towards enabling small utility packages that may be useful outside of lnd to be used as dependencies within other projects without needing to import all of lnd. For now, this is only done for the ticker and queue packages.

For future reference: https://github.com/go-modules-by-example/index/tree/master/009_submodules

Show resolved Hide resolved ticker/ticker.go
go.mod Outdated
@@ -49,3 +51,7 @@ require (
gopkg.in/macaroon.v2 v2.0.0
gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce // indirect
)

replace github.com/lightningnetwork/lnd/ticker v1.0.0 => ./ticker

This comment has been minimized.

Copy link
@halseth

halseth Feb 7, 2019

Collaborator

is this a standard way of using submodules?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Feb 7, 2019

Author Collaborator

I'll use an example to hopefully get the point across better:

Let's say we wish to rename the Ticker interface to something else. Without this replace directive, we would have to commit the changes to the ticker package, tag them, and update the version used within the project's root go.mod in order to use those changes elsewhere in the project. With the directive in place, we can avoid doing so.

I guess it's all a matter of preference. We can remove it and everything will still work fine, the extra hurdle will exist if we decide to not include the replace directive.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 12, 2019

Member

So then this just instructs the build to use whatever is checked into the repo rather than the tagged version? If that's so, then why do we need to add the queue or ticker packages to the go.mod of the root project?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Feb 12, 2019

Author Collaborator

Because the root project still depends on them. Also, a replace directive for a package can't exist without a require directive for the same package.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Feb 7, 2019

Hm, this also triggers a double Travis build? 😛

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

wpaulino commented Feb 7, 2019

Hm, this also triggers a double Travis build?

The branch was pushed as part of the repo rather than my own fork to avoid further replace directives to my fork, so there was a build for that and another for the PR.

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🐍

@@ -0,0 +1 @@
module github.com/lightningnetwork/lnd/ticker

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 12, 2019

Member

No go.sum needed here? Perhaps since it only imports items in the stdlib?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Feb 12, 2019

Author Collaborator

Yep, no external deps.

go.mod Outdated
@@ -49,3 +51,7 @@ require (
gopkg.in/macaroon.v2 v2.0.0
gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce // indirect
)

replace github.com/lightningnetwork/lnd/ticker v1.0.0 => ./ticker

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 12, 2019

Member

So then this just instructs the build to use whatever is checked into the repo rather than the tagged version? If that's so, then why do we need to add the queue or ticker packages to the go.mod of the root project?

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Feb 12, 2019

Yeah double travis build is only since it was pushed as a direct branch of lnd in order to be able to do the tag on his end.

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

wpaulino commented Feb 12, 2019

There are some changes that'll need to be made to the queue package for the buffer pools, so this should wait until that lands.

wpaulino added some commits Feb 7, 2019

ticker/ticker: rename ticker -> T
In this commit, we rename the previously unexported ticker into T,
making it exported along the way. This is nice as we now have access to
the actual interface implementation without the need of making further
type assertions.

@wpaulino wpaulino force-pushed the ticker-queue-modules branch 2 times, most recently from 422b527 to 9fc20f3 Feb 13, 2019

@wpaulino wpaulino force-pushed the ticker-queue-modules branch from 9fc20f3 to 33b6e7c Feb 13, 2019

@cfromknecht cfromknecht referenced this pull request Feb 15, 2019

Merged

brontide: read buffer pool #2419

1 of 1 task complete

@Roasbeef Roasbeef merged commit 44b8cd6 into master Feb 16, 2019

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 59.6%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wpaulino wpaulino deleted the ticker-queue-modules branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.