gwastro / sbank

GNU General Public License v2.0
4 stars 14 forks source link

Add missing lal declarations #58

Closed spxiwh closed 4 months ago

spxiwh commented 4 months ago

@duncanmmacleod Reported that sbank is failing to build on newest clang releases due to missing declarations. This adds the missing declarations (hopefully). Can you approve (if this passes tests etc.)?

@duncanmmacleod A related request to you: I shouldn't have to be doing this! As with numpy, there should be some way to find lalsuite's .h files, so I can just include them. I also need a way to find lalsuite's so files to link against, which doesn't require the horrible hack that sbank's packaging currently has. Adding these would greatly increase the ability to use Cython with lalsuite (this is better than SWIG because I can interact directly at the C layer).

duncanmmacleod commented 4 months ago

@duncanmmacleod A related request to you: I shouldn't have to be doing this! As with numpy, there should be some way to find lalsuite's .h files, so I can just include them. I also need a way to find lalsuite's so files to link against, which doesn't require the horrible hack that sbank's packaging currently has. Adding these would greatly increase the ability to use Cython with lalsuite (this is better than SWIG because I can interact directly at the C layer).

LAL provides a pkg-config (.pc) file in (almost?) all distributions which can be queried to find the include directory, library directorys, library names, etc, e.g:

$ pkg-config --cflags --libs lal
-pthread -I/home/duncan/opt/mambaforge/envs/py311/include -L/home/duncan/opt/mambaforge/envs/py311/lib -llal
spxiwh commented 4 months ago

Hmm, okay ... It's getting late here now, but I'll try and demonstrate the failure I see when building sbank directly linking to LAL (if it still exists ... It's been some time since I tried this).

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 55.36%. Comparing base (de73773) to head (627ffd2).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #58 +/- ## ======================================= Coverage 55.36% 55.36% ======================================= Files 10 10 Lines 1167 1167 ======================================= Hits 646 646 Misses 521 521 ``` | [Flag](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [Linux](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `?` | | | [macOS](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `55.36% <ø> (ø)` | | | [python3.10](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `?` | | | [python3.11](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `55.36% <ø> (ø)` | | | [python3.8](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `55.41% <ø> (ø)` | | | [python3.9](https://app.codecov.io/gh/gwastro/sbank/pull/58/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `55.41% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

spxiwh commented 4 months ago

For now, can we merge this though :-)

spxiwh commented 4 months ago

@duncanmmacleod LAL does not provide the pkg-config file in pypi builds. This causes issues when trying to build pypi wheels. We use the pypi lalsuite wheels to build sbank's pypi wheels (which makes the most sense in terms of consistency), but linking against it becomes difficult when we don't know where to find either the libraries, or the header files.

duncanmmacleod commented 4 months ago

Ok, good point, I wasn't sure how the wheels for this project were built.

@lpsinger might have some suggestions on the best way to solve this problem, otherwise I think it's a reasonable feature request to have the LAL Python library provide a get_library_{dir,name} or similar functions.