-
Notifications
You must be signed in to change notification settings - Fork 28
Use Go backports PPA in CI and bump go toolchain to 1.25.0 #1074
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
base: main
Are you sure you want to change the base?
Conversation
2ac8111
to
5dfc53a
Compare
.github/workflows/build-deb.yaml
Outdated
sudo apt-get update -y | ||
sudo apt-get install -y golang-1.25 | ||
add-apt-repository ppa:ubuntu-enterprise-desktop/golang | ||
apt-get install -y golang-1.25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the install line should not be needed though, since we want to be picked automatically when doing apt build-dep no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, see #1074 (comment)
.github/workflows/build-deb.yaml
Outdated
# Install Go 1.25 from our Go backports PPA | ||
add-apt-repository ppa:ubuntu-enterprise-desktop/golang | ||
apt-get install -y golang-1.25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, trying to install it here is too late, because the extra-source-build-script
is run after apt build-dep .
, which fails because golang-1.25
is not available in the APT sources. I'll create a PR in the desktop-engineering repo to add pre-deps-commands
where we can add the PPA before the dependencies are installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good... or even better, I'd just scope to the repository--- So something like the PPAs does: apt-dependencies
including a list of repositories we can add via add-apt-repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to install software-properties-common
before being able to use add-apt-repository
(which is a bit unfortunate because it pulls in a lot of dependencies which we don't really need), so I added that to the pre-deps-commands
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil | ||
} | ||
var n C.int | ||
tmp := C.copy_strv(strv, &n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, that's unclear to me why this would work in a copied strv and not in the original one... 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually just need to get the length of the array, we don't need to copy it. I pushed a fixup commit.
4bf732f
to
e1255f2
Compare
// Return the length of a NULL-terminated char** array. | ||
size_t strv_len(char **strv) { | ||
size_t n = 0; | ||
while (strv && strv[n]) n++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're checking for NULL
value already in the caller (and in general a strv
should never be null), so I'd just use: while (strv[n]) n++;
Ah, also take a const char *strv
or even better const char * const * strv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
85ae717
to
0a85010
Compare
Some of our dependencies now require a newer Go version than 1.23. Go 1.25 also allows us to use the new Output method of testing.T, making our test output easier to read.
With Go 1.25, `go test -race` failed with: fatal error: checkptr: misaligned pointer conversion goroutine 72 gp=0xc0002281c0 m=8 mp=0xc00018a008 [running]: runtime.throw({0x16ece45?, 0x0?}) /home/user/go/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:1094 +0x48 fp=0xc0002f7150 sp=0xc0002f7120 pc=0x489cc8 runtime.checkptrAlignment(0xc0005fec1e, 0x1586820, 0x1) /home/user/go/pkg/mod/golang.org/[email protected]/src/runtime/checkptr.go:20 +0x9a fp=0xc0002f7170 sp=0xc0002f7150 pc=0x41b8da github.com/ubuntu/authd/internal/users/localentries.strvToSlice(0xc0005fec1e)
Some of our dependencies now require a newer Go version than 1.23.
Go 1.25 also allows us to use the new Output method of testing.T, making our test output easier to read.
The Noble archives only have Go 1.23, so we use our own Go PPA to install Go 1.25.
UDENG-8125