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 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 force-pushed the 1000101:blockbook branch 3 times, most recently from a52535e to b7cf44e 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
1 of 10 tasks complete
@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
Contributor

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
Contributor

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 force-pushed the 1000101:blockbook branch from b7cf44e to 9c0d109 May 11, 2020
@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 1000101 force-pushed the 1000101:blockbook branch from 9c0d109 to 4600c55 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 1000101 force-pushed the 1000101:blockbook branch from 4600c55 to cdb4c08 May 11, 2020
@1000101
Copy link
Member Author

1000101 commented May 11, 2020

@mmahut ready for review

@1000101 1000101 requested a review from mmahut May 11, 2020
@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.

@ofborg ofborg bot added the 8.has: clean-up label May 12, 2020
@Mic92
Copy link
Contributor

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
Contributor

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
Contributor

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
17 checks passed
17 checks passed
blockbook, blockbook.passthru.tests on aarch64-linux No attempt
Details
blockbook, blockbook.passthru.tests on x86_64-darwin Unexpected error: command failed with exit code 1
Details
Evaluation Performance Report Evaluator Performance Report
Details
blockbook, blockbook.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00b9043"; rev="00b904305c0f79fdfa8affa44e9acc5c401669f0"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@1000101 1000101 deleted the 1000101:blockbook branch May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.