-
Notifications
You must be signed in to change notification settings - Fork 247
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: use go 1.20 and latest go-waku #4522
Conversation
|
Jenkins BuildsClick to see older builds (93)
|
8521a2a
to
827a5b1
Compare
|
1 similar comment
|
@jakubgs , while i was attempting to upgrade to go1.21, i ended up with this error in nix:
According to https://discourse.nixos.org/t/how-to-get-go-1-21-in-nix-shell/33433/3 it seems that the default go version is now 1.21 and there is no go_1_21 package, but seems weird to not specify the go version. Your advice is appreciated! |
@richard-ramos remember that the packages available form Nix are controlled by the version of Lines 2 to 7 in 846a4e2
If you look at the e7603eba51f2c7820c0a182c6bbb351181caa8e7 commit you'll see it doesn't contain Go 1.21 :https://github.com/NixOS/nixpkgs/tree/e7603eba51f2c7820c0a182c6bbb351181caa8e7/pkgs/development/compilers/go Now as the comment states, this should be the same version as used in As for what to upgrade to, I would argue that using |
And actually, @siddarthkay is attempting a Nixpkgs upgrade in here: So you might want to sync with him as to what version to use. |
Just checked 23.11, (the one used in mobile PR) and I see it contains the go version I need. I'll try changing it in this PR and see how it goes. Once status-im/status-mobile#18321 is merged we can proceed to upgrade nix in status-go |
8f44c51
to
a9a2d5b
Compare
Looks like upgrading to go1.21 ends up causing issues in the torrent lib. (cc: @siddarthkay @chaitanyaprem), maybe there's a new version compatible with it.
|
a9a2d5b
to
633e54c
Compare
After changing the version of anacrolix/torrent, now gomobile has a different complain:
I did run into this before, so it seems we need to upgrade gomobile somehow? |
633e54c
to
ada8eb3
Compare
f358aaf
to
df8b955
Compare
To upgrade Lines 27 to 39 in df8b955
For more details see: |
df8b955
to
aec4c80
Compare
99db6fc
to
716c0da
Compare
looks like there's a dependency that also includes sqlite and clashes with sqlcipher. I'm trying to figure out where this is happening cc: @siddarthkay @chaitanyaprem
|
Oh. It's anacrolix/torrent |
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.
💪
appDbPath + "-shm", | ||
appDbPath + "-wal", | ||
appDbPath + shm, | ||
appDbPath + "-wal", // nolint: goconst |
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 see where you got tired of fixing goconst
😄
shell.nix
Outdated
@@ -1,17 +1,17 @@ | |||
{ | |||
/* This should match Nixpkgs commit in status-mobile. */ | |||
source ? builtins.fetchTarball { | |||
url = "https://github.com/NixOS/nixpkgs/archive/e7603eba51f2c7820c0a182c6bbb351181caa8e7.tar.gz"; | |||
sha256 = "sha256:0mwck8jyr74wh1b7g6nac1mxy6a0rkppz8n12andsffybsipz5jw"; | |||
url = "https://github.com/NixOS/nixpkgs/archive/ddf0003c57fb5cbb4a9754f2f6d5ebe9cdae5151.tar.gz"; |
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.
Is this commit going to be used in status-mobile
as well?
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.
Should be! i copied it from status-im/status-mobile#18321 so it probably makes sense to merge both this PR and that one once approved
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.
hmm
we cant use this url -> https://github.com/NixOS/nixpkgs/archive/ddf0003c57fb5cbb4a9754f2f6d5ebe9cdae5151.tar.gz since its from nixpkgs master branch, what I had in the status-mobile PR back then was an experiment :D
Here is what I'll do next week :
- open PR with this status-go version in status-mobile to see what breaks and what doesn't.
- we'll verify if any
nixpkgs
stable version 23.11 got the fixes we want related toGraalVM
or not ( else we just fork and move on )
and then what url we decide on at status-mobile we can use that here.
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.
actually for now you could just use : e7603eba51f2c7820c0a182c6bbb351181caa8e7
since we do have go 1.20 over here -> https://github.com/NixOS/nixpkgs/blob/e7603eba51f2c7820c0a182c6bbb351181caa8e7/pkgs/development/compilers/go/1.20.nix
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.
Alright! will do that
5da3b57
to
f245a83
Compare
|
Hey @richard-ramos : can you create an issue with the error you faced and I can take a look for what we can do to get the torrent package to work with go 1.21 |
f245a83
to
b47b206
Compare
|
b47b206
to
95b9867
Compare
|
95b9867
to
86e2b53
Compare
|
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'm approving the shell.nix
changes.
86e2b53
to
06d04f6
Compare
|
06d04f6
to
d0ca444
Compare
|
See title