Update dependencie rand to 0.9 on simple-cipher

[lbellomo]

On simple-cipher, we can update the rand version to the latest version because the new version has several function name changes that, if you don't realize that you are looking at the wrong version of the doc, give meaningless errors like expected function, found module 'rand::rng' or module 'rng' is private (and it's even worse if you are looking at the rust-rand-book, because it doesn't say the version anywhere).

I saw that the rust-test-runner already updated the rand to 0.9 and I checked that it works by adding a solution with that version.

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[ellnix]

Ran across the same thing myself but that was before rand's version was bumped so I forgot about it. Good catch 👍

The reason the CI workflow is failing is because of the large change in rand from 0.8 to 0.9 is breaking example.rs: https://github.com/exercism/rust/blob/main/exercises/practice/simple-cipher/.meta/example.rs

I'll let you fix it if you want to take this PR through yourself, otherwise if you don't have the time I can fix it for you.

[senekor]

I had assumed that 0.8 would fail to compile anyway in the test runner. I submitted an iteration with 0.8 to test and it passed. Now I'm confused. The test runner only has access to the latest version of a library... 🤔

Maybe it's better to have rand = "*" to avoid this problem in the future?

[ellnix]

Maybe it's better to have rand = "*" to avoid this problem in the future?

Wouldn't that break example.rs and therefore CI every time rand has breaking changes?

[senekor]

Maybe it's better to have rand = "*" to avoid this problem in the future?

Wouldn't that break example.rs and therefore CI every time rand has breaking changes?

Yes, but broken CI is better than confused students. (or even failing tests because of mismatched version, although that doesn't seem to be the case here, which confuses me)

[ellnix]

Yes, but broken CI is better than confused students.

But there would also be the issue that rand = "*" would resolve differently on someone's local machine vs the test runner, also leading to cryptic error messages if rand has an update that is not yet in the test runner. We do not recommend submitting Cargo.lock anymore.

Edit: I think it might make sense to add an instructions append file telling the student to check the Cargo.toml file and look at the documentation for that specific version.

although that doesn't seem to be the case here, which confuses me

I just investigated it and it looks pretty straightforward, there are a bunch of crates that depend on rand 0.8.5 which is why it's being pulled in, if you just do a search for rand 0.8 in local-registry/Cargo.lock you'll see what I mean.

I confirmed locally that removing the crates that depend on rand 0.8 from Cargo.toml and generating Cargo.lock causes the runner to error on a rand 0.8 solution.

[senekor]

Yes, but broken CI is better than confused students.

But there would also be the issue that rand = "*" would resolve differently on someone's local machine vs the test runner, also leading to cryptic error messages if rand has an update that is not yet in the test runner. We do not recommend submitting Cargo.lock anymore.

Edit: I think it might make sense to add an instructions append file telling the student to check the Cargo.toml file and look at the documentation for that specific version.

The dependencies of the test runner are updated with dependabot, so the time frame where users might pull in newer versions should be minimal. But it wouldn't be great if it happens.

What if we had some CI check that made sure the dependencies in the exercise definitions match the available ones in the test runner? (I lowkey wish exercism had a monorepo, coordinating changes between all the disparate repos is no fun.)

although that doesn't seem to be the case here, which confuses me

I just investigated it and it looks pretty straightforward, there are a bunch of crates that depend on rand 0.8.5 which is why it's being pulled in, if you just do a search for rand 0.8 in local-registry/Cargo.lock you'll see what I mean.

I confirmed locally that removing the crates that depend on rand 0.8 from Cargo.toml and generating Cargo.lock causes the runner to error on a rand 0.8 solution.

Ah, that makes sense! Forgot to consider that transitive dependencies are also available. Thanks for checking.

[ellnix]

What if we had some CI check that made sure the dependencies in the exercise definitions match the available ones in the test runner? (I lowkey wish exercism had a monorepo, coordinating changes between all the disparate repos is no fun.)

It should be relatively easy, it can even be a string comparison. I presume that git submodules are a no-go considering the lengths we go to with symlinking problem-specifications?

If we used a git submodule of exercism/rust inside exercism/rust-test-runner we could have one of the CI tests update the submodule and search through Cargo.toml files while the rest of the tests simply clone exercism/rust-test-runner without submodules in blissful ignorance.

I expect we are much more likely to run into cases of an update of dependencies causing a regression rather than an update to an exercise adding a dependency that doesn't exist, so I think whatever we decide on belongs on the CI of exercism/rust-test-runner.

[senekor]

I presume that git submodules are a no-go considering the lengths we go to with symlinking problem-specifications?

Not quite, problem-specifications is a special case because configlet is stupid and insists on managing its own cache. I used to have problem-specifications as a submodule, but it lead to confusion whenever confliget's cache disagreed with it. Instead of trying to keep them in sync, symlinking to configlet's cache seemed like the more reliable solution. (I guess it would've been possible to do both, have a submodule and a script that makes sure problem-specifications is checked out at the same commit... but I don't see the point in that.)

As a small aside, I'm using jj as vcs instead of git, which doesn't really support submodules. (It ignores them, so I have to drop down to git commands to keep submodules in sync.) Not a big deal, but I prefer no submodules unless there's a tangible benefit.

A submodule also doesn't really make sense here in my opinion, because we always want to check the latest commit of the other repo. There's no point in pinning an old version. Whenever we run the check, we need to clone / fetch the latest commit anyway.

CI check on the test runner repo seems like a good idea. Only when pushing to the main branch, so we get notified when we need to update the track as well. The test runner has to be updated first, so we can't block those updates by the check.

[ellnix]

Created an issue on the test runner repository, so we can continue the discussion there since it's off topic here.

[lbellomo]

I'll let you fix it if you want to take this PR through yourself, otherwise if you don't have the time I can fix it for you.

Sure, thanks for the opportunity!
I promise to go to the forum first next time.

I fixed what was wrong and made sure it's work local. I also changed the commit message to be "exercise_name: change" trying to make it similar to the others commits.

I understand that i don't have to touch anything else (and that the .meta/config.json is taken care of by a bot).

[senekor]

Thanks!