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

Derive serialization for the types in spirv-headers #125

Merged
merged 2 commits into from
Apr 20, 2020
Merged

Derive serialization for the types in spirv-headers #125

merged 2 commits into from
Apr 20, 2020

Conversation

alisa101rs
Copy link
Contributor

Based on #123

Added serde as a optional dependency in spirv-header,
and feature gated derive for all enums and structs.

Added serde as a optional dependency in spirv-header,
and feature gated derive for all enums and structs.
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Nice!
The tricky thing here is if a client wants only one side of it (i.e. serialization but not deserialization), they wouldn't be able to do it, and we wouldn't be able to release a breaking version because spirv-headers tries to follow semver of spirv itself...
FYI, wanting only serialization is a thing that, for example, Firefox uses for some of the logic in release, because deserialization just takes so much more time to compile :(

@kvark kvark requested a review from antiagainst March 26, 2020 19:53
@kvark
Copy link
Member

kvark commented Mar 26, 2020

@antiagainst could you take a look please?

Added 2 separate features for Serialize and Deserialize,
also added Cargo.lock changes.
@alisa101rs
Copy link
Contributor Author

@kvark Is it good now?

@kvark
Copy link
Member

kvark commented Mar 27, 2020

Yes, that's more future-proof, thanks! Please make sure the path is CI tested.
Also, I really want @antiagainst to look at this before merging.

@alisa101rs
Copy link
Contributor Author

@kvark I am not sure that error in CI is caused by my commits.

@kvark
Copy link
Member

kvark commented Mar 27, 2020

indeed! filed #126

Copy link
Collaborator

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

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

Cool, thanks! LGTM. :)

[lib]
path = "lib.rs"

[dependencies]
bitflags = "1"
num-traits = "0.2"
serde = {version = "1", optional = true, features = ["derive"]}
Copy link
Member

Choose a reason for hiding this comment

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

note that this isn't correct, feature should be serde_derive, or the version needs to be more specific

@kvark
Copy link
Member

kvark commented May 1, 2020

@antiagainst how do we get this out for users?

@antiagainst
Copy link
Collaborator

@kvark: I see we might want more breaking changes along the way so it's better to sort out the versioning issue. I'm thinking maybe it's better to retire the current spirv_header crate and instead publish to spirv crate so that we have a chance to detach the spec version and the crate version. @neon64 asked me previously in #21 but back then we don't have a clear need to make it worthwhile. Now maybe its the time.

@kvark
Copy link
Member

kvark commented May 2, 2020

I haven't given it a lot of thought, tbh. It sounded to me, based on the previous discussions, that relying on upstream Spec semantic versioning can be a problem. In this case, yes, we have to detach it. Do you own "spirv" crate now? Sounds like a good plan :)

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

3 participants