-
Notifications
You must be signed in to change notification settings - Fork 704
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
Add support and CI workflow for riscv64 #1627
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.
Thank you for this. I appreciate the thoroughness of this PR. A few questions.
There is a merge conflict in ci.yml that needs to be resolved before I can submit this to Github Actions. Please rebase this onto main. |
05e5107
to
04282ec
Compare
Thanks for the thorough review :) Applied the pointed out suggestions and rebased onto main! |
Have you really tested this on riscv32? I'm wondering if we should just drop that part. |
I personally only tested riscv64. The commit contains both as it's based on the work of @amock and @xfbs, while the second commit is based on @light4's implementation, which was targeted for riscv64 only. I believe we will have tests through the CI workflow for riscv64, do you prefer that we add an entry in the CI config for riscv32 as well or rather to remove that part from the first commit? |
I'd rather we just remove it for now. We can add it back if/when we have somebody who is really needing it and who can help test it. |
f2ad112
to
10af6ca
Compare
Alright then. I'll keep riscv32 support on this branch and limited this branch to riscv64 now (3bc2da6). Regarding the previous comment, I've also added the missing |
Codecov Report
@@ Coverage Diff @@
## main #1627 +/- ##
==========================================
+ Coverage 92.12% 96.01% +3.88%
==========================================
Files 132 133 +1
Lines 18917 19885 +968
Branches 199 199
==========================================
+ Hits 17428 19092 +1664
+ Misses 1451 754 -697
- Partials 38 39 +1 see 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
include/ring-core/base.h
Outdated
@@ -89,6 +89,9 @@ | |||
#elif defined(__MIPSEL__) && defined(__LP64__) | |||
#define OPENSSL_64_BIT | |||
#define OPENSSL_MIPS64 | |||
#elif defined(__riscv) && __riscv_xlen == 64 |
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.
BoringSSL uses __SIZEOF_POINTER__
. I don't think that using the pointer size is as relevant for ring as it is for BoringSSL so I am open to using __riscv_xlen
instead. Any thoughts on this?
#elif defined(__riscv) && __SIZEOF_POINTER__ == 8
#define OPENSSL_64_BIT
#define OPENSSL_RISCV64
@leso-kn You probably didn't notice but the CI failed. It seems like the linking configuration isn't correct. I would stil llike to use the clang-based build system, but if you can't get it working, I'm OK with reverting back to what you had before with the GCC-based build system. |
It seems like you just need to change the linking so that it continues to be done by the GCC. See #1297 and #1057 where we had to do the same thing. |
I posted PR #1669 which rebases your changes on top of the current main branch. |
Thank you for your help with this! |
Hey :) Sorry for the late reply, it was a quite busy week, TL is on vacation and the customer requested a lot of things at work. Thanks for rebasing it! I'll do it here as well just for completedness, but feel free to merge either request – probably yours as the pipeline is already in progress I see. |
2da9d6d
to
7d734f7
Compare
@briansmith Rebasing done, with commit signing and original committer dates in case you prefer that. But feel free to merge either of the PRs :) |
OK, If you prefer this way, I will let CI run your PR and then merge yours! |
c520ad9
to
ac39c13
Compare
Co-authored-by: Alan Mock <alan@alanmock.com> Co-authored-by: Patrick Elsen <pelsen@xfbs.net> Signed-off-by: leso-kn <info@lesosoftware.com>
Co-authored-by: light4 <root@i01.io> Signed-off-by: leso-kn <info@lesosoftware.com>
ac39c13
to
16837d1
Compare
@@ -507,6 +512,9 @@ jobs: | |||
- target: i686-unknown-linux-gnu | |||
host_os: ubuntu-22.04 | |||
|
|||
- target: riscv64gc-unknown-linux-gnu | |||
host_os: ubuntu-22.04 |
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.
The previous CI run passed except coverage
. coverage
failed because this was missing. I added it and now coverage
is passing.
Thank you for all your assistance with this! |
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring used (https://bugs.gentoo.org/912999). Ring has added RISC-V support in briansmith/ring#1627, so update ring and additional necessary crates to allow us to build on RISC-V. This change was compile tested inside a virtual machine.
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring used (https://bugs.gentoo.org/912999). Ring has added RISC-V support in briansmith/ring#1627, so update ring and additional necessary crates to allow us to build on RISC-V. This change was compile tested inside a virtual machine.
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring used (https://bugs.gentoo.org/918098). Ring has added RISC-V support in briansmith/ring#1627, so update ring and additional necessary crates to allow us to build on RISC-V. This change was compile tested inside a virtual machine.
As there's been few recent activity on related PRs, this is #1574 rebased on latest
main
with:Tested and working on Alpine Linux (riscv64 with musl libc).