Pebaz / nimporter

Compile Nim Extensions for Python On Import!
MIT License
829 stars 33 forks source link

Fixed Issue with macOS build using 'd:lto' flag + Added support for `*.nim.cfg` and `*.nims` files to specify compiler flags. + Amended the README see #52 #55

Closed SekouDiaoNlp closed 2 years ago

SekouDiaoNlp commented 2 years ago

Hi @Pebaz ,

As discussed previously, this PR adds the following changes.

I also added a new workflow to test nimporter on more versions of Python and Nim. All tests pass on every system apart from nim version 1.5 because jiro4989/setup-nim-action fails to install this nim version through choose-nim. I checked their repository and did not see any open issue regarding this so I might open one myself while I keep investigating.

Waiting for your feedback.

Peace.

SekouDiaoNlp.

Pebaz commented 2 years ago

Hello @SekouDiaoNlp, fantastic work with this!

I'm cool with you taking the extra couple days before the release, that way it'll all be in the PR together.

Thanks so much!

🐝 Pebaz

SekouDiaoNlp commented 2 years ago

Hello @SekouDiaoNlp, fantastic work with this!

I'm cool with you taking the extra couple days before the release, that way it'll all be in the PR together.

Thanks so much!

🐝 Pebaz

Cool I'll go ahead then! You are welcome.

SekouDiaoNlp commented 2 years ago

Hi @Pebaz,

what's up?

I am done implementing all the changes and testing them. I updated the first post with all the changes made in this PR.

Fixes issues #51 and #52.

I completely refactored the code in NimCompiler.compile_nim_code()and NimCompiler.compile_nim_extension(). I also deprecated the use switches.py and added support for nim configuration files. There are new tests covering the new behaviour.

On the developer side, I fully type annotated nimporter.py and added a fully functional and PEP compliant pyproject.toml. setup.py is still fully supported, so there is no change for users who prefer to stick with setup.py, but other developers can use their preferred building pipeline now.

The README has been updated to reflect all the changes.

I'll monitor the progress of the worflow checks (I guess I went a bit overboard with the number of jobs in the matrix on the last tweak to the workflows lol. I will trim the amount of jobs testing the pyproject.toml stuff)

Let me know what you think.

SekouDiaoNlp.

SekouDiaoNlp commented 2 years ago

I trimmed the number of poetry builds to a more reasonable number.

SekouDiaoNlp commented 2 years ago

LGTM πŸ‘πŸΏ

Pebaz commented 2 years ago

This is fantastic work for real. I will do a code review either tonight or tomorrow! Thanks again! I'll be in touch once I get a chance to look more into the finished changes

SekouDiaoNlp commented 2 years ago

This is fantastic work for real. I will do a code review either tonight or tomorrow! Thanks again! I'll be in touch once I get a chance to look more into the finished changes

Thank you for the kind words. Take your time and keep me updated.

Peace.

SekouDiaoNlp commented 2 years ago

Hey @Pebaz! Thank you for your support!

I am glad that my work was useful and is valued.

I have had a few ideas for new features after I delved deep in nimporter.

I will write a post in the discussion section of the repository to share them with you beginning of next week.

And it's cool to be able to contribute back to a project that is useful to me in various projects.

Have a great evening!

✌️