samdze / playdate-nim

Nim bindings with extra features for the Playdate SDK
66 stars 3 forks source link

Nim 2 support #58

Closed Nycto closed 2 months ago

Nycto commented 9 months ago

This change refactors the build scripts a bit to support building on both 1.6.14 and 2.0.2, and adds support for building against Nim 2. The only thing that really needed to change was switch("define", "useMalloc")

samdze commented 8 months ago

Looks good! I'll test this soon. Regarding the name of the setup task, I'm not sure whether playdateSetup would be the best choice, maybe we can find a better one? initialize, configure, organize? Let me know what you think

Nycto commented 8 months ago

I'm good with configure. I'll update the PR

samdze commented 8 months ago

The README can also be updated replacing the corresponding line in the prerequisites

Nycto commented 8 months ago

I think Ive addressed all the feedback whenever you are ready to take another look

samdze commented 8 months ago

I'm going to test this again, one thing I noticed last time was that the clean task is now shadowed by nimble clean in Nimble 0.14+. Don't know if we can override that.

Nycto commented 8 months ago

I think this has a merge conflict with the other PR I submitted, so I’ll need to resolve that

Nycto commented 8 months ago

At this point, all the tangible changes to the actual configuration were done as part of #60, so this PR is really just about updating the github workflows to build against both Nim 1.6 and Nim 2

samdze commented 8 months ago

Works on macOS, I just get a bunch of warnings (-Wmaybe-uninitialized) building for device, probably due to how Nim transpiles to C using gotos which makes the instruction flow not predictable by the compiler. Might be worth to add a flag to ignore these warnings.

Other than that, the nimble clean task no longer works, it is being shadowed by the built-in nimble clean in Nimble 0.14+. Not sure if we can customize it in some way, otherwise we are forced to change this task name too.

Nycto commented 8 months ago

Updated to fix the clean conflict

samdze commented 8 months ago

Does it work on your setup? Calling nimble clean (on v0.14.2) having our clean routine inside before clean does nothing in my case. May be a Nimble bug?

samdze commented 8 months ago

Also I noticed a previous action run on the same PR generated an error: https://github.com/samdze/playdate-nim/actions/runs/8352993144/job/22880677174?pr=58

Don't know what happened, hopefully not a random memory corruption issue

Nycto commented 8 months ago

I’ll poke at the clean task some more this morning. I didn’t check the version of nimble I was using, I only flipped between Nim 1.6 and 2.

The build failure has been happening for a few months, so it’s unrelated to any recent changes. I was able to repro it in valgrind at one point, which pointed to a null access in the simulator itself, so I didn’t dig in much further. We can probably add a retry to that workflow and ignore it.

samdze commented 8 months ago

If this causes too much trouble right now, let's just rename our clean routine to one of cleanup, wipe, clear, etc.

Nycto commented 8 months ago

No trouble — I just ran out of time this week. I’ll try to find some hours to look at this again this weekend

samdze commented 7 months ago

I didn't investigate, but at first glance it seems that before and after hooks don't work for setup and clean tasks, which is unfortunate.

Let me know whether you'd like to address the clean task issue or just use a different name. In the latter case, I can just merge this PR and change the task name afterwards.

Nycto commented 7 months ago

Apologies for not getting back to this. Technically, Nim 2 works at this point because of the memory management changes we made, so this hasn’t been high on my priority list

samdze commented 2 months ago

Thank you Nycto. Finally merged this!