-
Notifications
You must be signed in to change notification settings - Fork 213
ash: Hide provisional extensions behind new provisional feature flag
#952
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
base: master
Are you sure you want to change the base?
Conversation
|
BTW, I don't know that it impacts anything you're doing here, but just to note that in the C bindings, the provisional types and commands are kept in a separate 'beta' header, but the provisional enums added to existing enumerated types have to be visible in the core API header as there is no way to define them separately from the type they are part of. |
|
They could remain in the same file - it's only a handful - and individual enum variants or constants can also be hidden behind a |
404e7dc to
6bc7a61
Compare
|
Working on this PR is really showing that the generators' lack of building definitions etc. top-down while being aware of their enabling feature(s)/extension(s) is such a hassle to build back in. It takes quite a while to push that information back in and is messy while at it. |
020d26d to
6b92340
Compare
6b92340 to
e42a781
Compare
Provisional extensions are susceptible to **breaking** API changes at any point (with accompanying `SPEC_VERSION` bump). By hiding them behind a non-default feature flag, we can document that opting into this feature allows us to do breaking changes to these modules in non-breaking `ash` releases when driven by upstream `Vulkan-Headers` changes to `vk.xml`. Moreover, upstream Khronos repositories that build `ash` in their CI can continue to build-test their changes without compile-testing any extension behind a `provisional` flag, which is especially useful if we've already wrapped a `provisional` extension that is seeing breaking changes.
e42a781 to
4133f98
Compare
I don't think we need to overthink this -- semver is ultimately a tool for humans, and we shouldn't make the human experience worse for its sake. |
Sure enough, keeping this behind a Semver isn't totally a "human" tool to me as it has a machine meaning to things like |
Fixes #343
Provisional extensions are susceptible to breaking API changes at any point (with accompanying
SPEC_VERSIONbump). By hiding them behind a non-default feature flag, we can document that opting into this feature allows us to do breaking changes to these modules in non-breakingashreleases when driven by upstreamVulkan-Headerschanges tovk.xml.Moreover, upstream Khronos repositories that build
ashin their CI can continue to build-test their changes without compile-testing any extension behind aprovisionalflag, which is especially useful if we've already wrapped aprovisonalextension that is seeing breaking changes.One important feature that is missing in this PR and the reason for its draft state is that we don't yet wrap any types
<require>d by provisional changes. While these types are unlikely to be useful without enabling the extension, non-breaking releases with breaking changes to such types should be considered invalid. And there may be provisional extensions that solely consist of new types (extending existing types viapNext).Secondly, feature flags may not technically exempt us from semver compatibility in releases, even if the feature flag is documented as such. We could "do this anyway" for simplicity, or move to something more aggressive like
cfgdefinitions.