Traverse-Research / gpu-allocator

🦀 GPU memory allocator for Vulkan, DirectX 12 and Metal. Written in pure Rust
https://traverse.nl/
Apache License 2.0
373 stars 49 forks source link

cargo: Support `ash 0.34` all the way up to `ash 0.37` #104

Closed MarijnS95 closed 2 years ago

MarijnS95 commented 2 years ago

Using a ranged semver here allows us to publish non-breaking patch releases whenever bumping to a new - compatible! - ash release. The upper bound serves to protect against breaking ash changes that may make this crate fail to compile, and is easy to bump whenever we've validated the next version to be compatible.

Note that the examples are updated to use the latest ash 0.37 and ash-window 0.10.0, containing breaking improvements around returning raw pointers to required extension string literals instead of CStr static borrows.

MarijnS95 commented 2 years ago

Changed to a range match for the crate for more versatility; see the updated PR description for more details.

Note that we could theoretically support back to ash 0.33 (the first release without traits which breaks use of the API here) if it weren't for the "debug" feature only being introduced in 0.34 (and I don't know nor feel it worth figuring out how to omit that feature from the list just for 0.33, that release is "ancient" by now :grimacing:).

attackgoat commented 2 years ago

I tried using this branch but got conflicts with the other libraries my project uses; ash-window, vk-sync-fork, etc.

gpu-allocator = { git = "https://github.com/Traverse-Research/gpu-allocator.git", branch = "ash-0.37" }

Cargo tree of interest:

├── ash v0.37.0+1.3.209
....
├── gpu-allocator v0.17.0 (https://github.com/Traverse-Research/gpu-allocator.git?branch=ash-0.37#6362bc0b)
│   ├── ash v0.35.2+1.2.203
│   ├── ....

I thought that because my code provides gpu-allocator with lots of ash types such as vk::MemoryRequirements and this new dependency range is in place that cargo would automatically pick the specific version my project used, but that doesn't seem to be the case. Instead cargo selects a 'good enough' version for gpu-allocator and expects me to work through the differences.

I guess this would be fine if the field names stayed the same and everything was basic types, but in these cases the ABI of the types themselves changed.

Any advice for downstream projects?

MarijnS95 commented 2 years ago

I guess this would be fine if the field names stayed the same and everything was basic types, but in these cases the ABI of the types themselves changed.

I don't think there's a case where types from different versions of the same crate are considered compatible by rustc, and all vk:: types have undoubtedly remained ABI compatible because they are utilized directly in Vulkan interop. This could only work if the semver trick is employed to make one crate-version defer to the types and symbols of the other crate-version.

This change, as the title implies, makes gpu-allocator support Ash from 0.34 all the way up to 0.37. However, cargo is - in my eyes - quite stupid in how it selects what version to use. I can only assume you've previously been using gpu-allocator 0.16 which used ash 0.35. As such, when updating to this git branch of gpu-allocator, cargo sees the selected version is still in range and doesn't bother to change/update it.

In any case, when running cargo update (or the implicit cargo generate-lockfile when no Cargo.lock lockfile exists, ie. a fresh clone of a project) it'll select the latest available version for that range. That will resolve this issue in your case (or it can be fixed manually through cargo update -p ash:0.35.2 iirc), but also means that combining crates with different max for ash will always result in duplicate (type-mismatching) dependencies on cargo update or a fresh clone.

Any advice for downstream projects?

Takeaway: Don't accept duplicate ash versions in your dependency tree, when (type) conflicts arise in a codebase. Trying to work around this while assuming ABI compatibility is messy, ugly, and quite frankly unnecessary.


All in all I'm not sure if this range is the right approach. On the one hand it allows the latest version of gpu-allocator to be used with a broader range of ash releases; on the other users have to be careful to manage and maintain their dependencies (checking in Cargo.lock solves nothing for released crates).

At the same time we'll be drastically reducing the frequency of breaking releases for ash, saving up breaking features for much larger releases instead of (accidentally, in this case) release a lot of breaking-version-bumps in quick succession (for relatively minor breakage/improvement).

attackgoat commented 2 years ago

@MarijnS95 Thank you - I simply forgot to cargo update which did indeed fix my tree.