rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.63k stars 428 forks source link

Please consider reducing the number of rand-* crates #850

Closed kpcyrd closed 4 years ago

kpcyrd commented 5 years ago

Hello!

I'm one of the (co-)maintainers of the rand-* packages in debian. I'd like to suggest a discussion about moving from the micro-crate approach to a smaller number of crates.

Our update workflow currently looks like this, we started a rand update shortly after the debian buster release in early July and are still working on it:

1) Due to the number of crates depending on rand we upload semver suffixed packages of rand crates first so we don't break the dependency tree for most of our packages 2) Since those are technically new packages we need to wait for approval to get them accepted 3) Next, we can update the first few packages (usually rand-core) 4) Next, we can start packaging crates that are a) new in this release b) depend on the newly updated rand-core and c) didn't previously exist 5) Again, those need to be approved 6) If the new crates are depending on each other step 4 and 5 need to be repeated 7) After all dependencies are updated and missing crates have been accepted we can push the actual rand update

A smaller number of crates (or even a single rand crate) would save us some overhead and make our work a bit easier (we are about a handful of volunteers currently maintaining 572 packages).

What are your thoughts on this?

Thanks!

dhardy commented 5 years ago

Hello!

I'm guessing you don't have a method of packaging multiple Rust crates into a single dpkg? Because I agree with your assessment that multiple packages make little sense within Debian. Rust already has many small crates, which provides a great mechanism for code reuse.

For version 0.7 we have made some effort to reduce the number of dependencies (see #713). Actually, that list is somewhat out of date: rand_pcg is now an optional dependency (I don't know how you handle this), getrandom was added, several platform-specific API crates are no longer needed (mostly won't affect Debian), and rand_chacha was re-written to use c2-chacha (technically we could copy all this code into rand_chacha but we don't particularly want to maintain it — more work for us).

burdges commented 5 years ago

You could package anything you like together, but..

You need some version number for the overall package, so rand should commit to updating its own version number anytime you suggest updating rand sub-crates. If not, you're ask them to make up this overall version number themselves, which makes no sense.

I'm also curious if/when Rust needs some "packaging and dynamic linking WG" that maintains a "Rust distribution" ala LaTeX which handles many common crates via dynamic linking. If such a project existed then it would provide a smaller number of version numbers, along side with the primary goals of making Rust code more memory efficient, and preventing churn by slowing crate updates.

dhardy commented 5 years ago

You need some version number for the overall package, so rand should commit to updating its own version number anytime you suggest updating rand sub-crates.

I don't think we want to commit to this: part of the reason for the separation is to de-couple versions.

There is another option which may be relevant when packaging internal dependencies: include them within the same lib (probably possible via automated code mutation; could potentially be done via a compiler option in the future). E.g. rand_chacha and its dependencies could all be built into a single rand dylib (with no API changes to rand); if none of the software you are packaging depends on rand_chacha (and rand_pcg etc.) directly then this may be the most efficient way to package rand.

preventing churn by slowing crate updates

In our case we don't have a very fast release schedule anyway; I don't believe slowing this even further would be a good trade.

"packaging and dynamic linking WG"

IMO, at some point Rust should revisit the question of "what is a crate". E.g. I think it should be possible to have multiple independently versioned APIs provided by the same lib — in this case a single "rand" package which provides rand_core 0.5 + rand 0.7 + rand_pcg 0.2 etc. This could also work well for compatibility shims if a crate is able to expose two versions of an API built from the same internal components.

vks commented 5 years ago

in this case a single "rand" package which provides rand_core 0.5 + rand 0.7 + rand_pcg 0.2

It is not clear what version this package should have. It's version would have to change for every minor update of rand or rand_pcg etc. I think there is a lot of potential for confusion.

dhardy commented 5 years ago

Hmm, when it comes to packaging dylibs this doesn't help.

Theoretically Rust could build it's own type of dylib which is allowed to include multiple APIs, and allowed to link in multiple versions of the dylib (so long as no more than one copy of the API version is active), but that's quite another thing (and hard to make compatible with C libs).

vks commented 5 years ago

It is not clear what version this package should have. It's version would have to change for every minor update of rand or rand_pcg etc. I think there is a lot of potential for confusion.

Actually, this is probably fine: Debian's rust-rand package could ship the rand crate with default features. (Not sure how features are dealt with when packaging.) The version in Debian would correspond to the rand version and the Debian revision could be increased whenever the packagers run cargo update to rebuild rust-rand. (See the definition of versions in Debian.) This way, it might be sufficient to provide only rust-rand, which is what most Rust crates are using.

dhardy commented 5 years ago

Perhaps @burdges is right that we should commit to updating the rand version any time we publish a semver-incompatible version of a dependency, for certain crates (I mean, so far we have published a new rand minor version almost every time we have published a new minor version of rand_core and PRNG crates). This of course means it will be harder releasing new versions of those crates, but that doesn't happen a lot.

That should allow many of these crates to be packaged together.

If we do that I would want to exclude rand_distr at least, possibly also getrandom.

dhardy commented 5 years ago

@vks's idea by itself could result in multiple versions static/thread-local symbols within binaries (e.g. ThreadRng) and unnecessary copies of code, but no API breakage at least.

burdges commented 5 years ago

We'll see if @kpcyrd thinks such versioning tricks suffice.. or maybe if other issues cause the trouble.

Apologies for the derail.. I have not considered rand's version trick much, so not sure if that's the real solution. I do think rust binaries including multiple crate versions makes sense for a young developing language, but rust eventually becoming a real systems language in which people write the kernel, system libraries, GUIs, etc. really benefits from a dylibs system in which very few crate version get used much. Ideally some WG would bless particular crate versions every year or so. We do not require this in scripting-inspired languages like Go because you deploy code devops style but you need this to become a foundation for everything else.

Now Debian packages many scripting-inspired languages, so @kpcyrd might be running into something more rand specific.

gnzlbg commented 5 years ago

I don't think we want to commit to this: part of the reason for the separation is to de-couple versions.

That sounds nice until things break.

I personally do not mind whether rand uses zero or 200 dependencies, but rand should make sure that it works with every single valid combination of its dependencies, and that updating minor rand versions works under arbitrary valid dependency graph constraints.

The more dependencies rand has, the more work this takes. The concern isn't theoretical: a patch level update from rand 0.6.1 to rand 0.6.5 fails in rust-lang/rust, because rand 0.6.5 does not link properly for the versions resolved for its dependencies in rust-lang/rust dependency graph. The versions resolved there are allowed by the Cargo.toml files of rand and its dependencies, so it is a valid dependency tree. This means that 0.6.5 should be yanked, and a new 0.6.6 without the issue should be released. The more dependencies rand has, the more work fixing things when they break will take.

dhardy commented 5 years ago

@gnzlbg I disagree. See #741 for example. This type of testing adds a lot of work for very little gain. For the specific issue you mention, I believe the correct solution would be to mark rand_core 0.3.0 as incompatible with version 0.4.0, thus forcing use of the compatibility shim 0.3.1. However, Cargo and semver simply always consider it acceptable to link multiple copies of a library into a build — something questionable in many cases.

This is similar to the diamond inheritance problem: A: B + C, B: D, C: D — but does it matter whether only a single copy of D is used? In some cases, yes. Cargo does not allow this constraint to be described.

gnzlbg commented 5 years ago

believe the correct solution would be to mark rand_core 0.3.0 as incompatible with version 0.4.0,

The links cargo key can be used to enforce that only a single version of a crate is linked. A dependency graph can only contain one crate with a particular links key.

This type of testing adds a lot of work for very little gain

I'm not suggesting that you test this, I'm arguing that this shouldn't happen. How you achieve that goal is up to you.

dhardy commented 5 years ago

The links cargo key can be used to enforce that only a single version of a crate is linked.

Where are the docs for that?

gnzlbg commented 5 years ago

Where are the docs for that?

https://doc.rust-lang.org/cargo/reference/manifest.html#the-links-field-optional https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key

Primarily, Cargo requires that there is at most one package per links value.

dhardy commented 5 years ago

So we would need to make rand_core a "native" library with a C API to do this? That's not a solution.

gnzlbg commented 5 years ago

So we would need to make rand_core a "native" library with a C API to do this?

No, that is not required.

dhardy commented 5 years ago

So if we add links = "rand_core" and an empty build script to every version of rand_core's manifest (via new patch releases), that should usage of multiple versions of rand_core? Or not? That documentation states no reason for this to be the case. It's also a heavier hammer than we need (which would e.g. prevent usage of rand 0.6 and 0.7 simultaneously).

It's also not possible to do that since (from my understanding) we cannot retroactively add this to the problematic version, 0.3.0. Also yanking version 0.3.0 doesn't solve the problem for those doing minimal upgrades.

gnzlbg commented 5 years ago

So if we add links = "rand_core" and an empty build script to every version of rand_core's manifest (via new patch releases), that should usage of multiple versions of rand_core?

If you do that, only a single version of rand_core is allowed in the dependency graph.

Note that the error produced in rust-lang/rust is a link-time error. That's super weird to encounter in Rust, and often only happens if you are trying to link two incompatible versions of a C library anyways.

It's also a heavier hammer than we need (which would e.g. prevent usage of rand 0.6 and 0.7 simultaneously).

This is broken already, at least for some combinations of rand 0.6 and 0.7.

It's also not possible to do that since (from my understanding) we cannot retroactively add this to the problematic version, 0.3.0. Also yanking version 0.3.0 doesn't solve the problem for those doing minimal upgrades.

You probably would need to yank 0.3.0 and 0.3.1 and release a newer version with the fix. But giving that the whole "compatibility wrapper" can create link-time errors, I wonder how compatible this "trick" actually is. I haven't studied what rand does there in detail.

dhardy commented 5 years ago

It isn't the compatibility wrapper which is responsible for link-time errors, it's the usage of multiple versions of a shared API (rand_core) without a compatibility wrapper which is responsible.

I don't think yanking 0.3.0 would fix anything: the only users hitting this error are those doing a conservative upgrade from an early 0.6.x of rand who are not updating rand_core. For everyone else, Cargo will automatically use the latest 0.3.x anyway.

This is broken already, at least for some combinations of rand 0.6 and 0.7.

That's the first I've heard of this. I don't see why it should obe.

gnzlbg commented 5 years ago

it's the usage of multiple versions of a shared API (rand_core) without a compatibility wrapper which is responsible.

So how does that result in linking errors of C symbols? Maybe we should move this conversation to a different issue.

dhardy commented 5 years ago

It doesn't. Sounds like we're talking about different issues, so yes, open a new issue if you like.

dhardy commented 4 years ago

I will close this issue now because (a) it has gone off-topic, (b) it appears to relate more to Rand 0.6, and (c) it has no concrete deliverables. I do consider this an important aspect of any design changes.

For reference, this is the dependency tree of a minimal crate:

rand-num v0.1.0 (/home/dhardy/projects/rand/rand-num)
└── rand v0.7.0
    ├── getrandom v0.1.11
    │   ├── cfg-if v0.1.9
    │   └── libc v0.2.62
    ├── libc v0.2.62 (*)
    ├── rand_chacha v0.2.1
    │   ├── c2-chacha v0.2.2
    │   │   ├── lazy_static v1.4.0
    │   │   └── ppv-lite86 v0.2.5
    │   └── rand_core v0.5.0
    │       └── getrandom v0.1.11 (*)
    └── rand_core v0.5.0 (*)
    [dev-dependencies]
    └── rand_hc v0.2.0
        └── rand_core v0.5.0 (*)

Of these, getrandom is an important separation (whose functionality might eventually be added into the std lib); rand_chacha and its dependencies may be worth discussing but in this case the discussion deserves its own issue.