-
Notifications
You must be signed in to change notification settings - Fork 85
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
chore(deps): update go to 1.22 #2760
Conversation
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.
Thanks ❤️ Haven't finished checking all yet, just left a quick comment for now.
TIL that Go simply downloads a new toolchain (1.22) if not available. I don't really like that, the Go dep, like the vendored deps, should be fixed and local. Seem like The CI here will then fail, but I'll upload a new image with the updated Go version as discussed. |
Yeah, I'll integrate it, you can then review it. |
Thanks.
This will also not be needed then anymore. The AppVeyor comment would be nice to have in the commit message body. |
OK, all error msg are now |
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.
Please split the GOTOOLCHAIN=local
changes into a separate commit (or even PR), as it is independent of the Go 1.22 change.
@@ -33,7 +33,7 @@ envinit: | |||
# Initializiation on MacOS |
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.
It seems we have a problem.
Above on line 31 there is:
# The gomobile/gobind libs are also needed when using `gomobile bind`, but it's not compatible with vendoring:
# https://github.com/golang/go/issues/50994#issuecomment-1032754206
GO111MODULE=off go get -u golang.org/x/mobile/cmd/gomobile
This fails in Go 1.22 because go get
is now disabled outside of modules. I locally built an image simply removing this line, but then cd backend/mobileserver; make
fails with
unable to import bind: no Go package in golang.org/x/mobile/bind
"golang.org/x/mobile/bind" is not found; run go get golang.org/x/mobile/bind: no Go package in golang.org/x/mobile/bind
Running go get golang.org/x/mobile/bind
did not actually fix it for me.
Any idea how to resolve this?
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 also tried adding a dummy import _ "golang.org/x/mobile/bind"
to force it to be vendored, but go mod vendor
only vendors files in Go folders, but bind
has files like these that are then missing:
gomobile: /opt/go/bin/gobind -lang=go,java -outdir=/tmp/gomobile-work-2993124781 github.com/BitBoxSwiss/bitbox-wallet-app/backend/mobileserver failed: exit status 1
failed to open Java support file: open /opt/go/src/github.com/BitBoxSwiss/bitbox-wallet-app/vendor/golang.org/x/mobile/bind/java/Seq.java: no such file or directory
EDIT: according to golang/go#43736 (comment) this should have been fixed 🤔
EDIT2: With the _ "golang.org/x/mobile/cmd/gobind"
import path the Java files are vendored, but the generated code is not modules aware it seems:
PWD=$WORK/src-android-386 GOMODCACHE=$GOPATH/pkg/mod GOOS=android GOARCH=386 CC=$ANDROID_HOME/ndk/21.2.6472646/toolchains/llvm/prebuilt/linux-x86_64/bin/i686-linux-android16-clang CXX=$ANDROID_HOME/ndk/21.2.6472646/toolchains/llvm/prebuilt/linux-x86_64/bin/i686-linux-android16-clang++ CGO_ENABLED=1 GOPATH=$WORK:$GOPATH go mod tidy
rm -r -f "$WORK"
gomobile: go mod tidy failed: exit status 1
go: error reading go.mod: missing module declaration. To specify the module path:
go mod edit -module=example.com/mod
I filed an issue here: golang/go#67927
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.
A workaround that seems to work is to mv vendor vendor.disabled
before make android
and move it back after 🙀
bb4737b
to
604bc9b
Compare
Could You provide a |
Thanks! I wouldn't push an image until we know if/how it will work, as we may need to push other image updates until then. You can simply build it locally though using |
I've solved golang/go#67927 |
This is amazing work, thanks @baizon. What exactly didn't work? gomobile is also installed here: Lines 28 to 32 in c221fca
So maybe you'd need to change that path instead? You may also need to do this to vendor the related packages? golang/go#67927 (comment) |
Sorry, wasn't very specific. My steps:
|
@baizon try also replacing the module name in your fork at https://github.com/baizon/mobile/blob/1d3b1e5c533f3e2b92f8cdde1921d96b618eb361/go.mod#L1 |
That was a nice challenge. Thanks for the support.
Next the 23 (go1.22) docker image? |
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.
@baizon very nice, a local test building the android app seemed to work.
What's the GOMODCACHE env var needed for?
If you don't mind, later I'll fork your fork to the BitBoxSwiss org, so we can use that instead.
@@ -1,2 +1,3 @@ | |||
export ANDROID_SDK_ROOT := /opt/android-sdk | |||
export ANDROID_NDK_HOME := /opt/android-sdk/ndk/21.2.6472646 | |||
export GOMODCACHE_ROOT := /tmp/gomodcache/pkg/mod |
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.
Why this? Is it required? If so, please add a comment why.
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.
Added a comment.
I've picked the |
@baizon I pushed the :24 image. I noticed that while the android build works, it does not seem to be using the Granted, before we also didn't (couldn't) use the vendor lib, but since you went into the gomobile internals, maybe you have a clue why or how to fix it? Ideally it behaves like the normal build with |
Image :24 is reporting an error:
The reason why
A check is performed which isn't working with the vendor option. With
|
I had a typo in the image name, should be fixed now.
Sorry I am confused, what does this relate to? The original issue to build the Android app with a vendor folder present works with Edit: now only a bunch of 1.22 lint issues remain about the new semantics for loop vars. Maybe we could disable the warning for now, or fix it in a separate commit. |
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.
Somehow appveyor reports
go: ..\..\..\go.mod requires go >= 1.22 (running go 1.21.8; GOTOOLCHAIN=local)
Not sure why, the appveyor file seems ok.
So, while building the android app gomobile is running Indeed, I'll add the lint exceptions. Edit:
I could try to remove |
b728b6d
to
dffc31f
Compare
This is the issue you fixed in your fork of gomobile, right? But even with your fork, the vendor module does not seem to be used when building the android app. I checked by modifying a dep in the vendor module (added some syntax error) but the build finished fine anyway, suggesting that the vendor folder was ignored when building the android app. Any idea how one would fix that? As I mentioned before, it is not a high prio as even before 1.22, the vendor module was not used for the android app, but it would be nice to solve this too if you know how. When building the Windows/macOS/linux apps, the vendor folder is used as expected. |
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.
@baizon could you please rebase and squash? Once we figure out the appveyor problem we can merge. Maybe simply re-running it will do 🤷
Done. |
https://www.appveyor.com/docs/windows-images-software/#golang lists > Go 1.22.1 x64 (C:\go - default in PATH) But the image or docs are buggy, C:\go is a symlink of C:\go121 and Go 1.22 does not seem to exist. We switch to choco to install the desired Go version. The benefit is that choco receives new updates faster too and has more granular version control.
Sorry, much to do at work at the moment. Thank You for the fix. I did merge your PR and did a rebase. |
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.
Thanks a lot for your patience and your effort @baizon!
As requested bumped go 1.22.0.
Notes:
scripts/docker_install.sh
changed tohttps://dl.google.com/go/go1.22.4.linux-amd64.tar.gz
(the latest from https://go.dev/dl/)Go 1.22.1 x64 (C:\go - default in PATH)
, see: https://www.appveyor.com/docs/windows-images-software/