gitoxide icon indicating copy to clipboard operation
gitoxide copied to clipboard

Upgrading `getrandom`

Open EliahKagan opened this issue 4 months ago • 4 comments

Summary 💡

The current version of getrandom is 0.3.3. One use in gitoxide of getrandom is in gix-diff, where it is a direct dependency when the wasm feature is enabled, currently requiring a 0.2.* version:

https://github.com/GitoxideLabs/gitoxide/blob/473fe522e84569f77bf38294a412f0d13fa54d63/gix-diff/Cargo.toml#L23-L24

https://github.com/GitoxideLabs/gitoxide/blob/473fe522e84569f77bf38294a412f0d13fa54d63/gix-diff/Cargo.toml#L47

The error

When Dependabot attempts to upgrade getrandom to a 0.3.* version, it fails:

Dependabot failed to update your dependencies because there was an error resolving your Rust dependency files.

Dependabot encountered the following error:

Updating crates.io index
error: failed to select a version for `getrandom`.
    ... required by package `gix-diff v0.53.0 (dependabot_tmp_dir/gix-diff)`
    ... which satisfies path dependency `gix-diff` (locked to 0.53.0) of package `gix-pack v0.60.0 (dependabot_tmp_dir/gix-pack)`
    ... which satisfies path dependency `gix-pack` (locked to 0.60.0) of package `gix-odb v0.70.0 (dependabot_tmp_dir/gix-odb)`
    ... which satisfies path dependency `gix-odb` (locked to 0.70.0) of package `gix-object v0.50.0 (dependabot_tmp_dir/gix-object)`
    ... which satisfies path dependency `gix-object` (locked to 0.50.0) of package `gix-ref v0.53.0 (dependabot_tmp_dir/gix-ref)`
    ... which satisfies path dependency `gix-ref` (locked to 0.53.0) of package `gix-discover v0.41.0 (dependabot_tmp_dir/gix-discover)`
    ... which satisfies path dependency `gix-discover` (locked to 0.41.0) of package `gix-testtools v0.17.0 (dependabot_tmp_dir/tests/tools)`
    ... which satisfies path dependency `gix-testtools` (locked to 0.17.0) of package `gix-path v0.10.19 (dependabot_tmp_dir/gix-path)`
    ... which satisfies path dependency `gix-path` (locked to 0.10.19) of package `gix-fs v0.16.0 (dependabot_tmp_dir/gix-fs)`
    ... which satisfies path dependency `gix-fs` (locked to 0.16.0) of package `gix-tempfile v18.0.0 (dependabot_tmp_dir/gix-tempfile)`
    ... which satisfies path dependency `gix-tempfile` (locked to 18.0.0) of package `gix-lock v18.0.0 (dependabot_tmp_dir/gix-lock)`
versions that meet the requirements `>=0.2.15, <=0.3.3` (locked to 0.3.2) are: 0.3.2

package `gix-diff` depends on `getrandom` with feature `js` but `getrandom` does not have that feature.
 package `getrandom` does have feature `std`


failed to select a version for `getrandom` which could resolve this conflict

This is not a Dependabot problem. That is, the error and the condition it reports are not specific to Dependabot, and the underlying limitation is not something we can fix by using Dependabot differently or substituting a different update procedure.

  • The cause is an actual incompatibility, arising from the removal of a feature we are depending on, as described below. (Although we can't fix the underlying incompatibility by using Dependabot differently, there is a way to make Dependabot not attempt the upgrade that causes the error, which I note below.)
  • Dependabot seems to be able to upgrade other packages without problems even when this error occurs. In particular, Dependabot encountered this error in the run associated with #2090, and that PR was generated without problems and seems to have had no trouble upgrading crates other than getrandom.

The cause

gix-diff specifies the js feature of getrandom. The js feature was removed as of getrandom 0.3.0. The change was made in https://github.com/rust-random/getrandom/pull/504, which notes:

This PR removes linux_disable_fallback, rdrand, js, test-in-browser, and custom crate features. As their replacement two new configuration flags are introduced: getrandom_browser_test and getrandom_backend. The latter can have the following values: custom, rdrand, linux_getrandom, wasm_js, esp_idf.

Further details and context are in the changelog and PR description there. See also the WebAssembly support section of the documentation for getrandom 0.3.3.

My guess is that we would not encounter problems making gix-diff compatible with that, but I haven't tried. One thing I might want to do first is to figure out if the current WASM tests are sufficient to reveal a breakage.

Edit: I have figured out some things about this, as detailed below.

Motivation 🔦

General motivations

It would be nice to let users of the gix-diff crate use the newer version of getrandom, which I believe has various compatibility and robustness improvements, even separate from this change in its features. Some new functionality has been backported to 0.2.* versions released since 0.3.* came out, but I don't think all is.

More broadly, I think it's generally a good idea to use newer versions of dependencies in the absence of a particular reason not to, unless the older version of the dependency is as well supported as the newer version.

Specific motivation - are we using js correctly?

The bigger motivation is that I am not sure our use of the js feature is correct. Versions of getrandom that offer the js feature include this in their documentation of it:

//! This feature should only be enabled for binary, test, or benchmark crates.
//! Library crates should generally not enable this feature, leaving such a
//! decision to *users* of their library. Also, libraries should not introduce
//! their own `js` features *just* to enable `getrandom`'s `js` feature.

Contrary to that, we currently enable the js feature unconditionally for the getrandom dependency of gix-diff. That dependency is itself conditional, being present only when the wasm feature is enabled. The js feature is also implemented in getrandom in such a way that it only has an effect when wasm32-unknown-unknown is the target, if I understand correctly. But I am not sure that wasm32-unknown-unknown being the target implies that JavaScript is available, or that JavaScript ought to be used if available, or that we know this better than users of gix-diff would know it. Maybe this is a case where it's reasonable for a library crate to enable the js feature, but it's not clear to me that this is so.

It looks like one of the reasons for removing the feature and using configuration flags instead is to keep library crates from imposing the associated assumptions on crates that use them, and that this has been a problem. The discussion in https://github.com/rust-random/getrandom/issues/675, particularly at https://github.com/rust-random/getrandom/issues/675#issuecomment-2941738862, seems relevant.

Noisy Dependabot runs

It's useful to be able to look at Dependabot logs to see what happened. The presence of the prominent error is potentially distracting. This is the case even if one wishes only to check the status and does not look at the actual log--it looks like the update operation has failed, when actually it has succeeded other than for getrandom:

Screenshot of the dependency graph feature on the Dependabot tab showing a recent version update job, indicating a 'Dependabot can't resolve your Rust dependency files' error

This could also be solved by editing dependabot.yml to only accept patch version updates to getrandom. This would probably be okay to do until the getrandom dependency of Dependabot is updated, though I worry that there might be some circumstance where some other getrandom dependency wouldn't be upgraded.

Via other crates

We already have other transitive dependencies on getrandom 0.3.*, through tempfile. (There is also a build-only one, through xz2's cc build dependency, via jobserver.) Nonetheless, at this time, upgrading the gix-diff dependency on getrandom from 0.2.* to 0.3.* might often not decrease build size, because we also have a separate transitive dependency on getrandom 0.2.*, through ring.

If upgrading it as a dependency of gix-diff is easy, then it's worth doing even without the benefit of improving deduplication. If it's not easy, then that might make it worthwhile to stay back at 0.2.*. But if it turns out to be hard in a way that overlaps with the difficulties upgrading it in ring, then maybe work on that in ring could show the way for doing it here, or vice versa.

My guess is that it should be feasible to do here because we don't yet have robust support for wasm32-unknown-unknown and because it's typically okay to introduce breaking changes in gix-diff (so long as SemVer reflects them), since the crate is not yet stabilized. But I mention ring anyway just in case there are any insights to be had. The relevant ring issue is https://github.com/briansmith/ring/issues/2341.

Near future use in gix-utils

For #1816, gix-utils will probably get getrandom as a direct dependency, to use to seed thread-local fastrand::Rng instances, for the technique described in https://github.com/GitoxideLabs/gitoxide/issues/1816#issuecomment-2620815045.

The point in the earlier comment https://github.com/GitoxideLabs/gitoxide/issues/1816#issuecomment-2618066550 about getrandom already being in the dependency tree is correct regardless of whether 0.2.* or 0.3.* is used, since both are already in the dependency tree, even separately from gix-diff, as described above. However, I'd prefer to use the newer version when adding it as a new dependency. I also want to avoid introducing any new wrong usage, or new occurrences of existing wrong usage, when doing so. Thus, I'd be somewhat reluctant to depend on the js feature when introducing getrandom as a dependency of gix-utils.

That's relevant because if gix-diff and git-utils can use the same version of getrandom, and either declare them the same way or declare them differently in way that is relevant to differences in how they use getrandom, then that's ideal.


Update

Target compatibility and test coverage

None of the commands we run on CI seem to attempt directly to exercise gix-diff for any WASM targets. (Somewhat related: #1988.) However, experimenting locally:

  • Running cargo build -p gix-diff --target wasm32-unknown-unknown fails because the errno crate doesn't support that target; the same thing happens when --features wasm is added that command. The default features of gix-diff are blob and index; both depend on errno.
  • cargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features works, as does cargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features --features wasm, as does adding the serde feature (replacing wasm with serde,wasm).

Having observed that, I then tried this change to gix-diff/Cargo.toml:

@@ -44,7 +44,7 @@ gix-traverse = { version = "^0.47.0", path = "../gix-traverse", optional = true
 thiserror = "2.0.0"
 imara-diff = { version = "0.1.8", optional = true }
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
-getrandom = { version = "0.2.8", optional = true, default-features = false, features = ["js"] }
+getrandom = { version = "0.2.8", optional = true, default-features = false }
 bstr = { version = "1.12.0", default-features = false }

 document-features = { version = "0.2.0", optional = true }

Note that I didn't change the version; I'm just checking to see whether we can observe the effect of not specifying the js feature of getrandom. Attempting to build that fails with:

ek in 🌐 catenary in gitoxide on  main [!] is 📦 v0.45.0 via 🦀 v1.88.0
❯ cargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features --features wasm
   Compiling thiserror v2.0.12
   Compiling getrandom v0.2.15
error: the wasm*-unknown-unknown targets are not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support
   --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.2.15/src/lib.rs:342:9
    |
342 | /         compile_error!("the wasm*-unknown-unknown targets are not supported by \
343 | |                         default, you may need to enable the \"js\" feature. \
344 | |                         For more information see: \
345 | |                         https://docs.rs/getrandom/#webassembly-support");
    | |________________________________________________________________________^

   Compiling gix-date v0.10.3 (/home/ek/repos/gitoxide/gix-date)
   Compiling gix-validate v0.10.0 (/home/ek/repos/gitoxide/gix-validate)
   Compiling gix-hash v0.19.0 (/home/ek/repos/gitoxide/gix-hash)
error[E0433]: failed to resolve: use of unresolved module or unlinked crate `imp`
   --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.2.15/src/lib.rs:398:9
    |
398 |         imp::getrandom_inner(dest)?;
    |         ^^^ use of unresolved module or unlinked crate `imp`
    |
    = help: if you wanted to use a crate named `imp`, use `cargo add imp` to add it to your `Cargo.toml`

For more information about this error, try `rustc --explain E0433`.
error: could not compile `getrandom` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

That is encouraging, because it suggests that we can extend the WASM CI job--which only builds, and does not attempt to run unit tests--to check for regressions in the ability to build gix-diff for wasm32-unknown-unknown, without having to fundamentally change the way those jobs work. (Eventually we will want to run tests, of course, but it's nice that it might not need to block a fix here.)

Looking slightly ahead and seeing what happens when I upgrade it:

diff --git a/gix-diff/Cargo.toml b/gix-diff/Cargo.toml
index a564ecf23..e3e23fadb 100644
--- a/gix-diff/Cargo.toml
+++ b/gix-diff/Cargo.toml
@@ -44,7 +44,7 @@ gix-traverse = { version = "^0.47.0", path = "../gix-traverse", optional = true
 thiserror = "2.0.0"
 imara-diff = { version = "0.1.8", optional = true }
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
-getrandom = { version = "0.2.8", optional = true, default-features = false }
+getrandom = { version = "0.3.3", optional = true, default-features = false }
 bstr = { version = "1.12.0", default-features = false }

 document-features = { version = "0.2.0", optional = true }

The errors help point the way for adapting to the changes, in a way that is consistent with, and that explicitly references, that documentation:

ek in 🌐 catenary in gitoxide on  main [!+] is 📦 v0.45.0 via 🦀 v1.88.0
❯ cargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features --features wasm
   Compiling thiserror v2.0.12
   Compiling getrandom v0.3.3
error: The wasm32-unknown-unknown targets are not supported by default; you may need to enable the "wasm_js" configuration flag. Note that enabling the `wasm_js` feature flag alone is insufficient. For more information see: https://docs.rs/getrandom/0.3.3/#webassembly-support
   --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.3.3/src/backends.rs:168:9
    |
168 | /         compile_error!(concat!(
169 | |             "The wasm32-unknown-unknown targets are not supported by default; \
170 | |             you may need to enable the \"wasm_js\" configuration flag. Note \
171 | |             that enabling the `wasm_js` feature flag alone is insufficient. \
172 | |             For more information see: \
173 | |             https://docs.rs/getrandom/", env!("CARGO_PKG_VERSION"), "/#webassembly-support"
174 | |         ));
    | |__________^

error[E0425]: cannot find function `fill_inner` in module `backends`
  --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.3.3/src/lib.rs:99:19
   |
99 |         backends::fill_inner(dest)?;
   |                   ^^^^^^^^^^ not found in `backends`

error[E0425]: cannot find function `inner_u32` in module `backends`
   --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.3.3/src/lib.rs:128:15
    |
128 |     backends::inner_u32()
    |               ^^^^^^^^^ not found in `backends`
    |
help: consider importing this function
    |
33  + use crate::util::inner_u32;
    |
help: if you import `inner_u32`, refer to it directly
    |
128 -     backends::inner_u32()
128 +     inner_u32()
    |

error[E0425]: cannot find function `inner_u64` in module `backends`
   --> /home/ek/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/getrandom-0.3.3/src/lib.rs:142:15
    |
142 |     backends::inner_u64()
    |               ^^^^^^^^^ not found in `backends`
    |
help: consider importing this function
    |
33  + use crate::util::inner_u64;
    |
help: if you import `inner_u64`, refer to it directly
    |
142 -     backends::inner_u64()
142 +     inner_u64()
    |

   Compiling gix-validate v0.10.0 (/home/ek/repos/gitoxide/gix-validate)
   Compiling gix-hash v0.19.0 (/home/ek/repos/gitoxide/gix-hash)
   Compiling gix-date v0.10.3 (/home/ek/repos/gitoxide/gix-date)
For more information about this error, try `rustc --explain E0425`.
error: could not compile `getrandom` (lib) due to 4 previous errors
warning: build failed, waiting for other jobs to finish...

The wasm-js feature and configuration setting could be enabled explicitly on CI, in a new job that tests gix-diff, and maybe that is all that would have to change for CI.

But I think there is a design decision that has to be made about what to enable by default, and maybe also a need to document what users need to do in order to build gix-diff on wasm32-unknown-unknown.

EliahKagan avatar Aug 02 '25 03:08 EliahKagan

Thanks a lot for investigating!

I didn't understand why gix-diff wouldn't be allowed to setup getrandom v0.3 such that the right feature flags are set. Is this what they still discourage? If so, getting CI to work wouldn't be too trivial as we can't set features of dependencies directly. Instead, gix-diff would have to provide another feature to configure getrandom for wasm, which feels like redundant to the existing wasm feature.

Thus far, all users needed to do to build on wasm is to enable the wasm feature in gix-diff, and I'd think that this is still feasible here. gix-diff doesn't unconditionally do that, so I think it's within the realm of what's documented in getrandom, crate-users are in control of it and if they want to support their target differently, they won't use wasm but configure getrandom directly.

Does that make sense or am I missing something?

Byron avatar Aug 02 '25 05:08 Byron

I didn't understand why gix-diff wouldn't be allowed to setup getrandom v0.3 such that the right feature flags are set. Is this what they still discourage?

I'm pretty sure we can make it work at least as well with 0.3.* as with 0.2.*, but I'm not sure I know how to do it properly.

I'm not sure I entirely understand the situation, but my understanding is that, in 0.3.*, there are two things to enable instead of one:

  1. The wasm-js feature, which sometimes pulls in heavy dependencies. So it is recommended not to include it unless it is known to be needed or those dependencies are already being pulled in.
  2. The wasm-js cargo configuration setting (called a configuration flag in the getrandom documentation, and maybe that's the preferred phrase, I'm not sure), which is separate from the wasm-js feature though they are named the same and often enabled together. Recognition for this (I think nonstandard) configuration setting was added in getrandom, partly to work around the absence of separate target names for wasm32-unknown-unknown without a JavaScript runtime and wasm32-unknown-unknown with a JavaScript runtime.

If I understand correctly, the idea is that:

  • The author of a binary crate (or test suite) is usually in a position to know whether a built binary (or test executable) can reasonably rely on a JavaScript runtime being present--and maybe also whether it can reasonably make use of it, such as if a JavaScript runtime does not have a complete standard library, or if its PRNG-related functionality is not of sufficient quality. In contrast, library authors are most often not in a position to know these things confidently.
  • But a library crate can enable a feature of another library crate, which led to the situation where library crates would enable the js feature of getrandom when they usually shouldn't. Thus, the functionality was split up in 0.3.* to include a configuration setting, which library crates cannot impose on binary crates that use them.

So I think that, if we move to 0.3.*, then however we go about it, developers of crates that use this crate will have to know that the wasm-js configuration setting needs to be enabled. We can't turn that on ourselves in a library crate for downstream binary crates.

Thus far, all users needed to do to build on wasm is to enable the wasm feature in gix-diff, and I'd think that this is still feasible here. gix-diff doesn't unconditionally do that, so I think it's within the realm of what's documented in getrandom, crate-users are in control of it and if they want to support their target differently, they won't use wasm but configure getrandom directly.

Yes, that part makes sense to me. For the wasm-js feature of getrandom:

  • I can imagine a case where it, too, could in principle not be wanted. I think it is possible to provide a custom implementation for getrandom to use. If someone builds their project to run in a wasm32-unknown-unknown environment without a JavaScript runtime or where there is a reason not to use it for this purpose, then maybe they could provide a different implementation. Unless I'm mistaken about this possibility, it makes it hard to be confident that we know there's nobody who would ever want to attempt to build this on wasm32-unknown-unknown without the wasm-js feature.
  • But I think that may not be a problem, and that it is still probably fine to have gix-diff enable that when its wasm feature is enabled. The main point of the wasm feature of gix-diff seems to be to request dependencies or features that are typically required for gix-diff to be usable on WASM, after all. It is already possible to build gix-diff on wasm32-unknown-unknown without the wasm feature.

I had wondered, before, why we had a wasm feature in gix-diff, rather than going by the target. My guess is that this is to allow greater control of whether functionality that is usually but not always wanted to WASM targets is to be used. I think the above-quoted piece of your reply indicates that this is the case, and maybe even that it is specific to getrandom.

So, in that case, having a getrandom dependency with the wasm-js feature as a dependency of gix-diff with the wasm feature is probably fine. Though binary crates and test suites--including those outside of the gitoxide project--as well our CI build checks for WASM, would still need to enable that configuration setting in their cargo commands.

My main other worry is that I'm not clear on what is using getrandom when it is a dependency of gix-diff, and what happens if it is absent. After all, building for wasm32-unknown-unknown does succeed with the wasm feature and thus without getrandom. (This doesn't just omit the js feature. It omits getrandom, as far as I can tell.)

My guess is that something is degrading to using a lower quality random number generator in its absence. (Or is it a separately provided getrandom dependency, somehow?) I would want to understand that before forming a confident opinion about what specific change should be made when adapting to getrandom 0.3.*, and also whether it's important or not to document anything associated with that.

I originally opened this issue after noticing the error in Dependabot output, so that this wouldn't be forgotten, but I wouldn't consider this a top priority. Describing the issue ended up being more involved than I expected, which is not a problem, but I haven't looked into everything thoroughly.

EliahKagan avatar Aug 02 '25 06:08 EliahKagan

I've opened #2093. This accompanying PR may or may not ever be worth merging, I am not sure, but it does seem to help illustrate some of the issues here.

It also helps identify a few areas I was mistaken above: both the feature and Rust configuration flag are called wasm_js, not wasm-js; it is a Rust configuration flag, not a Cargo flag; and the existing CI wasm job actually does detect when the wasm feature of gix-diff is broken, because it builds gix-pack, which depends on gix-diff (though I think may be best to try to build both explicitly, and #2093 does so).

One thing you said above, I think I didn't really reply to clearly here:

gix-diff would have to provide another feature to configure getrandom for wasm, which feels like redundant to the existing wasm feature.

But I think #2093 clarifies why I don't think this can be done with a feature, whether the existing wasm feature of gix-date or otherwise: the JavaScript backend of getrandom on wasm32-unknown-unknown can only be enabled by putting something in RUSTFLAGS or equivalent, ever since getrandom 0.3.

EliahKagan avatar Aug 03 '25 01:08 EliahKagan

But I think #2093 clarifies why I don't think this can be done with a feature, whether the existing wasm feature of gix-date or otherwise: the JavaScript backend of getrandom on wasm32-unknown-unknown can only be enabled by putting something in RUSTFLAGS or equivalent, ever since getrandom 0.3.

I see now! I was always thinking of Cargo features exclusively even though it's now a Rustc flag, to emphasise that it really will be the producer of the binary that has to enable it.

Byron avatar Aug 03 '25 08:08 Byron