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

frdrpc: convert frdrpc into a go module #194

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bhandras
Copy link
Member

This commit transforms the frdrpc subpackage into a versioned module. By doing so, projects that rely solely on the generated RPC code from the proto files will benefit from significantly reduced dependencies during compilation.

Pull Request Checklist

  • Update MinLndVersion if your PR uses new RPC methods or fields of lnd.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A couple of minor things, otherwise looks good.

frdrpc/go.mod Outdated
@@ -0,0 +1,17 @@
module github.com/lightninglabs/faraday/frdrpc

go 1.22.3
Copy link
Member

Choose a reason for hiding this comment

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

nit: when we're touching or creating all these go.mod files, can we please also sync the place we put the version? Either all at the top or the bottom of the file? Instinctively I'd say probably at the bottom...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Done.

github.com/lightninglabs/lndclient v0.17.4-1
github.com/lightningnetwork/lnd v0.17.4-beta
github.com/lightningnetwork/lnd/cert v1.2.2
github.com/lightningnetwork/lnd/kvdb v1.4.4
github.com/shopspring/decimal v1.2.0
github.com/stretchr/testify v1.8.4
github.com/urfave/cli v1.22.9
google.golang.org/grpc v1.59.0
google.golang.org/protobuf v1.31.0
google.golang.org/grpc v1.65.0
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure if this update might bite us, since in lnd we have an older version. Or is the plan to update that as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't think it causes any issues given Terminal compiles and runs with the frdrpc and poolrpc added as deps: lightninglabs/lightning-terminal#823 If you look closer it updates GRPC as well.

tools/Dockerfile Outdated
&& chmod -R 777 /tmp/build/
&& go install -trimpath github.com/golangci/golangci-lint/cmd/golangci-lint \
&& chmod -R 777 /tmp/build/ \
&& git config --global --add safe.directory /build
Copy link
Member

Choose a reason for hiding this comment

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

I think the ENV GOFLAGS="-buildvcs=false" is the actual, preferred fix for the issue? Or did that no longer work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think I was just simply adopting the Dockerfile that we use in other lightninglabs projects. Modified to use the suggested flag instead.

tools/go.mod Outdated
@@ -1,9 +1,197 @@
module github.com/lightninglabs/faraday/tools

go 1.16
go 1.21
Copy link
Member

Choose a reason for hiding this comment

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

We might as well bump this version too...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@lightninglabs-deploy
Copy link

@bhandras, remember to re-request review from reviewers when ready

Copy link
Member Author

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Thanks for the review @guggero! Attempted to address all your comments, PTAL 🙏

github.com/lightninglabs/lndclient v0.17.4-1
github.com/lightningnetwork/lnd v0.17.4-beta
github.com/lightningnetwork/lnd/cert v1.2.2
github.com/lightningnetwork/lnd/kvdb v1.4.4
github.com/shopspring/decimal v1.2.0
github.com/stretchr/testify v1.8.4
github.com/urfave/cli v1.22.9
google.golang.org/grpc v1.59.0
google.golang.org/protobuf v1.31.0
google.golang.org/grpc v1.65.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't think it causes any issues given Terminal compiles and runs with the frdrpc and poolrpc added as deps: lightninglabs/lightning-terminal#823 If you look closer it updates GRPC as well.

frdrpc/go.mod Outdated
@@ -0,0 +1,17 @@
module github.com/lightninglabs/faraday/frdrpc

go 1.22.3
Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Done.

tools/Dockerfile Outdated
&& chmod -R 777 /tmp/build/
&& go install -trimpath github.com/golangci/golangci-lint/cmd/golangci-lint \
&& chmod -R 777 /tmp/build/ \
&& git config --global --add safe.directory /build
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think I was just simply adopting the Dockerfile that we use in other lightninglabs projects. Modified to use the suggested flag instead.

tools/go.mod Outdated
@@ -1,9 +1,197 @@
module github.com/lightninglabs/faraday/tools

go 1.16
go 1.21
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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.

3 participants