-
Notifications
You must be signed in to change notification settings - Fork 19
macos fixes and build script refactor #32
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! It's always great when I get feedback for platforms I can't personally test on.
As far as I can tell, QuartzCore
is mostly used for animation and windowing, and since neither the triangle example nor any of the tests in this repository spawn a window, it probably isn't needed here.
I do have a couple issues, mostly around what happens when you run tests or build on linux. Let me know if you need help or clarification, or if you just disagree with my suggestions; I'm always open to trying things a new way. Thanks again!
build.zig
Outdated
else => { | ||
if (link_mode == .static) { | ||
libwgpu_path = wgpu_dep.path("lib/libwgpu_native.a"); | ||
link_mac_frameworks(wgpu_mod); |
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.
On linux this causes the build to fail since it can't find the frameworks. I think it would also fail on mac if you were trying to run the compute tests since compute-c
would also need the frameworks. At this point it might be better to move the mac-specific stuff out into its own case, like
switch (target_res.os.tag) { // above on line 132
.windows => {
// ... all the windows stuff like it is currently
},
.macos => {
// all the mac-specific stuff, both for static and dynamic link modes
link_mac_frameworks(wgpu_mod);
link_mac_frameworks(wgpu_c_mod);
},
else => {
// All the stuff for linux/FreeBSD/etc, both for static and dynamic link modes;
// This would be pretty much like it was before, but without the "} else if (target_res.os.tag == .macos ..." branch
}
|
||
const run_test = b.addRunArtifact(t); | ||
|
||
if (context.is_mac) { |
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.
Do the unit tests still pass on mac with this removed? They don't use the wgpu module, so if you remove them here the test
step won't link against those frameworks.
}, | ||
.multisample = wgpu.MultisampleState {}, | ||
.primitive = wgpu.PrimitiveState{}, | ||
.fragment = &wgpu.FragmentState{ .module = shader_module, .entry_point = wgpu.StringView.fromSlice("fs_main"), .target_count = color_targets.len, .targets = color_targets.ptr }, |
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.
A bit of a nitpick, but I kind of prefer this line the way it was before; if you don't have a particularly high-resolution monitor this line wraps (or goes off the screen) when you write it like this.
examples/triangle/triangle.zig
Outdated
defer staging_buffer.unmap(); | ||
|
||
const output = buf[0..output_size]; | ||
std.fs.cwd().makeDir("examples/output") catch {}; |
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 prefer not to suppress all errors here; if this were to fail for some other reason than PathAlreadyExists
, you'd see an error show up in write24BitBMP()
instead and it makes tracing the actual source of the error a little more difficult. It seems that std.fs.Dir
has a makePath()
function that handles the case where the directory already exists, and returns any other type of error, so maybe you could do try std.fs.cwd().makePath("examples/output");
i will look into it, if i remember correctly the changes i did to the build where done that if you import the module in another zig project the right dependencies are linked in by default, i didnt check the other stuff, sloppy on my part, will do! |
sooo... this was a long endeavor. i found the build script a little bit convoluted, thats why i flatened it as much as is could
please let me know if something does not work on your platform (windows / linux?) i can later check on my windows machine, unfortunately i dont have a linux system rn |
Thanks! I appreciate all the hard work. |
yes, i totally feel the pain! thanks for checking, btw i heard something about a bad windows update which compromises specific SSD's |
pinging this because wgpu-native was updated and the PR is bitrotting... |
Q: do you know if Framework "QuartzCore" is really required? triangle test works without it
note: depending on system installed macos frameworks makes the build non portable aka you can not cross compile from another platform.