Reexport dependencies

[Ofenhed]

When I updated diesel from 1.4.5 to 1.4.6 (a patch level update) my environment broke. The reason it broke is that I had copied the ipnetwork dependency from the Diesel Cargo.toml file, and Diesel had updated its dependency version for ipnetwork, where I had not.

To avoid this kind of issue I suggest that all Rust dependencies used in in diesel::sql_types and diesel::<database>::types should be reexported. I would suggest creating rust_types modules only containing #[cfg(feature...)] pub extern crate XXX; for the dependencies of that database type.

Users would then use for example ipnetwork by setting the diesel/network-address flag and using diesel::pg::types::rust_types::ipnetwork::IpNetwork.

This is no longer part of the rust guidelines, but seem to be the agreed upon method if third party types are to be used, according to this discussion in rust-lang.

[weiznich]

First of all to be very clear: The change released with diesel 1.4.6 is not a breaking change, because the old version of ipnetwork continues to be compatible with diesel. We only included more versions in our supported version range. If some build is broken by this it means that someone issued a plain cargo update and not a specific cargo update -p diesel.
Otherwise at least I'm personally against reexporting things from other crates via diesel as this would easily allow breaking changes to come in that way from third party crates that otherwise wouldn't affect diesel in any way. In consequence this would mean that we could support only crates that give similar guarantees for their API as diesel, which would remove every third party crate, but serde or so.

[Ofenhed]

Actually, it was on a build server, so it wasn't a cargo update, it was a cargo build in a clean environment. This also means that anyone downloading my project and building would have build issues that I would not have if I only run cargo update -p diesel. Sure, my personal build environment was not broken, but for anyone coming in new to the project it absolutely breaks.

edit: I realized now that it breaks because I don't have my cargo.lock checked in. I think, however, that this question should still be discussed. I don't think Cargo.lock is a solution, just a mitigation. I also find it risky, since it may quietly block patch level security updates.

I agree that it would allow breaking changes to come in, but only when the dependency version restrictions are too broad or that crate introduces a breaking change in a too low level. As I see it, it's really a three way choice:

1. With all types used publicly in Diesel, either reimplement them as Diesel local types or use types from std
2. Reexport the libraries used
3. Don't reexport the libraries used and let the users handle it with their Cargo dependencies

1 is the preferred method, as there will be no breaking changes ever, since you are completely in control. This is very impractical, since Diesel uses a lot of complex data types.

2 will expose breaking changes in dependencies. The user can still, however, use that crate as a direct dependency, just as right now, instead of using the reexported crate. This can open up to bad code practices, where developers use the reexported crates of other projects instead of keeping track of packages and breaking changes themself. If they do, though, the error messages will probably be more clear than error[E0277]: the trait bound ipnetwork::IpNetwork: Queryable<diesel::sql_types::Inet, _> is not satisfied, when the same code works on your colleges computer and you can clearly see that you have the correct types and flags supported to Diesel.

3 will break a lot, since the user needs to keep track of which dependencies Diesel have.

I think that all these options are crap, to be honest, and it's a real issue that Rust will have to deal with. I do find that option two is a little bit less crap than the other two, since it gives users options.

[weiznich]

From your write it sounds like variant 2 is "the" solution, but that's really not true. Now most of the dependencies in questions are at version 0.x and they release new versions 0.x+1 quite often. Now diesel has already released a version 1.y, which means we cannot simply do breaking changes anymore. That means removing support for an old version of a dependency is not possible, as this support is part of our public API. Now adding support for newer version of those dependencies is mostly easily possible, as most of their breaking changes where in API's not used by diesel, which means we can just support the old and the new version regardless of if there is a breaking change in any other part of the API. Now let's assume we would reexport dependencies as you proposed as variant 2. In that case it isn't possible to do this anymore as the whole API of the dependency is now also public API of diesel, which means we cannot update to any newer version than the initial supported one without an breaking change. So in fact instead of just allowing to use possibly fixed versions of those crate to be used we would pin that version to something really old. This problem becomes much much more serve as soon as you depend on any crate (say rocket or actix) which also depend on any of our dependencies. If they now decide to drop support for our dependency version for any reason we would need to issue a major release for just this version bump to stay compatible. It gets even worse if you assume that they do the same thing.
For me personally this would essentially mean to remove support for any third party crate from diesel as I'm not willing to maintain this.

If they do, though, the error messages will probably be more clear than error[E0277]: the trait bound ipnetwork::IpNetwork: Queryable<diesel::sql_types::Inet, _> is not satisfied, when the same code works on your colleges computer and you can clearly see that you have the correct types and flags supported to Diesel.

That's a rustc issue not a diesel issue then. I believe they can issue a much better error message like note: There is this impl for ipnetwork::IpNetwork (version x.y) but you used ipnetwork::IpNetwork (version z.a) instead here. Have you tried to raise that issue there?

I realized now that it breaks because I don't have my cargo.lock checked in. I think, however, that this question should still be discussed. I don't think Cargo.lock is a solution, just a mitigation. I also find it risky, since it may quietly block patch level security updates.

That's the official way to deal with these sort of problems. For applications you don't want to allow just arbitrary dependency updates out of the blue, you want to pin a specific version.

[Ofenhed]

I think that is a question of attitude. If it's documented as a shortcut to the crate used to build diesel, then the API that you must support is that diesel::...::ipnetwork points to the crate that diesel was compiled with. To control the version that diesel::...::ipnetwork points to the user can use Cargo.lock, but while diesel::...::ipnetwork is guaranteed to be compatible with the diesel instance it's content is not guaranteed to be compatible with your code and its content is not part of the API, no more than a link is the sum of its target. Do you think that is an unreasonable view of API:s?

[weiznich]

I don't think that would be allowed as something that is defined in RFC-1105. End the end it boils down to something I personally do not want to do this for the reasons outlined above. I believe the strategy we have choosen so far worked quite well. You can disagree with that, but this will not change as long as I'm the person that is maintaining this code, as I'm really don't want to maintain what you've proposed above. That would basically just "shift" the burden of using a dependency from you to me, which is something I'm not willing to do additionally to my other work on diesel. Supposedly some day someone makes that to some official Rust API guideline we can talk about this again, otherwise I consider this not actionable in any meaning full way. If that day comes the consequence for me would likely be to remove all third party dependencies for the reasons outlined above, at least as long as they've not choosen to have a long maintained 1.x release.