update rand crate to 0.8

[g-k]

Fix: #2888

make test passes on aarch64-apple-darwin rust v1.56.1

[sylvestre]

hello @g-k nice to see you here ;) & thanks for the patch

Looks like clippy is complaining on it:
https://github.com/uutils/coreutils/pull/2893/files

[g-k]

Hi Sylvestre, good to see you here too!

Updates:

• fixed another rng.gen_range call to use a range in a632f3a
• filed Please consider undeprecating rngs::adapter::ReadRng<File> rust-random/rand#1212 for the clippy warning, since it's "Deprecated since 0.8.4: removal due to lack of usage"
• on i686 and x86_64 still seeing this error:

error[E0599]: the method `gen` exists for mutable reference `&mut G`, but its trait bounds were not satisfied
Error:    --> src/uu/factor/src/factor.rs:281:11
    |
281 |         g.gen()
    |           ^^^ method cannot be called on `&mut G` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `G: rand::RngCore`
            which is required by `G: Rng`
            `&mut G: rand::RngCore`
            which is required by `&mut G: Rng`
    = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
    |
8   | use rand::Rng;
    |

I'll try to address that on a failing architecture.

[tertsdiepraam]

I was going through our issues and found some related issues:

• Implementation of shuf --random-source #2528
• Deprecation of ReadRng rust-random/rand#1152

[g-k]

Aha, thanks Terts! I closed the issue I made in rust-random/rand and I'll copy the relevant code over.

How closely is this project trying to match GNU coreutils? I didn't see that documented anywhere, but #2528 makes a good case for switching to --random-seed.

[tertsdiepraam]

How closely is this project trying to match GNU coreutils?

In general, we try to stay as close as possible, at least for now. In practice, it also depends on how much a feature is used. I like the solution you've come up with!

[sylvestre]

To me, the priority is to have rust/coreutils as a dropped in replacement but I don't mind adding stuff (or adding aliases) when it makes sense

[g-k]

Cool! That makes sense. I'll keep this PR focused on updating rand.

Rebased on the clap 3 upgrade and:

• on aarch64-apple-darwin / rust v1.58.1 (db9d1b20b 2022-01-20) clippy and test pass
• on stable-x86_64-unknown-linux-gnu / rustc 1.58.1 (db9d1b20b 2022-01-20) clippy passes and different tests fail (seems weird and unrelated to the rand update so maybe CI will reveal something):

failures:

---- test_du::test_du_h_flag_empty_file stdout ----
current_directory_resolved: 
run: /home/gguthe/coreutils/target/debug/coreutils du -h empty.txt
thread 'test_du::test_du_h_flag_empty_file' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<"1.0K\tempty.txt\n"
>"0\tempty.txt\n"

', tests/common/util.rs:233:9

---- test_du::test_du_threshold stdout ----
current_directory_resolved: 
run: /home/gguthe/coreutils/target/debug/coreutils du --threshold=10K
thread 'test_du::test_du_threshold' panicked at ''11	.
' does not contain 'links'', tests/common/util.rs:386:9

---- test_du::test_du_time stdout ----
current_directory_resolved: 
run: /home/gguthe/coreutils/target/debug/coreutils touch -a -t 201505150000 date_test
run: /home/gguthe/coreutils/target/debug/coreutils touch -m -t 201606160000 date_test
run: /home/gguthe/coreutils/target/debug/coreutils du --time date_test
thread 'test_du::test_du_time' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<"1\t2016-06-16 00:00\tdate_test\n"
>"0\t2016-06-16 00:00\tdate_test\n"

', tests/common/util.rs:233:9

---- test_ls::test_ls_long_total_size stdout ----
current_directory_resolved: 
touch: /tmp/.tmp9T0aPj/test-long
write(append): /tmp/.tmp9T0aPj/test-long
touch: /tmp/.tmp9T0aPj/test-long2
write(append): /tmp/.tmp9T0aPj/test-long2
run: /home/gguthe/coreutils/target/debug/coreutils ls -l
thread 'test_ls::test_ls_long_total_size' panicked at ''total 2
-rw-rw-r-- 1 gguthe gguthe    1 Jan 20 21:07 test-long
-rw-rw-r-- 1 gguthe gguthe    1 Jan 20 21:07 test-long2
' does not contain 'total 8'', tests/common/util.rs:386:9


failures:
    test_du::test_du_h_flag_empty_file
    test_du::test_du_threshold
    test_du::test_du_time
    test_ls::test_ls_long_total_size

test result: FAILED. 1444 passed; 4 failed; 27 ignored; 0 measured; 0 filtered out; finished in 28.86s

edit: fixed import and cspell test failures (hadn't cleaned the shuf cache)

[g-k]

Alternatively, ignore the spelling error on the line where it's imported (if that's supported) and rename it to implementations or another word that doesn't get flagged for subsequent uses.

[tertsdiepraam]

I think this is fine, impl (and hence impls) are such common words in Rust-related writing that it belongs on the jargon list. Might be better suited to put under abbreviations though?

[g-k]

Good call, done!

[tertsdiepraam]

Oh interesting hahaha, I was looking at a different abbreviations list (in jargon.wordlist.txt) but this is fine! Thanks!

[g-k]

Repro'd the earlier E0599 error on aarch64-apple-darwin and updated factor's quickcheck dep to a compatible rand version to fix it. Top-level cargo test passes now but the following factor tests fail with 'attempt to multiply with overflow' panics now:

failures:
    miller_rabin::tests::composites
    numeric::gcd::tests::linearity
    numeric::gcd::tests::multiplicative
    numeric::gcd::tests::scalar_multiplication
    numeric::modular_inverse::tests::random_values_u32
    numeric::modular_inverse::tests::random_values_u64

test result: FAILED. 33 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.06s

edit: Looks like it's #1559 and related to BurntSushi/quickcheck@71d743a

[tertsdiepraam]

Just to check that I understand the issue correctly. You updated quickcheck, which now generates numbers from the entire range of a numeric type, which overflows the factor tests because it multiplies numbers from that range? Can we (in some cases) fix this more elegantly by telling quickcheck we want u32 and then casting to u64 for the multiplied result?

[g-k]

You updated quickcheck, which now generates numbers from the entire range of a numeric type, which overflows the factor tests because it multiplies numbers from that range?

Yep! On main I haven't seen the Arbitrary impl for Factors generate anything >=100 across a handful of runs (printing from the function even with RUST_LOG=quickcheck cargo test -- --nocapture didn't work, so I printed generated values from the tests that fail after upgrading).

I haven't dug into how BurntSushi/quickcheck@71d743a might've caused it to use such a small range.

Can we (in some cases) fix this more elegantly by telling quickcheck we want u32 and then casting to u64 for the multiplied result?

Possibly, the Arbitrary impl returns a Factors type which stores a numbers factors as a vec of u64 primes. I was trying to not change the Factors struct and reuse the factor constructor fn which takes a u64. (I also tried factor(u32::arbitrary(g).into()) and Factors(RefCell::new(Decomposition::one())) but I still ended up with wider u64s).

Other options:

• keep quickcheck rand on the older version of rand (might need to split the tests into a separate file)
• find an cleaner way to handle the new failures
  • ignore the newly failing tests
  • look for a config flag to change how arithmetic is handled for the tests
  • register a panic handler to suppress the quickcheck panics
• do smaller clean up things
  • add a test helper fn for checked arithmetic to DRY up c3450cd
  • tidy up the numeric::modular_inverse::tests::random_values_u{32,64} tests since those pass with wrapped muls instead of checked muls. I'm not sure that gets closer to the intent of the original test logic.

I'm sure I've missed other options too. Digging into how quickcheck uses the Arbitrary impl might be worthwhile.

[tertsdiepraam]

Alright, thanks for the explanation! Given these options, I think the current implementation is fine then!

[sylvestre]

excellent, thanks :)