-
Notifications
You must be signed in to change notification settings - Fork 40
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
Refactor setup.py to align with CF. #450
Conversation
Codecov Report
@@ Coverage Diff @@
## master #450 +/- ##
==========================================
+ Coverage 99.76% 99.82% +0.06%
==========================================
Files 42 50 +8
Lines 3811 4667 +856
==========================================
+ Hits 3802 4659 +857
+ Misses 9 8 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions.
Co-authored-by: Amintor Dusko <[email protected]>
Co-authored-by: Amintor Dusko <[email protected]>
Co-authored-by: Amintor Dusko <[email protected]>
Co-authored-by: Amintor Dusko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vincentmr
Just a few thoughts and comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vincentmr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
tests
directory!All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs
.Ensure that the test suite passes, by running
make test
.Add a new entry to the
.github/CHANGELOG.md
file, summarizing thechange, and including a link back to the PR.
Ensure that code is properly formatted by running
make format
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
The way
setup.py
is written, it tries to find binaries in the environment (for MacOS in particular) and it may override some CMake decisions which would actually make sense in some build environments (e.g. on Conda-Forge). This is trying to apply this patch and a bit more.Description of the Change:
Remove (most of ) CMake argument parsing from
setup.py
. Adapt CI files and useenv:
when possible to set environment variables.Benefits:
Where necessary, libraries and default build tools are defined declaratively in the Actions files.
setup.py
is simplified and any CMake argument can be passed inCMAKE_ARGS
if required. This complies better with Conda-Forge, and hence we'll be able to remove some patches there.Possible Drawbacks:
Building from source may change a bit for users used to do so.
Related GitHub Issues:
PR 58