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

generator: Update list of vendor names #676

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

charles-lunarg
Copy link
Contributor

Noticed that the vendor list is a bit on the lean side. I'm not sure what impact this will have (in fact, I didn't even run the code because I wasn't sure how to set up the rust environment to test it, and it seems like a 'no brain required' change.)

Regardless, while I would of liked to address the TODO to locate the vendors from the code directly, my rust isn't capable of that at the moment. Rather than ask one of the ash maintainers to do it, this lets any new vendor extensions not cause ash builds to fail.

@MarijnS95
Copy link
Collaborator

I'm not sure what impact this will have

This list is only used while stripping enum variant types (FlagBits), and my general stance has been to not add vendors until they start shipping flags in vk.xml; we'd get a clear generator error otherwise which is really only relevant for those who update the bindings (also me).

@charles-lunarg
Copy link
Contributor Author

That makes sense.

I did make this change due to an upcoming additional vendor adding flag bits, but since its not in the public vk.xml I didn't want to add only the one that is needed.

Should I close the PR? seems this commit goes against the established policy for this repo.

@MarijnS95
Copy link
Collaborator

@charles-lunarg That is the kind of issue I had in mind, after adding ash to the Khronos CI. Happy to pull this in to unblock upstream, there's no real gain in not listing all the vendors here however.

@MarijnS95
Copy link
Collaborator

Ugh, just saw this in another PR half an hour ago: the freshly released bytemuck crate bumps our MSRV to 1.60: Lokathor/bytemuck#140 (comment)

I hope it's only the generator though, allow me to fix the CI first to only lint our published crates as the generator can rely on nightly for what I'm concerned.

@charles-lunarg
Copy link
Contributor Author

I don't think this is blocking upstream per-say, as the upstream CI checks don't fail if ash fails, it just documents that it didn't succeed. (Aka, a warning not an error).

I will fix the formatting error. I'll just copy/paste what the formatter tells me it should be. But I'll wait till the bytemuck issue is resolved :)

@MarijnS95
Copy link
Collaborator

Sure, I set it up that way to not truly block everyone, but seeing a red/orange "CI failed with warnings" is never useful/nice 😉

Just run cargo fmt locally, set up your Rust environment while we eagerly await more contributions 🥳

@MarijnS95
Copy link
Collaborator

Let's see if #677 addresses the MSRV issue.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Nov 7, 2022

@charles-lunarg You can rebase now :)

EDIT: Looks like I get the update-branch button as well, but missed the arrow to do a rebase. That's fine, we'll squash-merge this anyway.

Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Good, but one request: can we sort these alphabetically? 😬

@charles-lunarg
Copy link
Contributor Author

Alphabetical order is no issue, and it is much more pleasing to look at.

@MarijnS95 MarijnS95 merged commit 29b935b into ash-rs:master Nov 7, 2022
@MarijnS95
Copy link
Collaborator

@charles-lunarg Upstream CI should now be unblocked, which picks directly from the master branch.

@charles-lunarg charles-lunarg deleted the charles_update_vendor_list branch November 7, 2022 22:54
@charles-lunarg
Copy link
Contributor Author

Thanks!

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.

None yet

2 participants