search

mashingan / anonimongo

Another Nim pure Mongo DB driver
MIT License
43 stars 5 forks source link

adding gcsafe pragmas for findOne to work #29

Closed samsamros closed 11 months ago

samsamros commented 1 year ago

Adding gcsafe pragma to several procs, including the ones referenced in #28 , this resolves the issue for findOne in #27 , update is still pending testing!

mashingan commented 1 year ago

Can we try to use push-pop pragma? i.e {.push: gcsafe.} and {.pop.}. Perhaps it could make the changes easier and simpler.

samsamros commented 1 year ago

I was unaware of this use {.push: gcsafe.} I will read more, but I'm happy with a simpler solution than mine :D I'll do my homework in the meantime. I'll leave the gcsafedevelop branch just in case, to map the places that needed the pragma.

ok, did my homework. Are you thinking of applying this on several modules, all procs? (avoiding templates of course). What would be the strategy? sounds good, just want to learn what would be the best way to approach it on all the hard work you already put in place on this project.

mashingan commented 1 year ago

Perfectly fine with both direction, the push-pop pragma or manually adding the gcsafe pragma to all functions/procs. Push-pop pragma is easier but I'm unaware if anything'd break by applying it. I personally prefer the push-pop pragma direction, but as I said, both perfectly fine for me. After you decide which is better, you can merge this :+1:

inv2004 commented 1 year ago

I would vote for non-push/pop because I am a bit afraid of the push/pop solution - theoretically it sounds correct, but, and like you said, - it is more safe to apply the effect to minimum possible code. In my case it was just two functions

samsamros commented 1 year ago

this is a really good point, and would like to run some tests first. I think this is working with update and findOne, but will try the {.push.} pragma. I'm guessing we have to exclude templates and then write {.push.} again afterwards? If this is only affecting a few procs, I would just add the {.gcsafe.} to map them, but if this is affecting much more of the code, {.push.} seems easier to maintain, but just a guess, because I havent tested yet.

mashingan commented 1 year ago

I'm guessing we have to exclude templates and then write {.push.} again afterwards?

Not sure whether template is safe or not, but if you want to avoid interweaving push-pop when finding templates, you can move the template definitions to above so inside the {.push gcsafe.} and {.pop} are only procs.

samsamros commented 1 year ago

I will test this! So far, with this, find, findOne, and update work fine. So it seems it's only these procs that were causing #27

samsamros commented 12 months ago

I've been revewing this, and I think there are very few procs that will cause the compile time error to go off for CRUD procs. I think we should just keep {.gcsafe.} for these, and continue to move forward. If more procs require the pragma I think we should continue development with {.push.} {.pop.}.

If you agree I will continue with the merge. I've been testing 2 weeks on my applications, no errors in compile time have showed up!

inv2004 commented 11 months ago

@samsamros just merge it. It would not make things worse than we have now :)

danbarbarito commented 8 months ago

I just spent an hour trying to figure out why my code wasn't working even though I saw this was fixed in this PR.

For anyone else who is confused, this change has not made it to a new version of the package yet so I had to change my dependencies to reference the HEAD of this Github repo. ie: requires "https://github.com/mashingan/anonimongo#head"

samsamros commented 8 months ago

Hi @danbarbarito , the latest version includes these changes. I've faced this issue before, and I've had to uninstall the package with nimble nimble uninstall anonimongo, and if I had just compiled the code, Nim caches C code in linux in ${HOME}/.cache/nim/*. You can safely remove cached C code for your compiled code with rm. (just search for the project name first). Then you can install with nimble again, and that should get it done.

We should, however, bump the version to avoid conflict with existing versions. I'm opening an issue to do this next.

edit

The version has been updated to v0.7.1 . You should be able to install just by prompting nimble install anonimongo No further conflicts between nimble cached versions and code updated in this pull request.

danbarbarito commented 8 months ago

Thank you @samsamros!