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

Add missing lal declarations #58

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Add missing lal declarations #58

merged 3 commits into from
Apr 29, 2024

Conversation

spxiwh
Copy link
Collaborator

@spxiwh spxiwh commented Apr 26, 2024

@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
Copy link
Member

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

spxiwh commented Apr 26, 2024

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

codecov-commenter commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage   55.36%   55.36%           
=======================================
  Files          10       10           
  Lines        1167     1167           
=======================================
  Hits          646      646           
  Misses        521      521           
Flag Coverage Δ
Linux ?
macOS 55.36% <ø> (ø)
python3.10 ?
python3.11 55.36% <ø> (ø)
python3.8 55.41% <ø> (ø)
python3.9 55.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@spxiwh
Copy link
Collaborator Author

spxiwh commented Apr 26, 2024

For now, can we merge this though :-)

@spxiwh
Copy link
Collaborator Author

spxiwh commented Apr 29, 2024

@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
Copy link
Member

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.

Copy link
Member

@duncanmmacleod duncanmmacleod left a comment

Choose a reason for hiding this comment

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

LGTM

@spxiwh spxiwh merged commit 3208313 into master Apr 29, 2024
23 checks passed
@spxiwh spxiwh deleted the pr_fix_undeclared_functions branch April 29, 2024 09:52
duncanmmacleod added a commit to duncanmmacleod/sbank that referenced this pull request Apr 29, 2024
duncanmmacleod added a commit to regro-cf-autotick-bot/sbank-feedstock that referenced this pull request Apr 29, 2024
use a proper branch to make it easy to track/rebase, and add patch to backport
gwastro/sbank#58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants