Skip to content

Conversation

@dhardy
Copy link
Member

@dhardy dhardy commented Oct 30, 2025

Prepare a pre-release.

CI is going to fail. @tarcieri you really use inter-repo dependencies this way?

The "best" option may be to move rand_core to a new repo, though that makes it harder to test breaking changes. Or we just push to stabilise rand_core v1.0 (already the plan) and accept that CI will fail sometimes in the mean-time.

@dhardy dhardy requested review from newpavlov and tarcieri October 30, 2025 09:15
@dhardy
Copy link
Member Author

dhardy commented Oct 30, 2025

Alternatively, we could move chacha20 to this repo (from the stream-ciphers repo).
Advantage: easier to test that rand packages work together.
Disadvantage: none?
I considered further removing cipher support from chacha20 but concluded that this would significantly complicate the core objectives (provide quality rand_core and cipher implementations of the ChaCha algorithm).

@newpavlov
Copy link
Member

newpavlov commented Oct 30, 2025

CI is going to fail. @tarcieri you really use inter-repo dependencies this way?

In the RustCrypto case rand_core would reside in a separate repository (i.e. RustCrypto/traits), so we usually do not have such circular dependencies. When we do not have pre-releases we use dependency patching. I think it should work fine in this case, i.e. you should add the following lines to the root Cargo.toml:

[patch.crates-io]
rand_core = { path = "rand_core" }

For future development, I think it may be worth to consider moving rand_core into its own repository.

@dhardy
Copy link
Member Author

dhardy commented Oct 30, 2025

The patch is already there. It won't help here until chacha20 is updated to use the new rand_core version.

@newpavlov
Copy link
Member

Ah, I thought we already updated chacha20 to rand_core v0.10. I created RustCrypto/stream-ciphers#475 to amend this.

@newpavlov
Copy link
Member

@dhardy
What do you think about moving rand_core into a separate repo instead of merging this?

@tarcieri
Copy link

CI is going to fail. @tarcieri you really use inter-repo dependencies this way?

Yes, though if you're doing it right, you can get CI green before merging, e.g. by pulling in chacha20 from this PR: RustCrypto/stream-ciphers#475

Alternatively, we could move chacha20 to this repo (from the stream-ciphers repo).
Advantage: easier to test that rand packages work together.
Disadvantage: none?

chacha20 is first and foremost a stream cipher, not a random number generator. It's an integral dependency of the chacha20poly1305 crate and contains quite a bit of functionality not relevant to the RNG use case (e.g. stream cipher interface, 32-bit IETF variants, XChaCha).

@newpavlov
Copy link
Member

The stream-ciphers PR is merged.

@dhardy dhardy force-pushed the push-spqltotzmryv branch from 74ad09b to ce5c156 Compare October 30, 2025 14:16
[patch.crates-io]
rand_core = { path = "rand_core" }

[dependencies]
Copy link

@tarcieri tarcieri Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually put these at the bottom of the file but this should get CI green:

Suggested change
[dependencies]
[patch.crates-io.chacha20]
git = "https://github.com/RustCrypto/stream-ciphers"
[dependencies]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we don't actually want that (assuming the plan is to make crates.io releases of both crates today).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth to fix CI before merging. IIRC the patch section does not affect cargo publish, so you will be able to remove it in a separate PR after we will release cahcha20.

@dhardy
Copy link
Member Author

dhardy commented Oct 30, 2025

Tests now pass locally using:

chacha20 = { git = "https://github.com/RustCrypto/stream-ciphers.git" }

@newpavlov approve please?

What do you think about moving rand_core into a separate repo instead of merging this?

If we expect on-going breaking changes to rand_core, this would make sense. But likely we won't have many more breaking changes there.

Also, the only real advantage is that CI doesn't fail other rand crates when we update rand_core like this.

(We will also need rand_core and chacha20 pre-releases released soon since CI will be broken for this crate until then.)

@newpavlov
Copy link
Member

newpavlov commented Oct 30, 2025

If we expect on-going breaking changes to rand_core, this would make sense.

Personally, I prefer it from the decoupling point of view as well. I think the circular dependency will continue to be problematic in future. I also plan to submit a PR which re-works the block module.

@tarcieri
Copy link

tarcieri commented Oct 30, 2025

@dhardy you should be able to commit the change to use the patch and push it up, which will get CI green, and like @newpavlov said when you publish rand_core, it will be ignored. Then we can publish chacha20, and you can do a followup commit to switch to the crate release.

It seems like you shouldn't ever have to merge with CI red.

@dhardy
Copy link
Member Author

dhardy commented Oct 30, 2025

I decided to go ahead with a new repo: https://github.com/rust-random/core

rand_core = { path = "rand_core", version = "0.10.0-rc.1", default-features = false }
log = { version = "0.4.4", optional = true }
serde = { version = "1.0.103", features = ["derive"], optional = true }
chacha20 = { version = "=0.10.0-rc.2", default-features = false, features = ["rng"], optional = true }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
chacha20 = { version = "=0.10.0-rc.2", default-features = false, features = ["rng"], optional = true }
chacha20 = { version = "=0.10.0-rc.3", default-features = false, features = ["rng"], optional = true }

@dhardy dhardy closed this Nov 4, 2025
@dhardy dhardy deleted the push-spqltotzmryv branch November 4, 2025 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants