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

Nim 2 support #58

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Nim 2 support #58

merged 4 commits into from
Oct 1, 2024

Conversation

Nycto
Copy link
Collaborator

@Nycto Nycto commented Mar 2, 2024

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")

It conflicts with a built in nimble task
@samdze
Copy link
Owner

samdze commented Mar 15, 2024

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
Copy link
Collaborator Author

Nycto commented Mar 16, 2024

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

@samdze
Copy link
Owner

samdze commented Mar 16, 2024

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

@Nycto
Copy link
Collaborator Author

Nycto commented Mar 16, 2024

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

@samdze
Copy link
Owner

samdze commented Mar 17, 2024

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
Copy link
Collaborator Author

Nycto commented Mar 17, 2024

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

@Nycto
Copy link
Collaborator Author

Nycto commented Mar 17, 2024

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
Copy link
Owner

samdze commented Mar 17, 2024

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 added 3 commits March 19, 2024 18:43
This allows the default clean task to run, while adding
custom behavior specifically for playdate cleaning
@Nycto
Copy link
Collaborator Author

Nycto commented Mar 20, 2024

Updated to fix the clean conflict

@samdze
Copy link
Owner

samdze commented Mar 20, 2024

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
Copy link
Owner

samdze commented Mar 20, 2024

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
Copy link
Collaborator Author

Nycto commented Mar 20, 2024

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
Copy link
Owner

samdze commented Mar 21, 2024

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

@Nycto
Copy link
Collaborator Author

Nycto commented Mar 22, 2024

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
Copy link
Owner

samdze commented Apr 11, 2024

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
Copy link
Collaborator Author

Nycto commented Apr 15, 2024

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 samdze merged commit 4adb2c5 into samdze:main Oct 1, 2024
8 checks passed
@samdze
Copy link
Owner

samdze commented Oct 1, 2024

Thank you Nycto. Finally merged this!

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