Skip to content

Add compile_spirv proc macro to replace build.rs #592

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

Closed

Conversation

Hentropy
Copy link
Contributor

Adds a proc macro that builds and executes a SpirvBuilder at compile time. This replaces the build.rs use-case of SpirvBuilder, but does not replace using SpirvBuilder directly if runtime shader compilation is desired. The macro returns the result of SpirvBuilder::build(), ie. the path to the .spv file.

Syntax:

compile_spirv! {
    // required
    path: "path/to/crate",
    // optional, defaults to 1.3
    version: 1.5,
    // optional, defaults to Vulkan
    memory_model: GLSL450,
    // either...
    debug,
    // ...or, default is release
    release,
}

@Hentropy
Copy link
Contributor Author

This will unblock #423

@lachlansneff
Copy link

Does this work with emitting a spirv file for each entry point?

@Hentropy
Copy link
Contributor Author

No, though it could be extended to do so. Another useful extension would be to have it return the spirv directly, instead of a path to a .spv file.

@XAMPPRocky
Copy link
Member

Thank you for your PR! Can you update this to have the same options as the current build.rs version? E.g. Remove the version and memory_model arguments, and add the target and multi_module arguments?

Also I think the parsing logic for the arguments is a bit too overly complex in terms of the amount of duplicated code, I would use some macros to clean up the validation logic. You can see similar logic in my image macro PR.

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

We should discuss this in the next meeting, this is a pretty major change with large tradeoffs and should go through at least the MCP process (or even an RFC, tbh) before being approved. Doing a request changes to make sure it's blocked before that happens.

@khyperia khyperia added the mcp: proposed A major change to the compiler, that hasn't yet been approved. label Apr 13, 2021
@fu5ha fu5ha removed their request for review April 22, 2021 01:14
@Hentropy
Copy link
Contributor Author

Closing, unclear how #423 will end up working/if it will.

@Hentropy Hentropy closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mcp: proposed A major change to the compiler, that hasn't yet been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants