From 3a1ea59da093f345869084952d66652bf8a63290 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 7 Sep 2024 21:05:19 +0000 Subject: [PATCH 1/8] Basic idea for `rvv-off` feature So far, this only attempts to have an effect when `cmake` is used, even though I documented it to be broader than that. In addition to testing this feature, it should either: - Be broadened to cover the non-`cmake` case (which I am unsure how to do, because which `-march` selection should be passed to `cc` varies according to other features too, which I believe `cmake` takes care of detecting), or - Have its documentation narrowed to say it only applies when `cmake` is used. --- Cargo-zng.toml | 6 ++++++ Cargo.toml | 6 ++++++ zng/cmake.rs | 3 +++ 3 files changed, 15 insertions(+) diff --git a/Cargo-zng.toml b/Cargo-zng.toml index 97b9c694..3df21680 100644 --- a/Cargo-zng.toml +++ b/Cargo-zng.toml @@ -38,5 +38,11 @@ libc = "0.2.43" [build-dependencies] cmake = "0.1.50" +[features] +# This feature builds riscv64 binaries portable to machines without RVV, even if the build machine +# is riscv64 and detected to support RVV. This is useful for building a more portable binary, or on +# systems where auto-detection is incorrect (as in https://github.com/zlib-ng/zlib-ng/issues/1705). +rvv-off = [] + [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(zng)'] } diff --git a/Cargo.toml b/Cargo.toml index cefdd469..729843d5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -89,6 +89,12 @@ zlib-ng = ["libc", "cmake"] # or be completely removed in the future zlib-ng-no-cmake-experimental-community-maintained = ["libc"] stock-zlib = [] +# This feature only affects zlib-ng and is only relevant to riscv64 targets. It +# builds binaries portable to machines without RVV, even if the build machine +# is riscv64 and detected to support RVV. This is useful for building a more +# portable binary, or on systems where auto-detection is incorrect +# (as in https://github.com/zlib-ng/zlib-ng/issues/1705). +rvv-off = [] # Deprecated: the assembly routines are outdated, and either reduce performance # or cause segfaults. asm = [] diff --git a/zng/cmake.rs b/zng/cmake.rs index 25576253..b3a86a54 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -14,6 +14,9 @@ pub fn build_zlib_ng(target: &str, compat: bool) { .define("WITH_DFLTCC_INFLATE", "1") .cflag("-DDFLTCC_LEVEL_MASK=0x7e"); } + if cfg!(feature = "rvv-off") && target.contains("riscv64") { + cmake.define("WITH_RVV", "OFF"); + } if target == "i686-pc-windows-msvc" { cmake.define("CMAKE_GENERATOR_PLATFORM", "Win32"); } From d76bc6d74e92fc21653906fe2ec1d3d342fbf887 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 7 Sep 2024 21:48:43 +0000 Subject: [PATCH 2/8] Also recognize an environment variable To aid in testing, and possibly as something that should even be kept in some form, this checks if an `RVV_OFF` environment variable is set and, if so, behaves as though the `rvv-off` feature is enabled, whether that feature is actually enabled or not. --- zng/cmake.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zng/cmake.rs b/zng/cmake.rs index b3a86a54..fbd71bd1 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -14,7 +14,7 @@ pub fn build_zlib_ng(target: &str, compat: bool) { .define("WITH_DFLTCC_INFLATE", "1") .cflag("-DDFLTCC_LEVEL_MASK=0x7e"); } - if cfg!(feature = "rvv-off") && target.contains("riscv64") { + if target.contains("riscv64") && (cfg!(feature = "rvv-off") || env::var_os("RVV_OFF").is_some()) { cmake.define("WITH_RVV", "OFF"); } if target == "i686-pc-windows-msvc" { From 377b942033cfaf07d9a3a632b459ded478d5c68b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 8 Sep 2024 00:33:32 +0000 Subject: [PATCH 3/8] Document that `rvv-off` feature is experimental and needs `cmake` + Use the exact same comment for it in both manifest files. --- Cargo-zng.toml | 9 ++++++--- Cargo.toml | 7 ++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/Cargo-zng.toml b/Cargo-zng.toml index 3df21680..72b8bada 100644 --- a/Cargo-zng.toml +++ b/Cargo-zng.toml @@ -39,9 +39,12 @@ libc = "0.2.43" cmake = "0.1.50" [features] -# This feature builds riscv64 binaries portable to machines without RVV, even if the build machine -# is riscv64 and detected to support RVV. This is useful for building a more portable binary, or on -# systems where auto-detection is incorrect (as in https://github.com/zlib-ng/zlib-ng/issues/1705). +# Experimental: This feature only affects zlib-ng, is only relevant to riscv64 +# targets, and currently only has an effect when `cmake` is used. It builds +# binaries portable to machines without RVV, even if the build machine is +# riscv64 and detected to support RVV. This is useful for building a more +# portable binary, or on systems where auto-detection is incorrect +# (as in https://github.com/zlib-ng/zlib-ng/issues/1705). rvv-off = [] [lints.rust] diff --git a/Cargo.toml b/Cargo.toml index 729843d5..25a1711d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -89,9 +89,10 @@ zlib-ng = ["libc", "cmake"] # or be completely removed in the future zlib-ng-no-cmake-experimental-community-maintained = ["libc"] stock-zlib = [] -# This feature only affects zlib-ng and is only relevant to riscv64 targets. It -# builds binaries portable to machines without RVV, even if the build machine -# is riscv64 and detected to support RVV. This is useful for building a more +# Experimental: This feature only affects zlib-ng, is only relevant to riscv64 +# targets, and currently only has an effect when `cmake` is used. It builds +# binaries portable to machines without RVV, even if the build machine is +# riscv64 and detected to support RVV. This is useful for building a more # portable binary, or on systems where auto-detection is incorrect # (as in https://github.com/zlib-ng/zlib-ng/issues/1705). rvv-off = [] From a8c813e5c770403d40fc9b728853a67a75b1489e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 8 Sep 2024 07:30:29 -0400 Subject: [PATCH 4/8] Document `RVV_OFF` environment variable in `rvv-off` comments --- Cargo-zng.toml | 5 +++-- Cargo.toml | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Cargo-zng.toml b/Cargo-zng.toml index 72b8bada..a46a5e48 100644 --- a/Cargo-zng.toml +++ b/Cargo-zng.toml @@ -43,8 +43,9 @@ cmake = "0.1.50" # targets, and currently only has an effect when `cmake` is used. It builds # binaries portable to machines without RVV, even if the build machine is # riscv64 and detected to support RVV. This is useful for building a more -# portable binary, or on systems where auto-detection is incorrect -# (as in https://github.com/zlib-ng/zlib-ng/issues/1705). +# portable binary, or on systems where auto-detection is incorrect (as in +# https://github.com/zlib-ng/zlib-ng/issues/1705). Building with the `RVV_OFF` +# environment variable set (to any value) is equivalent to this feature. rvv-off = [] [lints.rust] diff --git a/Cargo.toml b/Cargo.toml index 25a1711d..351713d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,8 +93,9 @@ stock-zlib = [] # targets, and currently only has an effect when `cmake` is used. It builds # binaries portable to machines without RVV, even if the build machine is # riscv64 and detected to support RVV. This is useful for building a more -# portable binary, or on systems where auto-detection is incorrect -# (as in https://github.com/zlib-ng/zlib-ng/issues/1705). +# portable binary, or on systems where auto-detection is incorrect (as in +# https://github.com/zlib-ng/zlib-ng/issues/1705). Building with the `RVV_OFF` +# environment variable set (to any value) is equivalent to this feature. rvv-off = [] # Deprecated: the assembly routines are outdated, and either reduce performance # or cause segfaults. From 7579d46d20d2c516853e926fd93374be246ab141 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 22 Sep 2024 06:35:34 -0400 Subject: [PATCH 5/8] Remove `rvv-off` feature --- Cargo-zng.toml | 10 ---------- Cargo.toml | 8 -------- zng/cmake.rs | 2 +- 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/Cargo-zng.toml b/Cargo-zng.toml index a46a5e48..97b9c694 100644 --- a/Cargo-zng.toml +++ b/Cargo-zng.toml @@ -38,15 +38,5 @@ libc = "0.2.43" [build-dependencies] cmake = "0.1.50" -[features] -# Experimental: This feature only affects zlib-ng, is only relevant to riscv64 -# targets, and currently only has an effect when `cmake` is used. It builds -# binaries portable to machines without RVV, even if the build machine is -# riscv64 and detected to support RVV. This is useful for building a more -# portable binary, or on systems where auto-detection is incorrect (as in -# https://github.com/zlib-ng/zlib-ng/issues/1705). Building with the `RVV_OFF` -# environment variable set (to any value) is equivalent to this feature. -rvv-off = [] - [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(zng)'] } diff --git a/Cargo.toml b/Cargo.toml index 351713d4..cefdd469 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -89,14 +89,6 @@ zlib-ng = ["libc", "cmake"] # or be completely removed in the future zlib-ng-no-cmake-experimental-community-maintained = ["libc"] stock-zlib = [] -# Experimental: This feature only affects zlib-ng, is only relevant to riscv64 -# targets, and currently only has an effect when `cmake` is used. It builds -# binaries portable to machines without RVV, even if the build machine is -# riscv64 and detected to support RVV. This is useful for building a more -# portable binary, or on systems where auto-detection is incorrect (as in -# https://github.com/zlib-ng/zlib-ng/issues/1705). Building with the `RVV_OFF` -# environment variable set (to any value) is equivalent to this feature. -rvv-off = [] # Deprecated: the assembly routines are outdated, and either reduce performance # or cause segfaults. asm = [] diff --git a/zng/cmake.rs b/zng/cmake.rs index fbd71bd1..ecf0f878 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -14,7 +14,7 @@ pub fn build_zlib_ng(target: &str, compat: bool) { .define("WITH_DFLTCC_INFLATE", "1") .cflag("-DDFLTCC_LEVEL_MASK=0x7e"); } - if target.contains("riscv64") && (cfg!(feature = "rvv-off") || env::var_os("RVV_OFF").is_some()) { + if target.contains("riscv64") && env::var_os("RVV_OFF").is_some() { cmake.define("WITH_RVV", "OFF"); } if target == "i686-pc-windows-msvc" { From dbbe477669d978dce5bbb5c5b787b4311901bc99 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 22 Sep 2024 07:40:30 -0400 Subject: [PATCH 6/8] Use `RISCV_WITH_RVV` as the env var, taking ON/OFF values Instead of `RVV_OFF`. This checks if the value appears to match any of several hard-coded truthy or falsy values. Typically the literal values `ON` or `OFF` would be used, as is typically done when passing boolean `-D...` options to `cmake` on the command-line. It is typically more useful to pass `OFF` here than `ON`, becuase the default is `ON`, as noted in the linked section of the zlib-ng readme. At least for now, empty, blank, and otherwise unrecognized values are simply ignored. Recognized values are always "normalized" as either `ON` (if truthy) or `OFF` (if falsy). --- zng/cmake.rs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/zng/cmake.rs b/zng/cmake.rs index ecf0f878..7e48aea8 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -1,4 +1,5 @@ use std::env; +use std::ffi::OsStr; pub fn build_zlib_ng(target: &str, compat: bool) { let mut cmake = cmake::Config::new("src/zlib-ng"); @@ -14,8 +15,25 @@ pub fn build_zlib_ng(target: &str, compat: bool) { .define("WITH_DFLTCC_INFLATE", "1") .cflag("-DDFLTCC_LEVEL_MASK=0x7e"); } - if target.contains("riscv64") && env::var_os("RVV_OFF").is_some() { - cmake.define("WITH_RVV", "OFF"); + if target.contains("riscv") { + // Check if we should pass on an explicit boolean value of the WITH_RVV build option. + // See: https://github.com/zlib-ng/zlib-ng?tab=readme-ov-file#advanced-build-options + match env::var_os("RISCV_WITH_RVV") + .map(OsStr::to_str) + .map(str::trim) + .map(str::to_uppercase) + .map(Into::into) + { + Some("OFF" | "NO" | "FALSE" | "0") => { + // Force RVV off. This turns off RVV entirely, as well as the runtime check for it. + cmake.define("WITH_RVV", "OFF"); + } + Some("ON" | "YES" | "TRUE" | "1") => { + // Try to use RVV, but still don't do so if a runtime check finds it unavailable. + // This has the same effect as omitting WITH_RVV, unless it has already been set. + cmake.define("WITH_RVV", "ON"); + } + } } if target == "i686-pc-windows-msvc" { cmake.define("CMAKE_GENERATOR_PLATFORM", "Win32"); From 66df3f59e8eebbaac7cd89505157d34db947fffd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 22 Sep 2024 07:46:46 -0400 Subject: [PATCH 7/8] Say why it occasionally makes sense to force RVV off This is adapted from the comment on the feature that was added under the design that was previously explored. This way, the description will be present somewhere, even though it may be less discoverable here in `cmake.rs` than it would be in manifests. --- zng/cmake.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zng/cmake.rs b/zng/cmake.rs index 7e48aea8..6bfc949a 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -26,6 +26,9 @@ pub fn build_zlib_ng(target: &str, compat: bool) { { Some("OFF" | "NO" | "FALSE" | "0") => { // Force RVV off. This turns off RVV entirely, as well as the runtime check for it. + // This is not usually necessary, but can be useful for building binaries portable + // to systems that do not support RVV but where auto-detection fails to identify + // this (as in https://github.com/zlib-ng/zlib-ng/issues/1705). cmake.define("WITH_RVV", "OFF"); } Some("ON" | "YES" | "TRUE" | "1") => { From 8636464ca620113c2cbe76b6631aaa468deac8ed Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 22 Sep 2024 07:54:17 -0400 Subject: [PATCH 8/8] Fix up the environment variable string conversion * Tweak string conversion For correctness and MSRV compatibility. * Match and explicitly do nothing for absent or strange value * Fix the conversion lifetime by reorganizing the code * Slightly simplify --- zng/cmake.rs | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/zng/cmake.rs b/zng/cmake.rs index 6bfc949a..5d6b6eb8 100644 --- a/zng/cmake.rs +++ b/zng/cmake.rs @@ -1,5 +1,4 @@ use std::env; -use std::ffi::OsStr; pub fn build_zlib_ng(target: &str, compat: bool) { let mut cmake = cmake::Config::new("src/zlib-ng"); @@ -18,23 +17,21 @@ pub fn build_zlib_ng(target: &str, compat: bool) { if target.contains("riscv") { // Check if we should pass on an explicit boolean value of the WITH_RVV build option. // See: https://github.com/zlib-ng/zlib-ng?tab=readme-ov-file#advanced-build-options - match env::var_os("RISCV_WITH_RVV") - .map(OsStr::to_str) - .map(str::trim) - .map(str::to_uppercase) - .map(Into::into) - { - Some("OFF" | "NO" | "FALSE" | "0") => { - // Force RVV off. This turns off RVV entirely, as well as the runtime check for it. - // This is not usually necessary, but can be useful for building binaries portable - // to systems that do not support RVV but where auto-detection fails to identify - // this (as in https://github.com/zlib-ng/zlib-ng/issues/1705). - cmake.define("WITH_RVV", "OFF"); - } - Some("ON" | "YES" | "TRUE" | "1") => { - // Try to use RVV, but still don't do so if a runtime check finds it unavailable. - // This has the same effect as omitting WITH_RVV, unless it has already been set. - cmake.define("WITH_RVV", "ON"); + if let Ok(value) = env::var("RISCV_WITH_RVV") { + match value.trim().to_uppercase().as_str() { + "OFF" | "NO" | "FALSE" | "0" => { + // Force RVV off. This turns off RVV entirely, as well as the runtime check for it. + // This is not usually necessary, but can be useful for building binaries portable + // to systems that do not support RVV but where auto-detection fails to identify + // this (as in https://github.com/zlib-ng/zlib-ng/issues/1705). + cmake.define("WITH_RVV", "OFF"); + } + "ON" | "YES" | "TRUE" | "1" => { + // Try to use RVV, but still don't do so if a runtime check finds it unavailable. + // This has the same effect as omitting WITH_RVV, unless it has already been set. + cmake.define("WITH_RVV", "ON"); + } + _ => {} } } }