Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo: Replace metal-rs patch with git dependency #228

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented May 22, 2024

gpu-allocator is a library crate, meaning that [patch]es don't get applied when users depend on our crate, and falsely end up with a broken build while our CI said everything was hunky dory ( #224). Even though we cannot normally have a direct git dependency as that blocks us from publishing to crates.io, the current gpu-allocator release (though only for Metal) is broken anyway. It's relying on functionality that has just now been merged upstream, but still has to make it into a (followup patch) release.

What's worse, the harcoded hash that this was pointing to no longer has a live reference on our fork (maybe due to a force-push), and the CI is now failing to to fetch that commit by hash while build-testing gpu-allocator.

Let's bump to a git dependency for now, and replace that as soon as we have a workable solution, however that pans out.

@MarijnS95
Copy link
Member Author

The CI is probably not going to like this change either, because the patch is needed to build metal. The upstream PR (gfx-rs/metal-rs#316) isn't merged yet and as explained a [patch] or git relesae doesn't work anyway, so we might just switch this dep to our custom registry and postpone crates.io releases until metal-rs gets all outstanding PRs merged, together with a minor/patch version bump?

Alternatively, could we make the metal code compatible with the latest release (i.e. drop the heap type and acceleration structure allocations) to preempt all this mess?

@MarijnS95
Copy link
Member Author

Alternatively, could we make the metal code compatible with the latest release (i.e. drop the heap type and acceleration structure allocations) to preempt all this mess?

I think we should bite the bullet and do this. Neither d3d12 nor Vulkan provide helpers to turn an Allocation into a buffer/texture/accelerationstructure, but give direct access to the underlying heap and offset+size of the region that's provided for this allocation. Besides, it doesn't even publicly expose those basic fields like the other APIs.

Unfortunately once we fix that we're still left with a missing heap_acceleration_structure_size_and_align_with_size() to create an AllocationCreateDesc.

@MarijnS95
Copy link
Member Author

We could also do a quick hack and push a tag to make Traverse-Research/metal-rs@a354c33 valid again, but that doesn't make any of the other issues outlined here go away.

@MarijnS95 MarijnS95 force-pushed the delete-metal-patch branch 3 times, most recently from a71a431 to 6cc9b92 Compare May 24, 2024 09:23
@MarijnS95 MarijnS95 changed the title cargo: Delete metal-rs patch cargo: Replace metal-rs patch with git dependency May 24, 2024
@MarijnS95 MarijnS95 requested a review from tosti007 May 24, 2024 09:24
@MarijnS95 MarijnS95 requested a review from FlannyH June 5, 2024 11:47
`gpu-allocator` is a library crate, meaning that `[patch]`es don't
get applied when users depend on our crate, and falsely end up with
a broken build while our CI said everything was hunky dory (
#224).  Even
though we cannot normally have a direct `git` dependency as that blocks
us from publishing to crates.io, the current `gpu-allocator` release
(though only for Metal) is broken anyway.  It's relying on functionality
that has just now been merged upstream, but still has to make it into a
(followup patch) release.

What's worse, the harcoded hash that this was pointing to no longer
has a live reference on our fork (maybe due to a force-push), and the
CI is now failing to to fetch that commit by hash while build-testing
`gpu-allocator`.

Let's bump to a `git` dependency for now, and replace that as soon as we
have a workable solution, however that pans out.
@MarijnS95 MarijnS95 merged commit 52b88cd into main Jun 6, 2024
26 checks passed
@MarijnS95 MarijnS95 deleted the delete-metal-patch branch June 6, 2024 14:57
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.

2 participants