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

ci(push-build): add RISC-V builds #504

Merged
merged 1 commit into from
Dec 24, 2023
Merged

ci(push-build): add RISC-V builds #504

merged 1 commit into from
Dec 24, 2023

Conversation

MichaIng
Copy link
Collaborator

@MichaIng MichaIng commented Mar 4, 2023

Let's see, whether it is that easy.

@MichaIng MichaIng requested a review from ravenclaw900 March 4, 2023 18:42
@MichaIng MichaIng added the enhancement New feature or request label Mar 4, 2023
@MichaIng MichaIng linked an issue Mar 4, 2023 that may be closed by this pull request
@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 4, 2023

Okay, not so easy:

Others worked around it by switching form rusttls to OpenSSL: Skallwar/suckit#196

A PR/patch on ring itself seems to as well use OpenSSL internally (EDIT: It does in general, but here assembler is disabled): briansmith/ring#1436
Alternative, doing basically the same: briansmith/ring#1574

@ravenclaw900
Copy link
Owner

It's kind of hilarious that rustls may eventually be using OpenSSL as a crypto backend anyway. Regardless, I'd rather not switch away from using rustls. Besides being more modern, it's been found to be faster and use less memory than OpenSSL (https://jbp.io/2019/07/01/rustls-vs-openssl-performance.html).

And, even if we did switch away from rustls (and replaced ring with sha2 in our dependency tree), ring is still used in jsonwebtoken, so we'd face the same problem. I think this is just going to be blocked until ring gets RISC-V support. Considering we're only 1 PR away, it will hopefully happen soon.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 5, 2023

Agreed. There are 4 PRs open about this, one by the ring repo owner itself: #1562

@omac777
Copy link

omac777 commented Mar 5, 2023

I was able to confirm
briansmith/ring#1574
works.

Please make sure to:
export RUSTC_WRAPPER=
before attempting the pull/checkout/build/test. I've described the steps I took to pull/checkout/build/test in the 1574 comments.

Yeah this cross build doesn't experience the same issue with sccache. It would definitely need to be built on actual riscv hardware to experience the exact issue. Please consider taking sccache out of the equation for this particular build by doing the above export for the mentioned environment variable. Why? It's because sccache itself uses the ring crate and can't be installed for the same reason.

@ravenclaw900
Copy link
Owner

I'll be honest, I have no idea why nothing is building now. I'm using what should be the original 0.16.20 source, and none of the changes I've made should affect anything else. Even when I switched to a version without any RISC-V related changes, it still didn't build.

@ravenclaw900
Copy link
Owner

Ah.

Probably we should make -Werror depend on whether .git exists; i.e. don't do -Werror when building from crates.io.

@MichaIng
Copy link
Collaborator Author

So far so good. Now some OpenSSL library or headers seem to be missing for the RISC-V arch, respectively the linker does not find them 🤔.

@ravenclaw900
Copy link
Owner

ravenclaw900 commented Mar 18, 2023

Failing functions: ring::aead::chacha20_poly1305_seal, ring::aead::chacha20_poly1305_open, ring::ec::suite_b::ops::p256::p256_elem_inv_squared. There might be more, but that's what showed up in the error message.

EDIT: Ran the test suite with cross on my computer, there's a lot more missing. I'm wondering if it's failing to link with OpenSSL at all.

@MichaIng
Copy link
Collaborator Author

We could try it with a RISC-V container (debian-slim Sid Docker or DietPi RISC-V Sid with systemd-nspawn) and QEMU emulation. I wouldn't wonder if cross-compiling itself is the problem.

@ravenclaw900
Copy link
Owner

I don't think the cross containers have OpenSSL installed by default, and attempting to install it for RISC-V doesn't get it to compile. I'd be happy to try in a separate Docker container.

P.S. Does DietPi have Docker support yet? I tried a while back and I got a ton of systemd errors, but I think this was before you could build specifically for a container.

@Joulinar
Copy link
Collaborator

Nope we don't have a DietPi own Docker container as we don't see the use case.

@ravenclaw900
Copy link
Owner

A niche use case, but it would make it easier for me to test DietPi-Dashboard on an actual DietPi system without having to spin up a full VM.

@Joulinar
Copy link
Collaborator

What about a WSL2 container?

@MichaIng
Copy link
Collaborator Author

We have these container images: https://dietpi.com/downloads/images/

But they do not work with Docker (or WSL2), only successfully tested with systemd-nspawn when mounting the image as loop device and bind-mounting* the loop device(s) into the container. This is as close as it gets to a real DietPi system, as systemd is included and functional. I use those to compile software for dietpi-software where I want to assure 100% matching shared libraries on any Debian and Raspbian version. There is a RISC-V container as well.

* The part with the needed bind mounts will be removed soon. This is needed as /etc/fstab is used and systemd otherwise hangs trying to wait for the rootfs (and in case bootfs) device. However, I'll remove those from /etc/fstab in containers to have tmpfs mounts only.

@MichaIng
Copy link
Collaborator Author

It compiled here: https://github.com/MichaIng/DietPi/actions/runs/4525391685/jobs/7972162111
Later webpki failed to compile, likely because my ring was based on unreleased 0.17.0, just patched to 0.16.20 version string to make cargo happy. I did the absolute minimal required change: https://github.com/MichaIng/ring/commit/c1ffda23d34e8f291321ad5c08b26bc89ffbffdf

Will try now the same with actual v0.16.20 as basis, but it is likely that recent changes made this possible.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 27, 2023

Okay I fail to succeed with v0.16.20 as well:

Very similar to how the build here fails. There are a bunch of other architecture specific RING_SRCS entries for x86 and ARM (mostly assembly) and I guess those are needed, respectively replacements for them, to provide all functions.

So cross-compiling vs emulation does not make a difference here.

For reference, my last (cumulative) commit: https://github.com/MichaIng/ring/commit/8a29d72ca5a6f860ccdef8e729ec277336dd6659

@omac777
Did you actually succeed to use/embed ring into another project on RISC-V, or did you compile and test it standalone only? Based on our results, all that can be found on GitHub to make this work should fail the same way here, so any report about people who succeeded are then likely standalone tests or when using only a subset of the actual features ring provides.
EDIT: Ah, or 0.17.0 ships the needed changes. I read about replacements for the assembly code, which sounds like it could be the missing part in 0.16.20. But it seems to have API changes, so other dependants need to update before it is usable in your case(s).

@ravenclaw900
Copy link
Owner

Ring now has an active PR for RISC-V support: briansmith/ring#1627.

@MichaIng
Copy link
Collaborator Author

I just saw and subscribed to this topic an hour ago, aiming to revive our attempt here once it has been merged 😄. I think, it will still take a while until a new Ring version is released, and then until the crates we need add support for this new Ring version. AFAIR there were some API changes.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Oct 14, 2023

jsonwebtoken still depends on ring 0.16.y: https://github.com/Keats/jsonwebtoken/blob/master/Cargo.toml

Request + PR for an update is there already: Keats/jsonwebtoken#333

... not the only one:

For rustls, there are alpha versions along the dependency tail, and we could use an alpha version of your library as well: ravenclaw900/flexible-hyper-server-tls#2

sct has last been released two years ago, so even that its ring dependency has been updated in its main branch already, we might need to wait for a long time for this to be released. So I suggest to fork and/or use the current main branch. Commits since last release were only formatting, documentational, CI and the ring dependency update, hence nothing to worry.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Oct 15, 2023

rustls still defines ring as optional dependency: https://github.com/rustls/rustls/blob/main/rustls/Cargo.toml
The alpha version on crates.io still the version 0.16. tokio-rustls does not define the related optional feature(s), so not sure why ring is added as rustls dependency in our Cargo.lock: https://github.com/rustls/tokio-rustls/blob/main/Cargo.toml

We also need to update rustls-webpki to alpha5: https://crates.io/crates/rustls-webpki/0.102.0-alpha.5/dependencies
... ah, its alpha4 now already, which is sufficient.

EDIT: Nice, finally! I fixed my jsonwebtoken fork already, but as long as there are no other relevant commits upstream, should be fine to use the branch or the ring PR.

@MichaIng
Copy link
Collaborator Author

Works:

root@DietPi:/tmp# ./dietpi-dashboard &
[1] 14875
root@DietPi:/tmp# ss -tulpn | grep dash
tcp   LISTEN 0      1024               *:5252            *:*    users:(("dietpi-dashboar",pid=14875,fd=9))

image

@MichaIng
Copy link
Collaborator Author

I just enabled the dashboard with these artifacts for RISC-V in dietpi-software, for testing purpose: MichaIng/DietPi@42a12f8

I'll keep an eye on the dependencies to switch back to upstream and stable where possible.

@MichaIng
Copy link
Collaborator Author

We could now "downgrade" to latest rustls stable releases, which have their ring dependencies all updated:

@MichaIng
Copy link
Collaborator Author

webpki v0.102 has been released: https://github.com/rustls/webpki/releases/tag/v/0.102.0
If I am not mistaken, this was the last blocker for RISC-V support with stable dependencies 🙂.

rustls-pemfile v2.0.0 has also been released, so the little code change regarding this can remain: ravenclaw900/flexible-hyper-server-tls@ab814aa
I'll open a PR to "downgrade" the rustls dependency once renovate and/or dependabot pushed all other dependency updates.

@MichaIng MichaIng force-pushed the risc-v branch 2 times, most recently from 0f612df to ad5b4e8 Compare December 19, 2023 21:45
@MichaIng MichaIng force-pushed the risc-v branch 2 times, most recently from 71314b0 to 9d0e9bd Compare December 19, 2023 21:58
Cargo.toml Outdated Show resolved Hide resolved
@MichaIng
Copy link
Collaborator Author

Seems like there was another breaking change in rustls between 2.0.0-alpha.1 and 2.0.0 release 🤔. I'll have a look tomorrow.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Dec 24, 2023

Works now without any other changes from main. Merging it. riscv64gc builds could be added to required checks.
EDIT: Ah, requires another approval 🙂.

@MichaIng MichaIng enabled auto-merge (squash) December 24, 2023 15:15
@MichaIng MichaIng merged commit d94ddfb into main Dec 24, 2023
11 checks passed
@MichaIng MichaIng deleted the risc-v branch December 24, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RISC-V builds/support
4 participants