Skip to content

Now finds libaec with find_package#73

Closed
Ozaq wants to merge 1 commit intodevelopfrom
feature/remove-find-aec
Closed

Now finds libaec with find_package#73
Ozaq wants to merge 1 commit intodevelopfrom
feature/remove-find-aec

Conversation

@Ozaq
Copy link
Copy Markdown
Member

@Ozaq Ozaq commented Sep 29, 2025

Description

Now finds libaec from libaec-config.cmake

This is in preparation to move libaec to be a source dependency in
cxx-dependencies where we will be providing libaec 1.1.4.

This is also part of bump of libaec to 1.0.6 across the whole C++ stack.
With 1.0.6 libaec introduced cmae config files that replace FindModules.
Unfortunately the cmake config file for libaec 1.0.6 to 1.1.3 do not
work correctly if libaec is not installed system wide in combination
with a system wide installed version of libaec. The bug is that the
configuration file just picks up the first libaec.h file and the first
libaec.so. Which in this combination will point to the system wide
installed version.

This can (and is addressed in this PR) by hinting cmake to the correct
location of the loaded aec module on ATOS.

When using CMAKE_PREFIX_PATH this is not an issue because those paths
will be preferred for the resolution.

Libaec 1.1.4 fixes this issue.

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

@Ozaq Ozaq requested a review from ChrisspyB September 29, 2025 17:11
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.74%. Comparing base (448bd32) to head (b899032).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop      #73   +/-   ##
========================================
  Coverage    77.74%   77.74%           
========================================
  Files           97       97           
  Lines         4933     4933           
  Branches       444      444           
========================================
  Hits          3835     3835           
  Misses        1098     1098           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Ozaq Ozaq force-pushed the feature/remove-find-aec branch 3 times, most recently from 1a605c1 to dea0db6 Compare October 1, 2025 09:23
Comment on lines -34 to -42
ecbuild_find_package( NAME AEC VERSION 1.1.1 REQUIRED )

# ecbuild_find_package's version checking does not work if the version is not specified in the package.
if (NOT AEC_VERSION)
message(FATAL_ERROR "AEC version is too old (version unspecified). Minimum supported version is 1.1.1.")
endif()
if (AEC_VERSION VERSION_LESS 1.1.1)
message(FATAL_ERROR "AEC version is too old. Minimum supported version is 1.1.1. Found version: ${AEC_VERSION}")
endif()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would quite like to continue failing at config time if the aec version is too old. Unless there has been an update to ecbuild, "VERSION 1.1.1 REQUIRED" has proven insufficient to ensure this.

Copy link
Copy Markdown
Member

@ChrisspyB ChrisspyB Oct 6, 2025

Choose a reason for hiding this comment

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

The correct solution would be for ecbuild to properly fail if it picks it only picks up an aec older than 1.1.1. I believe the issue may be due to older aec's not having a discernable version number, so ecbuild fails to validate the version and keeps marching on...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please notice that we are not using the old FindAEC.cmake here any longer, but we are searching for a dependency 'libaec'. Libaec starting with 1.0.6 supplies version information in its libaec-config.cmake (I checked all distros we use). Further this is now the minimum accepted version across the stack. So this should error out reliably if any other ecbuild_find_package has picked up an earlier libaec. Internally this falls back to a find_package in config mode with modern targets and version numbers.


### AEC
# Override eccodes' aec with our own: we need a newer version.
# Can be removed once we rely on next eckit version
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it going to be compulsory to build eckit/eccodes with aec 1.1.1+?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

afaik, it is currently not compulsory to build those libraries with aec at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No this comment is about

unset(AEC_INCLUDE_DIRS CACHE)
unset(AEC_LIBRARIES CACHE)

still being in the code, this was left in for the transitionary period when eckit did not yet remove the use of FindAEC.cmake

The minimum eckit version should be hiked to the next version (current develop) to ensure eckit/eccodes/gribjump use all ecbuild_find_pacakge(libaec) (aka use libaec-config.cmake instead of FindAEC.cmake)

This is in preparation to move libaec to be a source dependency in
cxx-dependencies where we will be providing libaec 1.1.4.

This is also part of bump of libaec to 1.0.6 across the whole C++ stack.
With 1.0.6 libaec introduced cmae config files that replace FindModules.
Unfortunately the cmake config file for libaec 1.0.6 to 1.1.3 do not
work correctly if libaec is not installed system wide in combination
with a system wide installed version of libaec. The bug is that the
configuration file just picks up the first libaec.h file and the first
libaec.so. Which in this combination will point to the system wide
installed version.

This can (and is addressed in this PR) by hinting cmake to the correct
location of the loaded aec module on ATOS.

When using CMAKE_PREFIX_PATH this is not an issue because those paths
will be preferred for the resolution.

Libaec 1.1.4 fixes this issue.
@Ozaq Ozaq force-pushed the feature/remove-find-aec branch from 9250af0 to b899032 Compare October 8, 2025 08:11
@Ozaq
Copy link
Copy Markdown
Member Author

Ozaq commented Oct 8, 2025

Dropping this PR, this needs to go into the full on cxx-migration

@Ozaq Ozaq closed this Oct 8, 2025
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