Fix failed builds on RISC-V (64-bit) #3184

julianbraha · 2022-02-23T16:04:51Z

These changes add support for the RISC-V (64-bit) architecture by updating the ioctl-sys dependency, and replacing usage of the ureq crate with reqwest for downloading tldr.

ioctl-sys was updated to support 64-bit RISC-V about 5 months ago, with this commit: codyps/ioctl@295ed33
The ureq crate depends on rustls, which depends on ring. ring does not support RISC-V or other less-popular architectures, which leads to build failures on these systems. The reqwest crate is less architecture-dependent, and can be used to download tldr, instead.

All changes have been tested with cargo test on both x86-64 GNU/Linux, and riscv64 GNU/Linux in QEMU.
x86-64 GNU/Linux has no regressions.
riscv64 GNU/Linux in QEMU succeeds in building, but fails the following tests:

test_cp::test_cp_reflink_insufficient_permission
test_link::test_link_one_argument
test_mv::test_mv_permission_error
test_shred::test_shred_force
test_sort::test_tmp_files_deleted_on_sigint
test_test::test_file_not_owned_by_egid
test_test::test_file_not_owned_by_euid
test_touch::test_touch_permission_denied_error_msg
test_touch::test_touch_system_fails

sylvestre · 2022-02-23T16:16:36Z

src/uu/cp/Cargo.toml

@@ -28,7 +28,7 @@ uucore = { version=">=0.0.11", package="uucore", path="../../uucore", features=[
 walkdir = "2.2"

 [target.'cfg(target_os = "linux")'.dependencies]
-ioctl-sys = "0.6"
+ioctl-sys = { git = "https://github.com/jmesmon/ioctl.git", rev = "295ed33a8704a0eff31bebdcf6ca0bfeb93bf675" }


I am uncomfortable depending on some personal forked git repo.
So, I won't accept this change. Sorry :(
Do you know why it isn't merged upstream + published as a new crate?
thanks

I'm not sure why a new release hasn't been made yet, but that repo is actually the upstream, official ioctl-sys.
I'll ask them about updating the crate.

OK, thanks for codyps/ioctl#22 :)

A new crate release was just made and jmesmon/ioctl#22 was resolved, so I've amended commit 4345c2a to use the new version 0.8.0.

sylvestre · 2022-02-23T16:17:01Z

Cool stuff. Do you know if there is a way to get this tested on a github CI?

julianbraha · 2022-02-23T16:30:26Z

I haven't tried it, but there is a QEMU github action: https://github.com/docker/setup-qemu-action
I followed this guide to set up the VM: https://colatkinson.site/linux/riscv/2021/01/27/riscv-qemu/

sylvestre · 2022-02-23T18:26:12Z

I guess you saw that a few jobs are failing

julianbraha · 2022-02-23T20:09:18Z

I guess you saw that a few jobs are failing

I've amended the existing commits to fix problems with the spell checker, as well as an outdated Cargo.lock.

There are also some style warnings about existing code that my commits didn't modify.
I'm seeing a number of warning: use of deprecated struct `clap::App`: Replaced with `Command` in src/bin/uudoc.rs

julianbraha · 2022-02-23T20:43:53Z

Fixed the style problems. Didn't catch the cargo fmt suggestion earlier.

julianbraha · 2022-02-24T18:02:39Z

Rebased on #3186 to resolve an unrelated CI error.
Still not sure why I'm getting this:

  Downloaded crossterm v0.23.0
error: failed to parse manifest at `/home/julian/.cargo/registry/src/github.com-1ecc6299db9ec823/crossterm-0.23.0/Cargo.toml`

Caused by:
  feature `edition2021` is required

  this Cargo does not support nightly features, but if you
  switch to nightly channel you can add
  `cargo-features = ["edition2021"]` to enable this feature

After running:

cargo +1.54.0 update
cargo +1.54.0 build

EDIT:
The problem was the cargo +1.54.0 update. For some reason, I thought that updated the Cargo.lock. It does, but it also increases dependency versions unnecessarily.

julianbraha · 2022-02-28T05:06:09Z

Updated the ioctl-sys dependency to use the new crate version, 0.8.0.
Resolved CI warnings by (1) rebasing (2) undoing a cargo update (3) removing unecessary use's.

However, it looks like the added openssl dependency from switching to reqwest will cause cross-compilation to fail in the CI.
@sylvestre is it okay to introduce an openssl dependency (and just updated the CI)? Or should I look for another alternative to ureq?

tertsdiepraam · 2022-02-28T11:24:59Z

Can we get rid of the openssl dependency by using reqwest with rustls by using the rustl-tls feature?

From this bit in the docs it looks like openssl is only used when native tls is used:
https://docs.rs/reqwest/latest/reqwest/#tls

julianbraha · 2022-02-28T19:31:07Z

Can we get rid of the openssl dependency by using reqwest with rustls by using the rustls-tls feature?

The rustls-tls feature introduces a dependency on ring, which doesn't support RISC-V. This is the same problem with the current ureq crate.

tertsdiepraam · 2022-03-05T19:58:40Z

I see. Thanks for checking! Let's review our options:

Make uudoc optional and ignore the fact that it doesn't build on RISC-V (not great)
Use reqwest, introduce the OpenSSL dependency. I think having openssl as a dependency is fine, as long as it doesn't break the CI. So if you can fix that, I would accept this, unless @sylvestre has a different opinion.
Make the download of the examples an optional manual step. So, if you don't download the examples, they won't show up and it will print a warning. If you do have it then it will show up like it does now.
Find some other crate, although we are unlikely to find one that works, because they would need some TLS implementation. Unless there is some crate that just does HTTP without TLS.

2 & 3 are both fine with me.

sylvestre · 2022-03-11T22:47:02Z

Cargo.toml

@@ -252,7 +253,8 @@ lazy_static = { version="1.3" }
 textwrap = { version="0.14", features=["terminal_size"] }
 uucore = { version=">=0.0.11", package="uucore", path="src/uucore" }
 selinux = { version="0.2", optional = true }
-ureq = "2.4.0"
+reqwest = { version="0.11.9", features = ["blocking"] }


this is pulling a lot of new deps, is it possible to avoid that?

I'm unaware of any way to reduce the dependencies, besides simply not using reqwest.

We could simply wait for ring to add RISC-V support, and use the existing ureq crate. There's already a PR for this: briansmith/ring#1459

So you're suggesting that we close this PR? We could create an issue referencing that PR instead.

We could close this PR and create a different one if we want to take that route.
The downside is that we would still be depending on ring's architecture support, which is problematic for other systems (see #3216).

I would prefer to try and resolve the CI issues with openssl & cross-compilation instead.

darleybarreto · 2022-04-15T17:40:42Z

If I understood correctly, one can disable rustls feature by writing

ureq = { version = "2.4.0", default-features = false, features = ["gzip"]}

Did I miss something?

julianbraha · 2022-04-15T17:49:12Z

@darleybarreto the ureq crate will use native-tls in that case, which introduces an openssl system dependency, and breaks cross-compilation builds in the CI (which is the current state of this PR).

I should have some time to look at this again this weekend.

darleybarreto · 2022-04-15T17:58:09Z

Oh, I thought native-tls would use something available on any particular platform it supports. I just found an issue on rustls repo about ring. Perhaps pinging them about this issue would be interesting!

The ureq dependency is causing compilation errors on various platforms (see uutils#3184, uutils#3216, uutils#3375). Hence we remove that dependency and do not automatically download the archive anymore. Instead, we ask the user to download it separately when the archive is not found.

The ureq dependency is causing compilation errors on various platforms (see #3184, #3216, #3375). Hence we remove that dependency and do not automatically download the archive anymore. Instead, we ask the user to download it separately when the archive is not found.

tertsdiepraam · 2022-04-18T11:08:42Z

Closing this as both issues in this PR have been fixed. Thank you for your efforts and for bringing these to our attention!

sylvestre reviewed Feb 23, 2022

View reviewed changes

julianbraha force-pushed the risc-v branch from 25697e4 to 21d24dd Compare February 23, 2022 20:02

julianbraha force-pushed the risc-v branch from 21d24dd to e26cba0 Compare February 23, 2022 20:43

julianbraha force-pushed the risc-v branch from 86663e6 to 5bc46c1 Compare February 24, 2022 17:58

julianbraha force-pushed the risc-v branch 3 times, most recently from 4345c2a to e4fc557 Compare February 28, 2022 04:50

tertsdiepraam mentioned this pull request Mar 6, 2022

macOS m1 pro/max ARM64 binary? #3216

Closed

julianbraha added 2 commits March 9, 2022 19:45

docs: replace usage of ureq with reqwest for architecture independence

e28fa4b

update ioctl-sys for risc-v support

79efc07

sylvestre force-pushed the risc-v branch from 762186d to 79efc07 Compare March 9, 2022 18:45

sylvestre reviewed Mar 11, 2022

View reviewed changes

sylvestre added the awaiting response label Mar 15, 2022

tertsdiepraam mentioned this pull request Apr 8, 2022

Windows build cause an error with "ring" #3375

Closed

tertsdiepraam mentioned this pull request Apr 17, 2022

uudoc: Don't download the tldr archive automatically #3415

Merged

tertsdiepraam closed this Apr 18, 2022

tertsdiepraam mentioned this pull request Oct 3, 2022

Starting with localization #3997

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failed builds on RISC-V (64-bit) #3184

Fix failed builds on RISC-V (64-bit) #3184

julianbraha commented Feb 23, 2022

sylvestre Feb 23, 2022

julianbraha Feb 23, 2022

sylvestre Feb 23, 2022

julianbraha Feb 28, 2022

sylvestre commented Feb 23, 2022

julianbraha commented Feb 23, 2022

sylvestre commented Feb 23, 2022

julianbraha commented Feb 23, 2022 •

edited

Loading

julianbraha commented Feb 23, 2022

julianbraha commented Feb 24, 2022 •

edited

Loading

julianbraha commented Feb 28, 2022 •

edited

Loading

tertsdiepraam commented Feb 28, 2022 •

edited

Loading

julianbraha commented Feb 28, 2022 •

edited

Loading

tertsdiepraam commented Mar 5, 2022

sylvestre Mar 11, 2022

julianbraha Mar 17, 2022

tertsdiepraam Mar 18, 2022

julianbraha Mar 18, 2022 •

edited

Loading

darleybarreto commented Apr 15, 2022 •

edited

Loading

julianbraha commented Apr 15, 2022

darleybarreto commented Apr 15, 2022 •

edited

Loading

tertsdiepraam commented Apr 18, 2022 •

edited

Loading

Fix failed builds on RISC-V (64-bit) #3184

Fix failed builds on RISC-V (64-bit) #3184

Conversation

julianbraha commented Feb 23, 2022

sylvestre Feb 23, 2022

Choose a reason for hiding this comment

julianbraha Feb 23, 2022

Choose a reason for hiding this comment

sylvestre Feb 23, 2022

Choose a reason for hiding this comment

julianbraha Feb 28, 2022

Choose a reason for hiding this comment

sylvestre commented Feb 23, 2022

julianbraha commented Feb 23, 2022

sylvestre commented Feb 23, 2022

julianbraha commented Feb 23, 2022 • edited Loading

julianbraha commented Feb 23, 2022

julianbraha commented Feb 24, 2022 • edited Loading

julianbraha commented Feb 28, 2022 • edited Loading

tertsdiepraam commented Feb 28, 2022 • edited Loading

julianbraha commented Feb 28, 2022 • edited Loading

tertsdiepraam commented Mar 5, 2022

sylvestre Mar 11, 2022

Choose a reason for hiding this comment

julianbraha Mar 17, 2022

Choose a reason for hiding this comment

tertsdiepraam Mar 18, 2022

Choose a reason for hiding this comment

julianbraha Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

darleybarreto commented Apr 15, 2022 • edited Loading

julianbraha commented Apr 15, 2022

darleybarreto commented Apr 15, 2022 • edited Loading

tertsdiepraam commented Apr 18, 2022 • edited Loading

julianbraha commented Feb 23, 2022 •

edited

Loading

julianbraha commented Feb 24, 2022 •

edited

Loading

julianbraha commented Feb 28, 2022 •

edited

Loading

tertsdiepraam commented Feb 28, 2022 •

edited

Loading

julianbraha commented Feb 28, 2022 •

edited

Loading

julianbraha Mar 18, 2022 •

edited

Loading

darleybarreto commented Apr 15, 2022 •

edited

Loading

darleybarreto commented Apr 15, 2022 •

edited

Loading

tertsdiepraam commented Apr 18, 2022 •

edited

Loading