Recognize RISCV_WITH_RVV env var on RISC-V to set WITH_RVV cmake var

[EliahKagan]

This adds an rvv-off feature that prevents RVV vector instructions from being emitted in riscv64 builds even when the build machine is detected to support RVV. As discussed in #200 (comment) and surrounding comments, this might be considered to fix #200.

I don't regard this to be in especially good shape, mostly because it only affects builds that use cmake. I did not include any changes to cc.rs to try to get it to work without cmake.

To make it easier to test this, I have also added recognition of an RVV_OFF environment variable with the same effect, whenever present with any value. Current use of this feature would often likely be to work around zlib-ng/zlib-ng#1705 as described in #200, but this might also make it undesirable to use it as a feature, since it would involve modifying multiple crates in a transitive dependency chain. Using an environment variable is less reliable--at least currently implemented, rerunning cargo commands with a different value of the environment variable would not automatically rebuild anything--but more usable for this anticipated case. It also helped me in testing.

I am not sure about the names of either the feature or the environment variable. For the feature, I suspect it should have "experimental" in its name and not just its description, since zlib-ng-no-cmake-experimental-community-maintained does and this is at least as experimental as that (and also exacerbate that feature's experimental nature, but not actually having an effect when that feature is used).

I have not added anything in the test suite or to any CI scripts to test this. There seems to be a significant amount of functionality not under test in these ways, so maybe that is okay, but I am not sure.

Edit: Since opening this, I have noticed that the feature vs. environment variable considerations I was referring to have already been more precisely, clearly, and succinctly presented in #206 (comment). (However, since the functionality here and in #206 differ significantly in how they would likely be used, I don't know if a decision in either PR about this would predict a decision in the other.)

---

To test this manually, I used gitoxide, since that is the software where I observed #200 as documented there. First I produced and populated a libz-ng-sys directory along the libz-sys working tree by running the cargo-zng script after temporarily modifying it in this way:

diff --git a/cargo-zng b/cargo-zng
index 4dd8ee3..ca072e3 100755
--- a/cargo-zng
+++ b/cargo-zng
@@ -1,7 +1,7 @@
 #!/bin/bash
 set -eu
-tempdir="$(mktemp -d)"
-trap 'rm -rf "$tempdir"' 0 INT
+tempdir=../libz-ng-sys; mkdir "$tempdir"  # tempdir="$(mktemp -d)"
+# trap 'rm -rf "$tempdir"' 0 INT
 cargo package -l --allow-dirty |
     tr '\\' '/' |
     grep -vxF -e Cargo.toml.orig -e .cargo_vcs_info.json |
@@ -10,5 +10,5 @@ cargo package -l --allow-dirty |
 cp Cargo-zng.toml "$tempdir/Cargo.toml"
 cp -a systest "$tempdir/systest"
 mv "$tempdir/systest/Cargo-zng.toml" "$tempdir/systest/Cargo.toml"
-cd "$tempdir"
-cargo "$@"
+# cd "$tempdir"
+# cargo "$@"

Then, in the gitoxide working tree, which was in the same parent directory as libz-sys and libz-ng-sys, I verified that gix status and a gix clone command both failed as described in #200 without patching in the changes here, as well as when they were patched in but nothing was set to enable them.

Then I patched them in and also set RVV_OFF to 1. (I chose this value because it is intuitive. As noted above, the value is currently ignored.)

RVV_OFF=1 cargo build --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"'

That fixed the crash, as hoped.

That shows gix status. A test with gix clone, as well as a number of tests to verify that without the change the problem occurs, i.e. to validate this manual testing procedure itself, appear in this other longer and fairly disorganized transcript. (When I ran vim there, I did not use it to modify any files.)

[Byron]

Thanks a lot for tackling this!

What I like about this solution is that there is a documented cargo feature which (soon) would also advertise a corresponding environment variable. This resolves my biggest criticism towards using them - the difficulty to discover them outside of reading the code of build-scripts.

The only question left would be to what extend it has to be experimental. Is it the name that could change, or the way it's implemented, or is it the lack of CC support?
Under which circumstances would it be possible to remove the experimental status?
My answer to this would be "Once cc.rs also has support for it". As a matter of fact, if flate2 could use CC instead of cmake in the zlib-ng backend, that would definitely make it more accessible and gitoxide would be easier to compile as well.

I'd also be looking forward to hearing what @jongiddy thinks about this.

[EliahKagan]

What I like about this solution is that there is a documented cargo feature which (soon) would also advertise a corresponding environment variable. This resolves my biggest criticism towards using them - the difficulty to discover them outside of reading the code of build-scripts.

Would that be applicable to #206 as well then--could having both a feature and an environment variable there make sense?

If so, should the environment variables be named with a similar naming convention? I assume that, if done, this would entail keeping the current name there and expanding this one. Relatedly, maybe that would shed light on whether the value should be examined.

[Byron]

Yes, there it's a sole environment variable without documentation in lib.rs (on crate level at least for now). Here we started with something very specific to make it more suitable as a cargo feature, with the environment variable as another way to enable the feature to make it easier to enable for complex builds.

Admittedly, the only reason I am unsure about this is that I wonder what the original authors would want, or in lieu of this, what is most common with *-sys crates. To my mind, I'd not use environment variables at all unless they are strongly requested, and if that's done, they'd need proper documentation.

If this PR should be more like #206 instead, the environment variable could be named less specifically, the value could matter, and it could be documented on crate level instead. It seems to be more 'common' at least.

[jongiddy]

I'm OK with the current PR. My one comment is mostly for consideration rather than requiring action.

I do suggest that a specific value is required for the environment variable, say RVV_OFF=1. An empty string RVV_OFF= should be the same as being unset. Any other values trigger an error. This allows flexibility if other variants are required.

[jongiddy]

Generally I prefer "positive" features over "negative" features. By this I mean that the feature sounds like it adds something rather than taking something away.

For example, you could name the environment variable WITH_RVV and allowing the values ON and OFF. This would be familiar to someone who knows the zlib-ng build. It would allow forcing the use of RVV if there was ever a case where it was disabled.

You could name the feature with-rvv. However this would require the default to compile without RVV. This has the advantage of working in more places, at the cost of behaving differently to the default behavior of zlib-ng.

[Byron]

Thanks a lot for sharing your expertise, @jongiddy!

I'd be happy to go with all of the suggestions, while staying conservative on how the feature is handled so behaviour isn't changed. @EliahKagan are these changes you could do? And if present, please share your questions or suggestions as well.

[EliahKagan]

It seems to me that there is no particularly good design available here, and that it might be best either to wait to see what progress looks like on zlib-ng/zlib-ng#1705 (see zlib-ng/zlib-ng#1770), or to generalize this into a mechanism for passing arbitrary -D options to cmake.

I think the problems here both for a feature (and its name) and for an environment variable (and its name) are that:

• Any change from the current default behavior--other than fixing the bug that motivated this, which should be done upstream, and which may not suffice for binaries that are to be redistributed--is actually taking build functionality away. The default behavior is to attempt to auto-detect if RVV instructions should be emitted. This behavior is, conceptually, a feature. Auto-detection usually works, but fails on some RISC-V systems without RVV on some kernels. Any way of replacing auto-detection with a pre-set choice is taking away that feature.
• Making the current behavior a feature, so RVV is forced off unless the feature is enabled, would make many builds generate slower code, such that users would likely assume there had simply been a performance regression and not investigate. Making it a default feature would still have this effect for builds where default features are disabled in order to avoid getting new default features, though perhaps that practice is rare. Making the current behavior a feature would also introduce skew between the cmake and non-cmake cases in a way that is very unintuitive--one would lose build functionality by switching to cmake.
• In another relevant sense, neither having RVV nor omitting RVV is "negative," because if RVV instructions are not emitted, it is not as though the relevant functionality is omitted: instead, it is implemented with other instructions (that are usually slower). This should in theory make it possible to name the feature something that doesn't sound negative. But because the alternative instructions can be anything otherwise supported by the assumed CPU features--it is not as though we would be using XYZ instructions instead of RVV instructions--it may be hard to name an RVV-omitting feature precisely without making it sound negative. omit-rvv-for-compatibility sounds more active, but it doesn't sound less negative.

So I don't know what to do. I agree with the points raised by @jongiddy, but I don't think there is any easy way to address them properly. We can change which behavior a feature represents, and even separately from that we can rename it so it seems positive rather than negative, but I fear the situation would be inelegant, confusing, and misleading in almost the same way as in the PR now.

Not adding a feature, and implementing functionality where an environment variable--or separate environment variables--can be used to pass arbitrary -D options to cmake, seems to me like it could be a way out. But that itself has three interrelated problems:

• It is a more general solution to a problem that is not known, in the setting of this crate, to have other cases where it is needed (though it could be convenient in other cases for experimentation).
• It feels like it should be implemented in the cmake crate instead, as something dependent crates like this one can use.
• If implemented, it should allow multiple -D options to be passed. Then either they are passed in the same environment variable and we are introducing our own parsing rules (even if those rules are just "split on whitespace and don't recognize quoting"); or they are passed in separate environment variables (e.g. LIBZ_SYS_CMAKE_DEFINE_*), in which case we have to decide whether to respect case-folding for matching environment variable names on Windows.

I'd be happy to implement the more general functionality, but I'm checking first because I don't know if that is wanted.

[Byron]

Thanks a lot for sharing your thoughts, @EliahKagan , I thought they cover all the bases, and are more complete than what I had in my mind.

One part stuck out for me:

• Any change from the current default behavior--other than fixing the bug that motivated this, which should be done upstream, and which may not suffice for binaries that are to be redistributed--is actually taking build functionality away. The default behavior is to attempt to auto-detect if RVV instructions should be emitted. This behavior is, conceptually, a feature. Auto-detection usually works, but fails on some RISC-V systems without RVV on some kernels. Any way of replacing auto-detection with a pre-set choice is taking away that feature.

If we are actually waiting for this to be fixed downstream, but would like to offer a solution earlier, then it's in the nature of our solution that it's temporary. Maybe then we could keep it as is, as specific and negative as it may be, and add unstable-temporary to it, or some other suffix that indicates its fleeting nature.

Those who want to use it can, but they would be facing breaking builds eventually or pin the version. With that it would probably help to know if this feature could be removed in a patch or a minor release. My suggestion here would be a minor release to give some room.

Otherwise, if stability has to be guaranteed, I feel there is no way to do it sufficiently well.

[EliahKagan]

Even after the originally motivating upstream bug is fixed, the feature or other functionality for this in this project will remain useful for when one is building on a RISC-V system that supports RVV instructions, but planning to use the binaries on another system that does not support RVV instructions, or planning to redistribute them such that some users will need to run them on such a system.

Nonetheless, the strong connection between this and zlib-ng/zlib-ng#1705 remains, because that is the use case that motivated #200 and this, and also because whatever happens upstream may provide insight into what should be done here.

What if we call the feature skip-rvv-detection and the environment variable SKIP_RVV_DETECTION? Or skip-rvv-autodetection and SKIP_RVV_AUTODETECTION?

• An advantage is that it references the mechanism that one should wish to do without if one chooses to use this feature or to set this environment variable (or to set it to a truthy value).
• An advantage is that, because it is not labeled experimental or temporary, users may not be overly reluctant to use it. But this is, of course, a disadvantage if we are going to remove it later. I think, though, that since detection is what we are skipping, users who are building redistributable binaries will still want to turn that off and will still understand this feature as related to it.
• A disadvantage is that it is less specific than the current name or other names discussed so far: We are not just disabling auto-detection, we are explicitly turning off RVV.
• Depending on one's perspective, it is either an advantage or a disadvantage that this name sounds active and positive. It is an advantage to avoid distracting users with strange-sounding names. But I think that's only part of the point raised there about positive vs. negative feature names: features shouldn't just sound like they are adding something, but should, ideally, actually be adding something.

As an effectively separate decision, I lean toward thinking I should change the environment variable's handling to examine the value and treat some values as falsy. However, there is a disadvantage of doing so--there are ambiguous cases:

• The less serious kind of ambiguous case is values like blah that are inherently ambiguous. Users will typically know not to set that, and just about any handling is basically okay, including producing an error.
• But what should happen if the feature is turned on while the environment variable is set to a falsy value? Here, no way of handling it seems like it avoids doing something that would unpleasantly astonish some users. Some users will assume that the feature overrides the environment variable, others will assume the environment variable overrides the feature, others will assume that an ambiguous situation made up of settings that individually make sense will produce an error, and others will assume that the whole point of having both a feature and an environment variable is so that one of them can override the other without producing an error.

Is that a good argument against continuing just to check for the existence of the environment variable? Actually I think it is not:

• In practice people write things like VAR=1 even if any value will do. This creates the impression that VAR=0 might do something different.
• If we introduce other environment variables whose values are consulted, then it is even more unintuitive not to consult this one.

[Byron]

Thanks a lot for sharing, it feels like this conversation leads to a solution soon.

What if we call the feature skip-rvv-detection and the environment variable SKIP_RVV_DETECTION? Or skip-rvv-autodetection and SKIP_RVV_AUTODETECTION?

To me that sounds preferable over RVV_OFF, and I assume that WITH_RVV=OFF as passed to cmake actually disables auto-detection only.

Is that a good argument against continuing just to check for the existence of the environment variable? Actually I think it is not:

My intuition here was the opposite, primarily because I found it hard to decide what should override what. The solution seemed to be that if SKIP_RVV_AUTODETECTION is set to OFF or a similarly falsy value, it seems intuitive that it would override the cargo feature. The reason for this is that the feature can be controlled in the manifest, but the person building the software for distribution should have the final word on it.

So the logic would be something like if (cargo_feature_set && !env_var_falsy) || env_var_truthy { skip-RVV }. It should probably fail hard if the value of the environment variable isn't understood.

[jongiddy]

All good points here. In order to make something available, may I suggest that we support the environment variable USE_RISCV_RVV. Adding the architecture makes it clear that this option is only relevant for RISC-V and disambiguates in case another architecture decides to use the acronym RVV.

My suggestions for allowed values are:
(unset), (empty), auto: auto-selection of RVV
off, 0: force RVV disabled
on, 1: force RVV enabled
All other values produce an error.

The on option is useful for people who know they will only run on RVV-enabled processors and want to be certain that they get the performance of RVV. If they happen to build on a non-RVV processor they would prefer the build to fail.

We can leave out the feature for now, hence skipping the question of priority.

[EliahKagan]

This sounds good to me. I will look into whether there is a reasonable way to force RVV on or, if not, to force RVV instructions to be emitted.

Those two things are not quite the same. One problem, which I now recognize, is that I have not been properly distinguishing between checks that take place when zlib-ng is built and checks that take place at runtime unless the build has prevented them from being done. The currently broken detection is in the latter (or at least that), and a fix based on this workaround is being developed in this pull request which is waiting on a separate change but largely done.

Maybe this was already clear based on the information in #200 and zlib-ng/zlib-ng#1705 but, if not, I'm worried that a lot of the discussion here may be based on my misleading descriptions. I am furthermore not at all sure that I have had them straight in my own thinking: why have I so confidently claimed that a build with RVV forced off would be needed for redistribution, even after a runtime check is fixed? I am sorry about the confusion.

In any case, my plan now is to go forward with that approach unless it turns out not to be feasible or to involve excessive complexity, in which I'll go with something along previously discussed lines except (at least initially) still only with an environment variable.

[EliahKagan]

It appears the only documented way to affect RVV in zlib-ng through build configuration is the WITH_RVV variable, defined in the advanced build options table.

It is cmake-only: nothing appears for it in the configure column. This does not make it infeasible to achieve the same result without cmake, but since that goes beyond what the upstream project itself supports, I'm continuing not to attempt it as part of this PR.

The default is ON, and RVV instructions are emitted. Changing it to OFF avoids emitting RVV instructions (and thus also does not perform auto-detection). Forcing RVV instructions to be used even on systems that are detected as not supporting them does not appear to be an upstream feature, and looks like it would require editing the source code itself, beyond just configuration files. For this reason, I am departing from #218 (comment), but still doing some parts of it:

• I am omitting the rvv_off feature. One of its benefits was the accompanying comments, where it was defined in the manifest files. The important information from those comments is reworded for clarity and appears in comments in cmake.rs.

• I am replacing the RVV_OFF environment variable, whose value was not examined, with a RISCV_WITH_RVV environment variable, whose value is examined and used. The name RISCV_WITH_RVV reflects its more direct relationship to cmake -DWITH_RVV=... than the above-proposed USE_RISCV_RVV would've had.

  Setting RISCV_WITH_RVV to ON, OFF, or other Boolean synonyms, causes WITH_RVV to be defined with the value of ON or OFF. Otherwise it is not defined, which is equivalent to defining it with ON assuming nothing is in place to adjust its default value. Typically, one will either refrain from using this, or use the value OFF.

  I am just ignoring empty or otherwise blank, as well as other values that don't easily parse to common truthy or falsy strings, treating any such values as if the environment variable had not been defined. This can be changed if desired.

One other change I've made here, which is subtle and possibly even irrelevant, is that I am matching riscv in the target name rather than riscv64. In principle I think there could be a 32-bit RISC-V machine for which some of this applies (though I don't know if any such Rust targets have std currently).

I've tested that setting RISCV_WITH_RVV to OFF in the environment, with these changes, has the same effect as setting RVV_OFF to any value had achieved, before these changes. That is, with these commands, I verified that RISC_WITH_RVV can be used to suppress RVV and allow the gitoxide gix status and gix clone to work without SIGILL or any other problems:

RISCV_WITH_RVV=OFF cargo build --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"'
RISCV_WITH_RVV=OFF cargo run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --bin=gix -- status
RISCV_WITH_RVV=OFF cargo run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --bin=gix -- clone [email protected]:Byron/gitoxide.git

Edit: I have also tested with:

RISCV_WITH_RVV=OFF cargo nextest run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --all --no-fail-fast
GIX_TEST_IGNORE_ARCHIVES=1 RISCV_WITH_RVV=OFF cargo nextest run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --all --no-fail-fast

All tests passed.

[Byron]

Thanks a lot, particularly for the additional description and with detailed reasoning.

When looking at the docs of this crate one sees that there is nothing hand-written. Hence, there is a strong precedent to also not require RISCV_WITH_RVV to be documented as part of the (nonexistent) docs.

If one feels inspired, I'd definitely value any kind of crate-level documentation that informs about the various backend (CC, Cmake) and the environment variables they may be influenced by.

[EliahKagan]

One small correction to something I reported above. I mentioned also testing with:

RISCV_WITH_RVV=OFF cargo nextest run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --all --no-fail-fast
GIX_TEST_IGNORE_ARCHIVES=1 RISCV_WITH_RVV=OFF cargo nextest run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --all --no-fail-fast

But this is effectively the same as testing with just the first command:

RISCV_WITH_RVV=OFF cargo nextest run --config 'patch.crates-io.libz-sys.path="../libz-sys"' --config 'patch.crates-io.libz-ng-sys.path="../libz-ng-sys"' --all --no-fail-fast

That's because I didn't clean the directories extracted from generated archives in the first test run, so GIX_TEST_IGNORE_ARCHIVES=1 in the second test run had no practical effect. Really this was just rerunning the tests.

Cleaning and running with GIX_TEST_IGNORE_ARCHIVES=1 does produce one failure on the RISC-V test machine, but it is not related to the changes here, nor even to the upgrade to 2.2.2 in #219, because it also happens with max-pure, which I believe does not use crates from this project.

(The failing test is gix-ref-tests::refs packed::iter::performance, but I don't think my test machine is slow enough that the failure would be considered insignificant. I'll report it in gitoxide after a bit of further investigation.)

[EliahKagan]

I've opened GitoxideLabs/gitoxide#1605 for the gitoxide gix-ref-tests::refs packed::iter::performance test failure.