build: bump go to v1.24.6 #1144
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @GeorgeTsagk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily updates the Go programming language version used in the project to 1.24.6. This update necessitated adjustments to the linting configuration to accommodate a new govet rule, ensuring continued compatibility and smooth operation of the build and test processes.
Highlights
- Go Version Bump: The Go programming language version has been updated from 1.23.12 to 1.24.6 across various project configurations and module files.
- Govet printf Rule Handling: A new govet rule in Go 1.24.6, which flags non-constant format strings in printf calls, has been addressed by disabling this specific check in .golangci.yml and by disabling govet during test runs.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request bumps the Go version to 1.24.6 across the project. This is a necessary update but introduces a new govet check for printf usage that causes issues. The PR addresses this by disabling the printf check in .golangci.yml and disabling govet entirely during tests. While disabling the check for the linter is a reasonable workaround, disabling all vet checks during tests is a significant regression. I've provided a suggestion for a more targeted fix that keeps other vet checks running.
make/testing_flags.mk
Outdated
| @@ -1,6 +1,6 @@ | |||
| include make/compile_flags.mk | |||
|
|
|||
| TEST_FLAGS = | |||
| TEST_FLAGS = -vet=off | |||
There was a problem hiding this comment.
Disabling go vet completely during tests with -vet=off is a significant step back for code quality checks. While the new printf check in Go 1.24 is causing issues, turning off all vet checks means we might miss other potential bugs.
A more targeted approach would be to run go vet separately with the printf check disabled, and then run the tests with vet turned off. This way, we still get the benefit of all other vet checks.
I suggest creating a new target in the main Makefile for the vet check and making unit and unit-debug depend on it. This would keep the vet checks running for all test-related targets.
Here's a suggested change for the main Makefile:
# In Makefile
...
vet-check:
@$(call print, "Running go vet.")
go vet -printf=false ./...
unit: vet-check
@$(call print, "Running unit tests.")
mkdir -p app/build && touch app/build/index.html
$(UNIT)
unit-debug: vet-check
@$(call print, "Running unit tests.")
mkdir -p app/build && touch app/build/index.html
$(UNIT_DEBUG)
...With this change, you can keep TEST_FLAGS = -vet=off in this file, but you'll still be running all other vet checks before the tests.
There was a problem hiding this comment.
This was my main concern with the current way of disabling govet for make unit, interested to see what people think too
|
Oh itests are also hitting the |
GeorgeTsagk
force-pushed
the
bump-go-1.24.6
branch
from
cd85d72 to
ec9a0e7
Compare
GeorgeTsagk
added
the
no-changelog
Had to also disable |
There was a problem hiding this comment.
Thanks for the update @GeorgeTsagk 🙏!
I'm leaving an alternative suggestion of how you could solve the linter issue, that I think would be a better approach.
.golangci.yml
Outdated
| disable: | ||
| - printf |
There was a problem hiding this comment.
Instead of having to add this exception, I'd suggest that it'd probably be better to see if you can solve the underlaying issue the linter is erroring on. After testing, it seems to me like the linter is only erroring if we call a function like:
func test(errStr string, params ...interface{}){
// Something that does:
fmt.Sprintf(errStr, params...)
}
Where the call site doesn't actually set any value for params. Call sites that do set a value for params seems to not error. So I suggest updating such cases in litd, instead of updating the linter exception here.
For example, when I tried doing so for the status Manager, this update seems to work:
// SetErrored can be used to set the status of a sub-server as not Running
// and also to set an error message for the sub-server.
//
// NOTE: This will silently fail if the referenced sub-server has not yet been
// registered.
func (s *Manager) SetErrored(name string, errStr string) {
s.mu.Lock()
defer s.mu.Unlock()
log.Debugf("Setting the %s sub-server as errored: %s", name, errStr)
ss, ok := s.subServers[name]
if !ok {
return
}
log.Errorf("could not start the %s sub-server: %s", name, errStr)
ss.running = false
ss.err = errStr
ss.customStatus = ""
}
func (s *Manager) SetErroredf(name string, errStr string,
params ...interface{}) {
errStr = fmt.Sprintf(errStr, params...)
s.SetErrored(name, errStr)
}
NOTE: only example code above, and haven't considered if there's better ways to do so than the above :).
There was a problem hiding this comment.
Another alternative that seems to work is to simply pass nil as params ...interface{} when no extra params are present, so on the callsites that would be
s.statusServer.SetErrored(ss.Name(), err.Error(), nil)
I do like it as it's a bit more lean (-- we don't create an extra helper func)
I'll push that version and see how CI behaves -- will also strip away all the custom govet related rule exceptions
GeorgeTsagk
force-pushed
the
bump-go-1.24.6
branch
from
ec9a0e7 to
d096abb
Compare
There was a problem hiding this comment.
Awesome that you managed to get rid of the linter rule exception 🚀🎉!
Regarding #1144 (comment), I personally prefer creating helper functions as suggested in #1144 (comment) over having to pass nil to the functions.
My motivation for why that's preferred is that it'll otherwise be very hard for callers to understand that they need to pass nil to that params ...X arg, or they'll end up getting linter errors. If we stick to the nil passing alternative though, I think you should at least update the docs for those functions and add a comment which explains that you need to set nil for the params ...X arg if no params exists, and explain why that's needed.
I don't have a too strong preference for my suggestion though, so I'd be ok with your approach if others agree that it's better :)! So I'd be interested to hear what @ellemouton thinks about this.
rpc_proxy.go
Outdated
| @@ -365,7 +365,7 @@ func (p *rpcProxy) makeDirector(allowLitRPC bool) func(ctx context.Context, | |||
| handled, conn, err := p.subServerMgr.GetRemoteConn(requestURI) | |||
| if err != nil { | |||
| return outCtx, nil, status.Errorf( | |||
| codes.Unavailable, err.Error(), | |||
| codes.Unavailable, err.Error(), nil, | |||
There was a problem hiding this comment.
nit: There's already a status.Error function available that won't require the passing of nil here, so I would suggest using that instead.
There was a problem hiding this comment.
Yeap followed this approach instead of passing in nil to all call sites.
I added the helper you recommended above.
There's only 1 call site where I pass in nil to RPCErrString. This was only a single instance, so figured it wasn't really worth splitting RPCErrString into two helpers.
GeorgeTsagk
force-pushed
the
bump-go-1.24.6
branch
from
d096abb to
3d89cc5
Compare
There was a problem hiding this comment.
lgtm!
thanks for the update, and thanks to @ViktorTigerstrom for the helper suggestion 🙏
There was a problem hiding this comment.
Awesome, thanks a lot for the updates and the PR @GeorgeTsagk! LGTM 🎉🔥!!
I realized that there's a potentially cleaner way you could solve the linting issue if you'd prefer to avoid adding the helper function. Leaving that suggestion below.
I'm totally ok with either version, so leaving it up to you which version you prefer :)! So I'd be ok with merging the PR as is if you'd prefer 🔥.
status/manager.go
Outdated
| // | ||
| // NOTE: This will silently fail if the referenced sub-server has not yet been | ||
| // registered. | ||
| func (s *Manager) SetErroredf(name string, errStr string, |
There was a problem hiding this comment.
Hmmm I realized that one alternative approach that you may like better than the helper function alterantive would be to add an if case in the SetErrored function like this:
func (s *Manager) SetErrored(name string, errStr string,
params ...interface{}) {
s.mu.Lock()
defer s.mu.Unlock()
log.Debugf("Setting the %s sub-server as errored: %s", name, errStr)
ss, ok := s.subServers[name]
if !ok {
return
}
log.Errorf("could not start the %s sub-server: %s", name, errStr)
if len(params) > 0 {
errStr = fmt.Sprintf(errStr, params...)
}
ss.running = false
ss.err = errStr
ss.customStatus = ""
}
And something similar in the RPCErrString like this:
func RPCErrString(req *lnrpc.RPCMiddlewareRequest, format string,
args ...interface{}) (*lnrpc.RPCMiddlewareResponse, error) {
feedback := &lnrpc.InterceptFeedback{}
resp := &lnrpc.RPCMiddlewareResponse{
RefMsgId: req.MsgId,
MiddlewareMessage: &lnrpc.RPCMiddlewareResponse_Feedback{
Feedback: feedback,
},
}
if len(args) > 0 {
format = fmt.Sprintf(format, args...)
}
if format != "" {
feedback.Error = format
}
return resp, nil
}
I'm totally ok with either version though, so feel free to keep the PR and merge it as if you'd like 😃 🎉!
There was a problem hiding this comment.
Ah, much better approach! Will actually apply it. Thanks again 💯
GeorgeTsagk
force-pushed
the
bump-go-1.24.6
branch
from
3d89cc5 to
9f3a85a
Compare
In preparation for the next commit which bumps golang to a newer version, we want to make some code changes that would otherwise render some log-related calls problematic. With go1.24 a new govet rule was added that disallows non-constant strings (i.e including a tag like "%s") in calls to printf. See more in the related issue golang/go#60529.
GeorgeTsagk
force-pushed
the
bump-go-1.24.6
branch
from
9f3a85a to
69ca658
Compare
Description
Bumps the version of golang to v1.24.6
Notes
With this bump we also include a new
govetrule that disallows printing non-constant strings (containing%sfor example), giving many errors like the following:For this we add a rule to
.golangci.ymlto exclude theprintfcheck fromgovetwhen linting, and also we disablegovetwhen running tests -- could not find a workaround that directly targets theprintfrule.The latter is important as we use
make unitfor the "check commits" CI step.Rationale
The motivation behind this bump is the following PR: #1138
which bumps a lot of dependencies that indirectly bump go to v1.24.6 (the above PR currently includes a commit that bumps golang for LiT, but decided to split it off into its own PR)