-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix sysgpu_backend build option #1264
Conversation
src/sysgpu/main.zig
Outdated
@@ -23,6 +17,24 @@ const impl = switch (backend_type) { | |||
var inited = false; | |||
var allocator: std.mem.Allocator = undefined; | |||
|
|||
fn backendFromBuildOption(comptime opt: build_options.@"build.SysgpuBackend") sysgpu.BackendType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being named build_options.@"build.SysgpuBackend"
looks weird and wrong to me.
We have other options in build_options
that don't have to be written that way, for example here you can see we have build_options.want_sysaudio
as a boolean: https://github.com/hexops/mach/blob/main/src/main.zig#L18
I think we do something similar for core.PlatformType
: https://github.com/hexops/mach/blob/main/build.zig#L48
Can you look into making it work similar to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the solution might be writing something like:
const backend_type: sysgpu.BackendType = switch(build_options.sysgpu_backend) {
// ... all the cases ...
};
And then don't have a backendFromBuildOption
function at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah that would be nicer. I've updated the PR. I've just learnt the .value
syntax so it didn't occur to me that you can avoid specifying the type entirely via inference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you for fixing this!
The sysgpu backend build option was treating the build option as the wrong enum type. This type is defined in
build.zig
and is exposed through the generatedbuild_options
struct. This is really nasty, but I think this is the correct way to do this.The generated code for
@import("build_options")
for reference: