Skip to content

Conversation

@MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Mar 23, 2022

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 MarijnS95 force-pushed the ash-0.37 branch 2 times, most recently from 61e0fb1 to 25a8c6e Compare March 23, 2022 21:34
@MarijnS95 MarijnS95 marked this pull request as draft March 24, 2022 07:27
@MarijnS95 MarijnS95 changed the title cargo: Upgrade to ash 0.37 and ash-window 0.10 cargo: Support ash 0.34 all the way up to ash 0.37 Mar 24, 2022
@MarijnS95
Copy link
Member Author

MarijnS95 commented Mar 24, 2022

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 😬).

@MarijnS95 MarijnS95 marked this pull request as ready for review March 24, 2022 09:15
@attackgoat
Copy link

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
Copy link
Member Author

MarijnS95 commented Apr 9, 2022

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).

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.
@attackgoat
Copy link

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

@MarijnS95 MarijnS95 mentioned this pull request May 11, 2022
Copy link

@BeastLe9enD BeastLe9enD left a comment

Choose a reason for hiding this comment

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

Looks good to me, I tested it (including all examples), it works as it should.

@MarijnS95 MarijnS95 requested a review from manon-traverse May 17, 2022 15:21
@MarijnS95 MarijnS95 merged commit 6c8e150 into main May 17, 2022
@MarijnS95 MarijnS95 deleted the ash-0.37 branch May 17, 2022 15:45
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.

5 participants