Skip to content
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

blockbook: 0.3.2 -> 0.3.3 #86957

Merged
merged 2 commits into from May 12, 2020
Merged

blockbook: 0.3.2 -> 0.3.3 #86957

merged 2 commits into from May 12, 2020

Conversation

1000101
Copy link
Member

@1000101 1000101 commented May 5, 2020

Motivation for this change

Number behavior in 1.14 has changed: golang/go#37308 and it breaks the current (0.3.2) version of blockbook (prevents it from starting).

Moreover, I've added few parameters during build so the resulting application will have nicer UI (version+commit instead of unknown/unknown). I have left the buildtime unknown as it would render the package non-reproducible and the resulting binary file hash would change, too.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@1000101 1000101 changed the title Use go version to 1.13 as Number behavior in 1.14 has changed blockbook: Use go version to 1.13 as Number behavior in 1.14 has changed May 5, 2020
@1000101 1000101 changed the title blockbook: Use go version to 1.13 as Number behavior in 1.14 has changed blockbook: Use go version 1.13 as Number behavior in 1.14 has changed May 5, 2020
@1000101 1000101 changed the title blockbook: Use go version 1.13 as Number behavior in 1.14 has changed blockbook: Use go version 1.13 instead of 1.14 May 5, 2020
@1000101 1000101 mentioned this pull request May 5, 2020
10 tasks
@c00w
Copy link
Contributor

c00w commented May 6, 2020

I think this should just be patched upstream - I have a temporary patch at https://termbin.com/5t4z, but validation is beyond my willingness, since this project doesn't use go modules, and go test didn't work for me on a normal clone. If you get the upstream project to use go modules, I promise to run the tests on my patch and fix anything that breaks :)

With regards to this breaking on 1.14, I think the solution here is to make sure we run the tests as part of hydra build + make sure there is a test which covers this :D Let me know if you need advice, I haven't dug into this closely, but many packages (including go 1.14) have tests which run inside hydra.

@mmahut
Copy link
Member

mmahut commented May 7, 2020

@c00w doesn't look like it compiles with that patch.

go/src/blockbook/bchain/coins/dcr/decredrpc.go:367:3: cannot use 0 (type untyped int) as type string in field value
go/src/blockbook/bchain/coins/dcr/decredrpc.go:369:44: invalid type assertion: infoChainResult.Result.Version.(int) (non-interface type int32 on left)
go/src/blockbook/bchain/coins/dcr/decredrpc.go:640:3: cannot use block.Result.Version (type interface {}) as type json.Number in field value: need type assertion

@Mic92
Copy link
Member

Mic92 commented May 7, 2020

Upstream seems active. Have you reported this issue?

@1000101
Copy link
Member Author

1000101 commented May 7, 2020

Upstream seems active. Have you reported this issue?

Yes, they've confirmed they're going to fix it before 1.13 runs out of support but there's no ETA. Could take weeks or a few months. Moreover, they're planning on switch to modules soon: https://github.com/trezor/blockbook/tree/gomod (this should be implemented sooner as they've already prepared the branch).

I've even sent the patch to blockbook devs but they said it's more complicated than that (will take more effort on their part).

@c00w
Copy link
Contributor

c00w commented May 8, 2020

@mmahut Yep - I couldn't get the project to build and didn't want to mess with it - let me see if I can just pull the go mod fork and fix it there.

@c00w
Copy link
Contributor

c00w commented May 8, 2020

Go mod fork doesn't build + their build scripts require docker, I'll just hack this out using nix to build :)

@c00w
Copy link
Contributor

c00w commented May 8, 2020

@1000101 - try
c00w@2740f81

should be usable with

https://github.com/c00w/nixpkgs.git c00w
cd c00w
git checkout blockbook
nix-build default.nix -A blockbook

I can confirm it builds on my machine, but not much else.

@Mic92
Copy link
Member

Mic92 commented May 8, 2020

If you apply the patch, please also send it upstream.

@1000101
Copy link
Member Author

1000101 commented May 8, 2020

If you apply the patch, please also send it upstream.

Unfortunately, it doesn't work (application crashes on startup).

The question is, do we really need to patch it at all? Upstream will fix this in a few weeks and then we can drop go1.13 for good, if we hate it that much :). But there's no point in having a broken package in nixpkgs.

Regarding module development (which I want to partake) - of course I coul work on the module definition in my own repo where 1.13 still exists, but that's not the main issue.

@1000101 1000101 changed the title blockbook: Use go version 1.13 instead of 1.14 blockbook: 0.3.2 -> 0.3.3 May 11, 2020
@1000101
Copy link
Member Author

1000101 commented May 11, 2020

Upstream has released a new version with 1.14 support along with migration to modules.

@1000101
Copy link
Member Author

1000101 commented May 11, 2020

@mmahut ready for review

@1000101 1000101 requested a review from mmahut May 11, 2020 21:16
@mmahut
Copy link
Member

mmahut commented May 12, 2020

@GrahamcOfBorg build blockbook

@1000101
Copy link
Member Author

1000101 commented May 12, 2020

I've removed aarch64-linux from platforms since to run blockbook properly, you need at least 32GB RAM, so it's not really feasible on most ARM systems anyway.

@Mic92
Copy link
Member

Mic92 commented May 12, 2020

Don't say that :)

$ uname -a && free -m
Linux aarch64.nixos.community 4.19.109 #1-NixOS SMP Wed Mar 11 13:15:13 UTC 2020 aarch64 GNU/Linux
              total        used        free      shared  buff/cache   available
Mem:         128663       76490       28229         509       23943       50854
Swap:             0           0           0

@Mic92
Copy link
Member

Mic92 commented May 12, 2020

However it is fine from my side to remove aarch64 since it fails to link against rocksdb.

@1000101
Copy link
Member Author

1000101 commented May 12, 2020

Don't say that :)

$ uname -a && free -m
Linux aarch64.nixos.community 4.19.109 #1-NixOS SMP Wed Mar 11 13:15:13 UTC 2020 aarch64 GNU/Linux
              total        used        free      shared  buff/cache   available
Mem:         128663       76490       28229         509       23943       50854
Swap:             0           0           0

Wow, I had to edit my previous comment! But the thing is, I've seen many people troubleshooting blockbook on rpi. And obviously it always crashes on rocksdb due to insufficient RAM.

However it is fine from my side to remove aarch64 since it fails to link against rocksdb.

I've reported it to upstream but since aarch64 is not really supported, I'm afraid it won't get fixed anytime soon.

@Mic92
Copy link
Member

Mic92 commented May 12, 2020

Result of nixpkgs-review pr 86957 1

1 package built:
- blockbook

@Mic92 Mic92 merged commit 1e69c8c into NixOS:master May 12, 2020
@1000101 1000101 deleted the blockbook branch May 12, 2020 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants