multi: bump Go to 1.24.6 #10167
multi: bump Go to 1.24.6 #10167
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 @starius, 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 performs a comprehensive update of the Go programming language version used across the project from 1.23.12 to 1.24.6. The changes ensure that all development, build, and deployment environments, including Docker images, Go modules, and documentation, are aligned with the new Go version. This update aims to leverage the latest features, performance improvements, and security patches provided by Go 1.24.6, contributing to a more robust and up-to-date codebase.
Highlights
- Go Version Update: The core change involves updating the Go version across all relevant project files, including
go.modfiles in the main project and sub-modules, to Go 1.24.6. - Build Environment Alignment: All Dockerfiles and build scripts that specify a Go version have been updated to use the new 1.24.6 image, ensuring consistency in the build environment.
- Documentation Updates: The installation documentation (
docs/INSTALL.md) has been revised to reflect the new minimum Go version and includes updated download links and checksums for Go 1.24.6 binaries. - Configuration Consistency: Configuration files such as
.golangci.ymlandMakefilehave been adjusted to reference Go 1.24.6, maintaining compatibility with linting and build processes.
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. ↩
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
starius
force-pushed
the
go124
branch
2 times, most recently
from
fcb4f43 to
72447d3
Compare
| t.Parallel() | ||
|
|
||
| ctx := context.Background() | ||
| ctx := t.Context() |
There was a problem hiding this comment.
Oooh, this is nice, wasn't even aware of this change. Need to re-read the release notes, probably many other goodies in there.
There was a problem hiding this comment.
Another cool thing is https://pkg.go.dev/testing/synctest
It was introduced in 1.24 as an experiment and matured in 1.25 but with a slightly different API. We could use it instead of lnd/clock package.
macaroons/constraints.go
Outdated
| if !ok { | ||
| return fmt.Errorf("unable to get peer info from context") | ||
| return fmt.Errorf( | ||
| "unable to get peer info from context", |
There was a problem hiding this comment.
nit: Errorf has a formatting exception rule that allows this to be formatted a bit more compact:
https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md#exception-for-log-and-error-message-formatting
There was a problem hiding this comment.
Fixed:
diff --git a/macaroons/constraints.go b/macaroons/constraints.go
index 231fd1a46..5716e9eb3 100644
--- a/macaroons/constraints.go
+++ b/macaroons/constraints.go
@@ -136,7 +136,8 @@ func IPLockChecker() (string, checkers.Func) {
// check.
pr, ok := peer.FromContext(ctx)
if !ok {
- return fmt.Errorf("unable to get peer info from context")
+ return fmt.Errorf("unable to get peer info from " +
+ "context")
}
peerAddr, _, err := net.SplitHostPort(pr.Addr.String())
if err != nil {
@@ -144,8 +145,8 @@ func IPLockChecker() (string, checkers.Func) {
}
if !net.ParseIP(arg).Equal(net.ParseIP(peerAddr)) {
- msg := "macaroon locked to different IP address"
- return fmt.Errorf(msg)
+ return fmt.Errorf("macaroon locked to different IP " +
+ "address")
}
return nil
}
If CGO is enabled, Go tries to use the gold linker (-fuse-ld=gold), which is not installed. CGO was disabled, because it is not needed for golangci-lint.
starius
force-pushed
the
go124
branch
3 times, most recently
from
747f80c to
24eb45a
Compare
|
Pushed one more commit: "multi: golang.org/x/net/context -> context" |
|
I rolled back |
|
Forgot to mention that I added commit "lntest: fix error check in shutdownAllNodes" which fixes another issue found in the process. |
fn/io_test.go
Outdated
| // stored on new file with the original one. | ||
| func TestWriteFile(t *testing.T) { | ||
| // Root can bypass read-only file mode. | ||
| if syscall.Geteuid() == 0 { |
There was a problem hiding this comment.
Not sure I follow this change, can you explain more? Do you mean we should only run these tests when we are not in root? I think the method being tested should be agnostic about the sys role?
There was a problem hiding this comment.
A regular user can't overwrite their file if it read-only, but root user can overwrite his files even in this case.
$ echo 111 > test.txt
$ chmod -w test.txt
$ ls -l test.txt
-r--r--r-- 1 user user 4 Aug 25 23:52 test.txt
$ echo 222 > test.txt
-bash: test.txt: Permission denied
$ sudo su
# echo 111 > test.txt2
# chmod -w test.txt2
# ls -l test.txt2
-r--r--r-- 1 root root 4 Aug 25 23:55 test.txt2
# echo 222 > test.txt2
# cat test.txt2
222
That is why the test used to fail when run by root with an error hard to understand. This is how I actually discovered this special rule of root. I added t.Skip to prevent surprising test runners in the future if they happen to run tests under root.
There was a problem hiding this comment.
I think this is reverse causation? The test should reflect the expected behavior of the method, not creating exceptions just to let it pass. For instance, does it mean the WriteFile function will behave differently when we are under root? If so, should we fix that instead? or at least document it somewhere?
There was a problem hiding this comment.
that is really good point @yyforyongyu, what about removing the permission handling from this test entirely, I mean the function should test writing not permission handling, this way we avoid the case of running this case with the root user ?
There was a problem hiding this comment.
I removed commit "fn: skip tests when running as root". The commit is not required to bump Go.
Let's solve the issue of that test failing when run by root user in a separate PR.
| && cd /tmp/tools \ | ||
| && go install -trimpath github.com/golangci/golangci-lint/cmd/golangci-lint \ | ||
| && golangci-lint custom \ | ||
| && CGO_ENABLED=0 go install -trimpath github.com/golangci/golangci-lint/cmd/golangci-lint \ |
There was a problem hiding this comment.
interesting - so i guess this speeds things up a bit?
There was a problem hiding this comment.
I haven't measured it, but in general disabling CGO makes building faster. I disabled CGO to fix the build on Arm64. An alternative was to install ld-gold package, which doesn't make sense given the Go package doesn't even need CGO.
non-constant format string in call to (*net/textproto.Conn).Cmd
It resulted in interpreting the error message as a format string. Use Fatal(err) instead.
It resulted in interpreting the error message as a format string.
Use failf("%v", err) instead.
Fixed multiple cases in which a non-constact string variable was used as a format string for fmt.Errorf.
Use the new feature of Go 1.24, fix linter warnings. This change was produced by: - running golangci-lint run --fix - sed 's/context.Background/t.Context/' -i `git grep -l context.Background | grep test.go` - manually fixing broken tests - itest, lntest: use ht.Context() where ht or hn is available - in HarnessNode.Stop() we keep using context.Background(), because it is called from a cleanup handler in which t.Context() is canceled already.
Previously the test failed only if the last node failed to shutdown. The code was updated to fail if any node failed to shutdown.
Change Description
Updated Go version everywhere to 1.24.6.
Also fixed a couple of minor issues found when testing locally:
golangci-lintin Docker on Arm64 (disabled CGO for that)fntest which fails when running as rootSteps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.