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

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

Merged
merged 45 commits into from
Nov 5, 2021

Conversation

SekouDiaoNlp
Copy link
Collaborator

@SekouDiaoNlp SekouDiaoNlp commented Nov 2, 2021

Hi @Pebaz ,

As discussed previously, this PR adds the following changes.

  • Fixes issue '-d:lto' flag breaks macOS #51 by passing the 'd:lto' flag only if not on macOS.
  • Implements support for *.nim.cfg and *.nims files to specify Nim compiler flags. See discussion Add support for overriding default compiler flags with '.nim.cfg' and '.nims' configuration files #54 for details.
  • Removed Support for switches.py
  • Added tests for nim configuration files detection.
  • Show DeprecationWarning warning if switches.py is detected.
  • All tests passed. See
  • Updated README.md with information about the change to nim configuration files
  • Updated README.md with deprecation notice about switches.py
  • Refactor code in nimpoter.py and nimporter-cli.py to eliminate code duplication.
  • Add type annotations to nimprter.py (Use mypy for type checking)
  • Add pyproject.toml file as per PEP 517, PEP 518, PEP 621 and PEP 660. (Use poetry as build backend. Setup.py left intact so users of the library can still use setup.py).
  • Bumped version number to 1.1.0 to indicate deprecated functionality + new features.
  • BONUS: Amended the README section Source Distribution to fix error in specifying chosenim_install dependency. See Clarification request concerning choosenim_install #52.

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.

# Conflicts:
#	tests/test_distribution.py
@SekouDiaoNlp SekouDiaoNlp added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Nov 2, 2021
@SekouDiaoNlp SekouDiaoNlp self-assigned this Nov 2, 2021
@SekouDiaoNlp SekouDiaoNlp linked an issue Nov 3, 2021 that may be closed by this pull request
@SekouDiaoNlp
Copy link
Collaborator Author

SekouDiaoNlp commented Nov 3, 2021

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

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

@SekouDiaoNlp
Copy link
Collaborator Author

LGTM 👍🏿

@Pebaz
Copy link
Owner

Pebaz commented Nov 4, 2021

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

SekouDiaoNlp commented Nov 4, 2021

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 SekouDiaoNlp changed the title Fixed Issue with macOS build using 'd:lto' flag + Added support for *.nim.cfg and *.nims files to specify compiler flags. 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 Nov 4, 2021
Copy link
Owner

@Pebaz Pebaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SekouDiaoNlp I looked through all your changes. Absolutely amazing work! 🚀 This was a large refactor and I think you've done an exemplary job getting it done quickly with high quality. The changes that help to modernize Nimporter are awesome and helps to increase the longevity of the project as a whole.
Thanks for fixing these issues, have a fantastic day, and congrats on the big refactor! 😃🎉

@Pebaz Pebaz merged commit 6ef2d83 into master Nov 5, 2021
@SekouDiaoNlp
Copy link
Collaborator Author

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!

✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarification request concerning choosenim_install '-d:lto' flag breaks macOS
2 participants