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

[build] Add risc-v support to gitian #13665

Merged
merged 2 commits into from Aug 16, 2018

Conversation

Projects
None yet
6 participants
@ken2812221
Copy link
Member

commented Jul 15, 2018

Based on #13660 #13710 , add gitian tarball for RISC-V

@fanquake fanquake requested a review from laanwj Jul 16, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:gitian-risc-v branch Jul 16, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

ACK but only after we can compile with a compiler that doesn't produce a broken test_bitcoin. If inlining is broken, who knows what other bugs are introduced.
(see #13543 (comment) and following comments)
Maybe Ubuntu will provide a newer version of gcc at some point.

@DrahtBot DrahtBot removed the Needs rebase label Jul 17, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:gitian-risc-v branch Jul 18, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

@laanwj How about force it to use gcc-8?

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Yes, that was what I meant. But how?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Just see what I do in the fourth commit.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Didn't know that a package for that was available!
Can someone confirm that the resulting test_bitcoin runs without issues?

Edit: interesting;
I wonder if this is the only way to get the -8 variant to be used.
If there's no update-alternatives incantation to update the symlinks. This would be more convenient for the build guide.

Edit no, apparently the risc-v compiler packages do not provide alternatives

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Concept ACK

@laanwj laanwj requested a review from theuni Jul 19, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:gitian-risc-v branch Jul 19, 2018

@theuni
Copy link
Member

left a comment

Concept ACK, but I don't think it's wise for us to be producing supported binaries for a platform that's still going through teething issues with compilers. Especially since so few people are able to test.

I'm all for it for 0.18, but -1 from me for 0.17.

- "gcc-8-riscv64-linux-gnu"
- "binutils-riscv64-linux-gnu"
- "g++-8-multilib"
- "gcc-8-multilib"

This comment has been minimized.

Copy link
@theuni

theuni Jul 19, 2018

Member

I suspect that this will cause more glibc back-compat issues. That's fine, of course, they just need to be accounted for.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jul 19, 2018

Author Member

I've tested this. make -C src check-symbol is happy about this in gitian build.

depends/packages/qt.mk Outdated
@@ -92,6 +92,8 @@ $(package)_config_opts_linux += -no-opengl
$(package)_config_opts_arm_linux += -platform linux-g++ -xplatform bitcoin-linux-g++
$(package)_config_opts_i686_linux = -xplatform linux-g++-32
$(package)_config_opts_x86_64_linux = -xplatform linux-g++-64
$(package)_config_opts_aarch64_linux = -xplatform linux-aarch64-gnu-g++

This comment has been minimized.

Copy link
@theuni

theuni Jul 19, 2018

Member

This shouldn't be needed after #13696 (comment).

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jul 19, 2018

Author Member

This is not my commit, just based on #13696

@@ -67,6 +67,8 @@ __asm(".symver log2f_old,log2f@GLIBC_2.2.5");
__asm(".symver log2f_old,log2f@GLIBC_2.4");
#elif defined(__aarch64__)
__asm(".symver log2f_old,log2f@GLIBC_2.17");
#elif defined(__riscv)

This comment has been minimized.

Copy link
@theuni

theuni Jul 19, 2018

Member

Is this necessary? We don't have to support much back-compat for riscv, as its glibc support is pretty new anyway.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jul 19, 2018

Author Member

This might not be necessary. But we should drop --enable-glibc-back-compat flag when building for RISC-V

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 19, 2018

Member

I think he did this to appease the symbols check, the alternative would be to do a different symbols check per target platform.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jul 19, 2018

Author Member

I did this so the linker would know what log2f_old is.

@laanwj laanwj added this to the 0.18.0 milestone Jul 20, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

I'm all for it for 0.18, but -1 from me for 0.17.

Added appropriate milestone

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13724 ([contrib] Support ARM and RISC-V symbol check by ken2812221)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ken2812221 ken2812221 changed the title [build] Add risc-v support to gitian [WIP][build] Add risc-v support to gitian Jul 22, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:gitian-risc-v branch Aug 6, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 6, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Aug 7, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

gitian linux failure:

ERROR: Feature 'system-zlib' was enabled, but the pre-condition 'libs.zlib' failed.

ERROR: Feature 'openssl-linked' was enabled, but the pre-condition '!features.securetransport && libs.openssl' failed.

ERROR: Feature 'xcb' was enabled, but the pre-condition 'libs.xcb' failed.

ERROR: Feature 'system-freetype' was enabled, but the pre-condition 'features.freetype && libs.freetype' failed.

ERROR: Feature 'fontconfig' was enabled, but the pre-condition '!config.win32 && !config.darwin && features.system-freetype && libs.fontconfig' failed.
funcs.mk:242: recipe for target '/home/ubuntu/build/bitcoin/depends/work/build/riscv64-linux-gnu/qt/5.9.6-c69c157d1d9/qtbase/.stamp_configured' failed
make: *** [/home/ubuntu/build/bitcoin/depends/work/build/riscv64-linux-gnu/qt/5.9.6-c69c157d1d9/qtbase/.stamp_configured] Error 3
make: Leaving directory '/home/ubuntu/build/bitcoin/depends'

@ken2812221 ken2812221 force-pushed the ken2812221:gitian-risc-v branch to c4aecd1 Aug 8, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

@MarcoFalke Sry, missing one line during rebasing #13710

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

@MarcoFalke @DrahtBot fetch the wrong commit.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

What is the reason this is still WIP? would like to merge this early in the 0.18 cycle if possible.
(going to test)

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

@laanwj You can see @DrahtBot 's binary, it exports so many unnecessary symbols

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

I'm not sure how worried we should be about exporting unnecessary symbols on RISC-V at this point, from what I gather everything is still extremely experimental, there aren't even ready-to-use linux distribution images.

The "don't export unnecessary symbols" check was, at the time, added to avoid collisions between statically-linked and dynamically-linked OpenSSL, back in the day that Qt was still linked dynamically. This is no longer an actual issue.
Now it's only a "nice" sanity and hygiene check, to avoid the overhead of huge unnecessary symbol tables in an executable.

So I don't think that needs to hold back progress, seems like be something that is improved in a later PR, but I'll leave it up to you.

@ken2812221 ken2812221 changed the title [WIP][build] Add risc-v support to gitian [build] Add risc-v support to gitian Aug 16, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

@laanwj Thanks for clarification. The export symbol things can be done in future PR. At least I can't solve the problem for now, leave for someone who can work on improving this.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

ACK c4aecd1
gitian build result tested on actual hardware (HiFive Unleashed=U540) w/ Fedora release 29 (Rawhide):

# ./test_bitcoin 
Running 303 test cases...
test/scheduler_tests.cpp(141): info: counter2++
test/scheduler_tests.cpp(141): info: counter2++
test/scheduler_tests.cpp(145): info:  == 
test/scheduler_tests.cpp(145): info: i          
*** No errors detected

Also synced the beginning of the chain, I'll keep this node running.

Going to merge this because I want to keep building for RISC-V on the 0.18 branch. If any issues come up we can revert this before the 0.18.0 release.

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Aug 16, 2018

Merge bitcoin#13665: [build] Add risc-v support to gitian
c4aecd1 Add risc-v 64-bit to gitian (Chun Kuan Lee)
96dda8b [depends] Add riscv qt depends support for cross compiling bitcoin-qt (Chun Kuan Lee)

Pull request description:

  Based on ~bitcoin#13660~ bitcoin#13710 ,  add gitian tarball for RISC-V

Tree-SHA512: 8db73545a2ea7fe03fa156598479335ea3c79aa3fb9c5cc44b8563094b1deb7c94d29c1dab47fac129dbfa2e3e774301b526474beeeb59c9b0087d3ea087dbd6

@laanwj laanwj merged commit c4aecd1 into bitcoin:master Aug 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ken2812221 ken2812221 deleted the ken2812221:gitian-risc-v branch Aug 16, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Also synced the beginning of the chain, I'll keep this node running.

This was successful!
Synchronization of the full chain on the HiFive Unleashed board to a nbd partition took about 2-3 days.
(had to restart at some point as there were problems with the SD card interface or driver, but there were zero issues with bitcoind)

@bitcoin bitcoin deleted a comment from DrahtBot Aug 23, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.