0.5.x doesn't compile anymore

[MaikKlein]

   Compiling rspirv v0.5.4
error[E0599]: no variant or associated item named `DecorateStringGOOGLE` found for type `spirv::Op` in the current scope
   --> /home/maik/.cargo/registry/src/github.com-1ecc6299db9ec823/rspirv-0.5.4/grammar/table.rs:433:11
    |
433 |     inst!(DecorateStringGOOGLE, [], [(IdRef, One), (Decoration, One)]),
    |           ^^^^^^^^^^^^^^^^^^^^
    |           |
    |           variant or associated item not found in `spirv::Op`
    |           help: there is a variant with a similar name: `DecorateString`

error[E0599]: no variant or associated item named `DecorateStringGOOGLE` found for type `spirv::Op` in the current scope
  --> /home/maik/.cargo/registry/src/github.com-1ecc6299db9ec823/rspirv-0.5.4/grammar/reflect.rs:53:20
   |
53 |         spirv::Op::DecorateStringGOOGLE |
   |                    ^^^^^^^^^^^^^^^^^^^^
   |                    |
   |                    variant or associated item not found in `spirv::Op`
   |                    help: there is a variant with a similar name: `DecorateString`

error[E0308]: mismatched types
   --> /home/maik/.cargo/registry/src/github.com-1ecc6299db9ec823/rspirv-0.5.4/mr/constructs.rs:190:22
    |
190 |             version: (spirv::MAJOR_VERSION << 16) | (spirv::MINOR_VERSION << 8),
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found u8

error[E0599]: no variant or associated item named `DecorateStringGOOGLE` found for type `spirv::Op` in the current scope
  --> /home/maik/.cargo/registry/src/github.com-1ecc6299db9ec823/rspirv-0.5.4/mr/build_annotation.rs:62:56
   |
62 |         let mut inst = mr::Instruction::new(spirv::Op::DecorateStringGOOGLE, None, None, vec![mr::Operand::IdRef(target), mr::Operand::Decoration(decoration)]);
   |                                                        ^^^^^^^^^^^^^^^^^^^^
   |                                                        |
   |                                                        variant or associated item not found in `spirv::Op`
   |                                                        help: there is a variant with a similar name: `DecorateString`

error: aborting due to 4 previous errors

There was a breaking change in spirv_headers 1.3 to 1.4 where DecorateStringGOOGLE got renamed.

Temporary workaround is to fix the cargo.lock file to point to 1.3.4 instead.

If instead we had specified the version string as ^1.0, cargo should update to 1.1 if it is the latest 1.y release, but not 2.0

[MaikKlein]

Might be safer to always do a major version increment for a new release, and do something like 2.0+1.5.

[Jasper-Bekkers]

My PR removed Op::DecorateStringGOOGLE in favor of OpDecorateString because they are defined in an enum and have identical values.

[Skepfyr]

So, I'm pretty sure, we should technically be yanking the version of spirv_headers as we just broke semver, however that puts us in a tight spot around the fact that the API version matches the crate version. This can't be true any more, so I think we need to come up with a better versioning scheme for spirv_headers. I can easily see cases where we want to update the crate of an older SPIR-V version to add new features, so the best path probably needs to involve a way of letting us update an arbitrary API version, possibly all at once as the feature changes are likely to be shared between API versions. I'm tempted to suggest feature gating any modifications to the API but that could become unruly very quickly, another option I can see is releasing different crates for different API versions but that has discoverability downsides. Does anyone have any thoughts/suggestions? @gfx-rs/translators

[Jasper-Bekkers]

So, I'm pretty sure, we should technically be yanking the version of spirv_headers as we just broke semver

The other alternative is to not try to make it work with semver to begin with since upstream can't guarantee it either and recommend strict versioning for this crate instead.

[Skepfyr]

Unfortunately, that's not an option, Cargo and anyone using the library will assume semver, that's not a contract we can break.
I asked around a bit and another suggested solution is to have a different module for each version, possibly elevating (i.e reexporting it at root) the latest version to make updates more normal.

[kvark]

@Skepfyr we need to yank the breaking version until a solution is in place...
@antiagainst could you add me to crates.io owners of the crates?

[antiagainst]

Thanks for the report @MaikKlein and sorry for the delay!

It's a common pattern in SPIR-V to promote a certain symbol into core by dropping the suffix and we should maintain both symbols in the spirv_headers and map to the same value. I've been doing that for other symbols like SubgroupEqMask vs SubgroupEqMaskKHR in BuiltIn via associated constants. Updating spirv_headers from 1.3 to 1.4 only introduces more symbols and should surely not break. So yes this is a bug we should fix.

Regarding the version, I still prefer to stay with the current version correspondance because of the simplicity and also we are already on this route. As said in the previous section it should not be a problem to update from 1.3 to 1.4 to 1.5 or whatsoever given just pulling in more symbols. It's unfortunate that we break the correspondance for 1.4.1. But AFAICT, APIs specify major and minor versions when picking which SPIR-V to support. (Vulkan 1.0 supports SPIR-V 1.0, Vulkan 1.1 supports 1.3, etc.) And I expect spirv_headers users to do the same generally. The revision number (the patch number in semver's term) does not matter that much. I'm fine to detach the patch number from SPIR-V spec and use it for patches of the crate (which should really happen very rare). And make the expectatation straight by put it in the doc of spirv_headers.

How does this sound? :)

Besides, is someboday willing to work on a fix to use associated constants to provide the missing symbols? :)

[antiagainst]

Yanked 1.4.1 for now.

[Skepfyr]

I had a look around and I couldn't find the SPIR-V versioning scheme, if someone could find this it would be really helpful. It is very possible they aren't following semver, which could bite us badly if a change comes that breaks that promise. I agree that the current scheme is the simplest, although the unified spec may hint that we should be approaching this differently. However, my main issue with the current approach is that we can't make changes to the old specs, they are essentially set in stone and adding new bits of information to them, eg operand number/types is impossible.

[antiagainst]

I'm working on fixing this bug by generating associated constants for the duplicated op symbols now.