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

Update FindAEC and add to CI #145

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Update FindAEC and add to CI #145

merged 3 commits into from
Oct 29, 2024

Conversation

ChrisspyB
Copy link
Member

@ChrisspyB ChrisspyB commented Oct 24, 2024

Three changes:

  • Report dlerror on a failed dlopen call.
  • Update findAEC.cmake to be consistent with eccodes' findAEC.cmake. This searches more paths, and also looks for libaec.h rather than libsz.h
  • Add AEC to the CI dependencies so that it uses a recent AEC, rather than whatever happens to be on the runners' systems.

Context:
Downstream dependency gribjump generates a plugin that depends on libaec/1.1.1 which is not backwards compatible. However, if eckit is linked against the system AEC (which is generally quite old), this old AEC will be loaded before the AEC gribjump is linked against. As such, whenever eckit loads the gribjump plugin at runtime, there is a missing symbol error.

This PR updates the CI to build AEC from source, and updates the findAEC search path to find it. We also now throw an error in loadPlugin if loadDynamicLibrary returns nullptr. Without this, an incorrect error message is thrown instead (plugin object not registered).

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 63.75%. Comparing base (4550f54) to head (db99575).

Files with missing lines Patch % Lines
src/eckit/system/LibraryManager.cc 28.57% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #145      +/-   ##
===========================================
- Coverage    63.75%   63.75%   -0.01%     
===========================================
  Files         1066     1066              
  Lines        55345    55351       +6     
  Branches      4085     4086       +1     
===========================================
+ Hits         35286    35289       +3     
- Misses       20059    20062       +3     

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

@ChrisspyB
Copy link
Member Author

It might also be sensible to put findAEC.cmake in ecbuild, as there are now several packages with their own implementation

@wdeconinck
Copy link
Member

It might also be sensible to put findAEC.cmake in ecbuild, as there are now several packages with their own implementation

I agree that if this is goes into multiple packages then ecbuild could be a good common place.

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

It all looks good to me!

Copy link
Member

@iainrussell iainrussell left a 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 too!

@ChrisspyB
Copy link
Member Author

If you are happy, please go ahead and merge :)

@iainrussell iainrussell merged commit 9cd9faf into develop Oct 29, 2024
220 checks passed
@iainrussell iainrussell deleted the feature/findaec branch October 29, 2024 11:09
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.

4 participants