Upgrade the getrandom dependency of gix-diff

[EliahKagan]

Would fix #2092

I don't know if I'll manage to make this work. But even if not, maybe showing my attempts in this PR will make the situation in #2092 clearer.

[EliahKagan]

I don't believe we can cause enabling the wasm feature of gix-diff to achieve this effect, nor otherwise achieve this effect for users of gix-diff in a way that spares them the need to do an extra step.

That is, whoever does the build must do something to achieve the effect of passing --cfg getrandom_backend="wasm_js" to rustc, in addition to whatever features are enabled. This can be done in a .cargo/config.toml file, but having such a configuration here will not affect other projects that are not hosted in this repository and that host gix-diff.

I believe placing the onus on the downstream developer, rather than library crate developers, to enable the wasm_js backend, is one of the reasons for the breaking changes in getrandom 0.3. Reasons for this may include security--the JavaScript backend may not always be adequately secure, if I understand correctly. If all possible uses of getrandom through gix-diff do not require unpredictable random numbers, then security may not be an issue. I suspect that is the case, but I don't know for sure. Anyway, I still don't know of a way to make it work like it did in 0.2.

Options include:

1. Documenting that this is so and going ahead with the upgrade, or
2. Holding off on upgrading (or even planning not to upgrade) getrandom.

There may be other options I haven't thought of, and there may be gaps or inaccuracies in my understanding.

[Byron]

Additional documentation might help - users now have to know how to configure getrandom if they want to build for these targets, and if I am not mistaken the compiler error will also be quite specific about what has to be done.

Thus, maybe additional documentation in a plumbing crate of gitoxide wouldn't be read anyway.

[EliahKagan]

The gix-diff docstring in lib.rs is not visible when looking at the diff module of gix (diff pulls in the contents of gix-diff with pub use gix_diff::*; but also has its own items). So maybe often not? But people would still search for such documentation. I can try to document it in some way, if we go forward with the approach currently implemented here.

[Byron]

If you find a good spot for documentation, I think it's absolutely fair to add it. From what I wrote below it seems though that getrandom has nothing to do with this crate, they are only tangentially related.

[EliahKagan]

See #2093 (comment). If this is to be merged in something like its current form, then in addition to documenting the actions developers using gix-diff will have to take to set the Rust flag to select the optional getrandom backend, I think I would want to rebase this to make one of the commits that modifies gix-diff/Cargo.toml conventional.

Per #2092 (comment), even if this should just be closed, it's my hope that helps make the situation clearer than before.

[Byron]

Maybe this could be as 'easy' as going with this - it works like it worked before.

Without the wasm_js Cargo feature I suppose it wouldn't work, but would it work without that feature and without the RUSTFLAGS setting? If that was the case, that might be preferred as it leaves everything to the caller.

Otherwise, I'd be OK merging exactly what we have here.

[EliahKagan]

Maybe this could be as 'easy' as going with this - it works like it worked before.

I don't view this as working the same as it did before, because:

• Without the changes in this PR, whoever builds something with gix-diff and targets wasm32-unknown-unknown need only enable the wasm feature of gix-diff to get the getrandom dependency to be present and attempt to use the JavaScript backend.
• In contrast, with the changes in this PR, whoever does that must both enable the wasm feature of gix-diff and also cause --cfg getrandom_backend="wasm_js" to be included in rustflags.

However, it is true that I've avoided changing how it works as much as possible (or at least as much as I know how). If you think that's sufficient, I'd be happy to merge this. It seems to me that something should probably also be done to indicate the change. To elaborate on my previous thinking:

• This is technically breaking. Most users of gix-diff won't be broken, simply because most aren't on wasm32-unknown-unknown, but perhaps a few are, so we should probably make sure this doesn't just bump the patch version number. Although we rarely use it in this repository, I recall that chore!: was recognized properly by csr when used fairly recently. I could amend and reword one of the messages to include that. (change!: or feat!: would also be reasonable.)
• Documenting it probably doesn't need to be done in this PR, though, since if a release ends up being made without a documentation change (and users don't see the changelog entry), then the error messages will still guide the way to some extent: they describe the need for the configuration flag and include the URL of the getrandom documentation that covers it.

Without the wasm_js Cargo feature I suppose it wouldn't work, but would it work without that feature and without the RUSTFLAGS setting? If that was the case, that might be preferred as it leaves everything to the caller.

Omitting both--like omitting one or the other--will break the build on wasm32-unknown-unknown, unless the getrandom dependency is removed (or removed for that target). However, if the dependency is removed, then I am not sure what the wasm feature of gix-diff would represent.

[Byron]

I noticed something:

❯ cargo tree -i getrandom -p gix-diff
getrandom v0.3.2
└── tempfile v3.20.0
    └── gix-tempfile v18.0.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-tempfile)
        ├── gix-diff v0.53.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-diff)
        └── gix-lock v18.0.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-lock)
            └── gix-index v0.41.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-index)
                ├── gix-diff v0.53.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-diff)
                └── gix-worktree v0.42.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-worktree)
                    └── gix-diff v0.53.0 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-diff)

I wonder why gix-diff got the wasm feature just to configure getrandom. Why not any of the other crates in the dependency chain?

It feels like I tried to make something work, and it did work because it was only a Cargo feature. Now getrandom explicitly discourages it, which will probably tell us as much as: don't offer wasm here.

If this makes sense, this should be a breaking change to indicate the dependency gets removed, and that applications need to configure it entirely themselves. It's a hurdle, but there seems to be no way around it, while also I think what we have now is obviously confusing to us, and more like a hack to make it easier for downstream. But the motivation for the hack, to make it easy, can't be fulfilled anymore.

[EliahKagan]

Thanks--I had sort of assumed that we did need getrandom, but I did wonder what the reason was, given that there are no direct references to getrandom--nor to wasm--within the code of gix-diff itself. This relates to something I mentioned in #2092 (comment) but then never followed up on any further:

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 had meant to say:

After all, building for wasm32-unknown-unknown does succeed without the wasm feature and thus without getrandom.

Is the wasm feature of gix-diff, and its dependency on getrandom, just for tempfile? If so, what bad things happen when the wasm feature is not enabled on a WASM target? Do operations that involve generating names for temporary files fail at runtime when attempted, even after a successful build? Is temporary file name generation more brittle?

If this makes sense, this should be a breaking change to indicate the dependency gets removed, and that applications need to configure it entirely themselves.

I don't think I really understand how it has to be configured. If I remove the wasm feature of gix-diff and its associated direct dependency on tempfile, then the current WASM jobs on CI will succeed just by no longer specifying the removed wasm feature.

So I will need some way of testing for the functionality that requires configuring a JavaScript backend for getrandom on some WASM targets. Of course, I will also need that if I am to write any useful documentation about what people are supposed to do.

---

Taking a look at:

C:\Users\ek\source\repos\gitoxide\gix-diff [main ≡]> git grep -Fn tempfile -- '*.rs'
src/blob/pipeline.rs:228:    /// As these files are ultimately named tempfiles, they will be leaked unless the [gix_tempfile] is configured with
src/blob/pipeline.rs:425:                                        gix_tempfile::new(
src/blob/pipeline.rs:427:                                            gix_tempfile::ContainingDirectory::Exists,
src/blob/pipeline.rs:428:                                            gix_tempfile::AutoRemove::Tempfile,
src/blob/platform.rs:310:        #[error("Could not write content of '{rela_path}' to tempfile for passing to external diff command")]
src/blob/platform.rs:320:        /// Possibly a tempfile to be removed after the run, or `None` if there is no old version.
src/blob/platform.rs:321:        pub(crate) old: Option<gix_tempfile::Handle<gix_tempfile::handle::Closed>>,
src/blob/platform.rs:322:        /// Possibly a tempfile to be removed after the run, or `None` if there is no new version.
src/blob/platform.rs:323:        pub(crate) new: Option<gix_tempfile::Handle<gix_tempfile::handle::Closed>>,
src/blob/platform.rs:431:    // TODO: fix this - the diff shouldn't fail if binary (or large) files are used, just copy them into tempfiles.
src/blob/platform.rs:442:        ) -> Result<Option<gix_tempfile::Handle<gix_tempfile::handle::Closed>>, prepare_diff_command::Error> {
src/blob/platform.rs:449:                    let mut tmp = gix_tempfile::new(
src/blob/platform.rs:451:                        gix_tempfile::ContainingDirectory::Exists,
src/blob/platform.rs:452:                        gix_tempfile::AutoRemove::Tempfile,
tests/diff/blob/pipeline.rs:21:            let tmp = gix_testtools::tempfile::TempDir::new()?;
tests/diff/blob/pipeline.rs:98:        let tmp = gix_testtools::tempfile::TempDir::new()?;
tests/diff/blob/pipeline.rs:153:        let tmp = gix_testtools::tempfile::TempDir::new()?;
tests/diff/blob/pipeline.rs:236:        let tmp = gix_testtools::tempfile::TempDir::new()?;
tests/diff/blob/pipeline.rs:306:        let tmp = gix_testtools::tempfile::TempDir::new()?;
tests/diff/blob/pipeline.rs:440:        let tmp = gix_testtools::tempfile::TempDir::new()?;

gix-diff does not use tempfile directly, though it almost uses it directly in its test suite, which uses tempfile as republished by gix-testtools. I think the far more important usage by gix-diff of tempfile is through gix_tempfile.

gix-tempfile itself depends on tempfile without omitting default features:

gitoxide/gix-tempfile/Cargo.toml

Line 38 in 202bc6d

tempfile = "3.20.0"

That transitively depends on getrandom on most targets. I say this based on this dependency and the feature list of tempfile.

[Byron]

Is the wasm feature of gix-diff, and its dependency on getrandom, just for tempfile? If so, what bad things happen when the wasm feature is not enabled on a WASM target? Do operations that involve generating names for temporary files fail at runtime when attempted, even after a successful build? Is temporary file name generation more brittle?

I don't know actually, I'd expect it to fail building entirely. It's worth noting that getrandom has no mention whatsoever in gix-diff and really is only used in gix-tempfile

I don't think I really understand how it has to be configured. If I remove the wasm feature of gix-diff and its associated direct dependency on tempfile, then the current WASM jobs on CI will succeed just by no longer specifying the removed wasm feature.

That's great, I wasn't even aware that it could build without the wasm feature and always thought it was just providing it for convenience to users. In that case, I definitely think it's OK to remove wasm and leave the configuration of getrandom to the downstream entirely, particularly now that v0.3 requires setting rustc-flags.
This definitely would be a breaking change, but one worth making.

If I had better memory I'd probably know why wasm was added in the first place, but from this investigation I don't see why it would have been more than a convenience.