-
Notifications
You must be signed in to change notification settings - Fork 368
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
Leverage pkg-config in setup.py #498
Conversation
1e9ca44
to
cf21ca3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
==========================================
- Coverage 92.34% 92.20% -0.14%
==========================================
Files 91 91
Lines 11033 11033
Branches 1524 1524
==========================================
- Hits 10188 10173 -15
- Misses 840 855 +15
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cf21ca3
to
7804a11
Compare
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 except for one nit:
@@ -46,7 +47,7 @@ jobs: | |||
python3.10-dbg | |||
- name: Install Python dependencies | |||
run: | | |||
python3 -m pip install --upgrade pip cython | |||
python3 -m pip install --upgrade pip cython pkgconfig |
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.
I don't think this change should be needed. make test-install
runs
$(ENV) CYTHON_TEST_MACROS=1 $(PIP_INSTALL) -e .[test]
and that pip install will use the build dependencies specified in the pyproject.toml
. We need pip
here so that we can pip install
, and I believe we need cython
for its coverage plugin, not for the package build.
python3 -m pip install --upgrade pip cython pkgconfig | |
python3 -m pip install --upgrade pip cython |
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.
Nope, I'm wrong, we do need it. Not for the make test-install
, but because later we do make ccoverage
, which does:
CFLAGS=" -O0 -pg --coverage" make build
and that needs it.
Sorry for the bad advice!
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.
LGTM
3e3b517
to
7804a11
Compare
Using pkg-config we can get automatically the build flags in MacOS and Linux and this doesn't require the user to manually modify CFLAGS just to build the extension. Signed-off-by: Pablo Galindo <pablogsal@gmail.com>
7804a11
to
c7a0573
Compare
Using pkg-config we can get automatically the build flags in MacOS
and Linux and this doesn't require the user to manually modify CFLAGS
just to build the extension.
Signed-off-by: Pablo Galindo pablogsal@gmail.com