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

Don't use serde default features (std) for spirv crate #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dinnerbone
Copy link

Despite spirv crate being no_std, it pulls in serde's std feature by accident. This fixes that, so it can be used safely in no_std crates

@MarijnS95
Copy link
Collaborator

I'm curious how this got to be (but git blame isn't readily available on mobile). Most likely the crate used to be no_std and serde integration was added at some later time?

In any case I don't think we should land this in isolation, but rather spend some time adding a no_std check to CI as well as the relevant clippy lints that guide contributors to use core primitives over std.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Dec 26, 2024

Looks like serde was added in #125, but the dependency is optional behind a non-default feature meaning that spirv is already no_std compliant unless the serialize or deserialize features are turned on.

To that effect, I think you should rewrite your pull request title and description to explain that you have tested and confirmed that iff the (de)serialize features are used, this crate could still be completely no_std-compatible while also providing serde support.

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