Skip to content

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
Contributor

@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
Contributor

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.

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
Contributor

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
module github.com/lightninglabs/faraday/tools

go 1.16
go 1.21
Copy link
Contributor

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.

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 🙏

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
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.

@bhandras bhandras requested a review from guggero September 26, 2024 19:27
@lightninglabs-deploy
Copy link

@guggero: review reminder

Copy link
Contributor

@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.

LGTM 🎉

@guggero guggero merged commit e1145fd into lightninglabs:master Oct 14, 2024
5 checks passed
@bhandras bhandras deleted the frdrpc-module branch October 14, 2024 10:45
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