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

Changes to support generating the sokol-fetch bindings in zig #1048

Merged
merged 3 commits into from
May 15, 2024

Conversation

trace-andreason
Copy link
Contributor

Related to this issue: floooh/sokol-zig#63. Not ready to be merged, just thought it might be helpful for that discussion

@trace-andreason trace-andreason marked this pull request as draft May 14, 2024 04:32
@floooh
Copy link
Owner

floooh commented May 14, 2024

...that fetch callback should actually be handled here:

sokol/bindgen/gen_zig.py

Lines 339 to 340 in f70cc07

elif util.is_func_ptr(field_type):
l(f" {field_name}: ?*const fn ({funcptr_args_c(field_type, prefix)}) callconv(.C) {funcptr_result_c(field_type)} = null,")

...I'm currently investigating why it isn't...

@floooh
Copy link
Owner

floooh commented May 14, 2024

Aha, because it's a typedef in the sokol_fetch.h header, and that's not detected in the bindings generation script.

If I remove the sfetch_callback_t typedef from sokol_fetch.h and replace the callback declaration in sfetch_request_t to this:

typedef struct sfetch_request_t {
    ...
    void (*callback) (const sfetch_response_t*);
    ...
} sfetch_request_t;

...it works, and generates the following Zig struct:

pub const Request = extern struct {
    channel: u32 = 0,
    path: [*c]const u8 = null,
    callback: ?*const fn ([*c]const Response) callconv(.C) void = null,
    chunk_size: u32 = 0,
    buffer: Range = .{},
    user_data: Range = .{},
};

...can you integrate this sokol_fetch.h change in your PR and remove your "manual exception hack"?

@trace-andreason
Copy link
Contributor Author

yep, sounds good! Are you cool with these overloaded names?

    'sfetch_continue':                      'fetch_continue', # 'fetch' is reserved in Zig
    'sfetch_desc':                          'description'     # 'desc' shadowed by earlier definiton

fetch_continue might be weird because it'll probably end up being fetch.fetch_continue() in zig...

@trace-andreason trace-andreason marked this pull request as ready for review May 14, 2024 16:58
@trace-andreason
Copy link
Contributor Author

made those changes

@floooh
Copy link
Owner

floooh commented May 14, 2024

...for the sfetch_desc override I would actually prefer sfetch_get_desc which would then end up as fetch.getDesc() on the Zig side.

For continue maybe resume? Since it's "resuming from paused state".

...tbh, sfetch_resume() would also be a good change in the C header.

@floooh
Copy link
Owner

floooh commented May 14, 2024

...one other change I'd like is to restrict sokol_fetch.h to the Zig bindings for now, e.g. remove the sokol_fetch item from the tasks array in gen_all.py, and instead do this down in the gen_zig block:

zig_tasks = [
    *tasks,
    [ '../sokol_fetch.h', 'sfetch_', [] ],
]
for task in zig_tasks:
   ...

...should have recommended this from the start but didn't think of the other language bindings...

@trace-andreason
Copy link
Contributor Author

turns out resume is also reserved

@trace-andreason
Copy link
Contributor Author

So I'll give you my 2 cents on naming here, coming from a beginner and copying examples written in C to zig. If we made continue be continue_fetching or something that starts with continue, you will at least get some editor help when just typing in the name of the c function.

@trace-andreason
Copy link
Contributor Author

I went ahead and named it continue_fetching, but open to any other suggestion. All other comments are incorporated.

@floooh
Copy link
Owner

floooh commented May 14, 2024

Yeah continue_fetching is fine :)

@trace-andreason
Copy link
Contributor Author

bindings build step is going to fail until sokol_fetch.c file is added to the repo

@floooh
Copy link
Owner

floooh commented May 15, 2024

Yep, I think I can take over from here. I'll also need to do a quick check if the dropped typedef breaks anything (I don't think so, but technically it's a breaking change). I'll try to look into it later today.

@floooh
Copy link
Owner

floooh commented May 15, 2024

Zig master might have had a breaking change in LazyPath. I'm looking into it.

PS: yep, I'll check what's needed to fix sokol-zig (and park the current zig-0.12.0 compatible version in a branch)

@floooh
Copy link
Owner

floooh commented May 15, 2024

Ok, took a bit longer then I had hoped, but the sokol-zig build.zig is now fixed for the current zig head version.

PS: the new D bindings suffer from the same problem because that also uses Zig as build system, but let's ignore that for now ;)

@floooh
Copy link
Owner

floooh commented May 15, 2024

I'll go ahead and merge this PR.

The next step would then be in sokol-zig again to add the C source file to build.zig here:

https://github.com/floooh/sokol-zig/blob/7d569775df73d1e869a1df12dfc647e56c7cb77a/build.zig#L265

...and the fetch.zig module here:

https://github.com/floooh/sokol-zig/blob/7d569775df73d1e869a1df12dfc647e56c7cb77a/src/sokol/sokol.zig#L9

Ideally we would also have a new sample, but I can do that at a later time.

@floooh floooh merged commit 8b747dd into floooh:master May 15, 2024
30 of 33 checks passed
floooh added a commit that referenced this pull request May 15, 2024
@floooh
Copy link
Owner

floooh commented May 15, 2024

...also added a little blurb to the CHANGELOG about the theoretically breaking change in sokol_fetch.h

@trace-andreason
Copy link
Contributor Author

next steps for sokol-zig: https://github.com/floooh/sokol-zig/pull/67/files

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