-
-
Notifications
You must be signed in to change notification settings - Fork 368
Description
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:
Lines 23 to 24 in 473fe52
## Make it possible to compile to the `wasm32-unknown-unknown` target. | |
wasm = ["dep:getrandom"] |
Line 47 in 473fe52
getrandom = { version = "0.2.8", optional = true, default-features = false, features = ["js"] } |
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 rust-random/getrandom#504, which notes:
This PR removes
linux_disable_fallback
,rdrand
,js
,test-in-browser
, andcustom
crate features. As their replacement two new configuration flags are introduced:getrandom_browser_test
andgetrandom_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 rust-random/getrandom#675, particularly at rust-random/getrandom#675 (comment), 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
:
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 briansmith/ring#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 #1816 (comment).
The point in the earlier comment #1816 (comment) 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 theerrno
crate doesn't support that target; the same thing happens when--features wasm
is added that command. The default features ofgix-diff
areblob
andindex
; both depend onerrno
. cargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features
works, as doescargo build -p gix-diff --target wasm32-unknown-unknown --no-default-features --features wasm
, as does adding theserde
feature (replacingwasm
withserde,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
.