ziggy-pydust icon indicating copy to clipboard operation
ziggy-pydust copied to clipboard

Restore `zig build` for 0.13

Open jburgy opened this issue 9 months ago • 5 comments

jburgy avatar Feb 26 '25 14:02 jburgy

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 26 '25 14:02 CLAassistant

@jburgy WoW! Nice comptime tricks in the src/discovery.zig. I am curious whether the deeply nested function calls at comptime may require bumping up the branch quota with @setEvalBranchQuota especially when binding large zig modules to python.

slonik-az avatar Mar 31 '25 13:03 slonik-az

@gatesn @lwwmanning @delta003 @robert3005 as the members of Spiral listed on this repository, would one of you consider reviewing this PR?

jburgy avatar Apr 02 '25 19:04 jburgy

Will make sure to have a look before EOW, likely during the weekend

robert3005 avatar Apr 03 '25 05:04 robert3005

@robert3005 I deleted commented out code or confirmed your understanding and closed the corresponding conversations. I left explanations without closing the remaining ones so they stand out, please take another look. Here are my replies to your overall comment:

Thanks a lot for the pr. I went through the type discovery and registration as that is the only part that actually changes where the rest is just api adjustments for that change. I think it's overly verbose for the user to explicitly pass the parent type every time. I wonder if we can change signature of the definition methods like py.class, etc. to include the parent type.

I tried several variations but this is the only one that worked. Other attempts either suffered "circular dependency" or "closure capture of comptime var" errors.

For all the builtin types I'm not sure we need to pass root, i.e. does PyString require handle to the current state? It's a bit unfortunate that we must return new types now instead of rewriting existing types but that's zigs design choice. It would be cool if you could mutate types in comptime

Passing root to builtin types is required by the many calls to tramp.Trampoline in src/conversions.zig and src/pytypes.zig. I agree that it makes for an awkward API but that's the only permutation I could find.

jburgy avatar Apr 14 '25 12:04 jburgy