Skip to content

Find definitions and samples relative to the library - #581

Draft
mcocdawc wants to merge 1 commit into
developfrom
feature/relocatable-data-paths
Draft

mcocdawc wants to merge 1 commit into
developfrom
feature/relocatable-data-paths

Conversation

@mcocdawc

@mcocdawc mcocdawc commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Problem

The default definitions and samples paths are absolute and fixed at build time (ECCODES_DEFINITION_PATH / ECCODES_SAMPLES_PATH in eccodes_config.h). If an installation is moved after make install, ecCodes can no longer find its own data unless the environment variables are set by hand.

This hit the new CI: ecCodes is installed to github-ci/install/eccodes-<sha>…, then staged for downstream builds into github-ci/staging/<pkg>-…/deps/N. In metkit (ecmwf/metkit#293) metkit_test_codes_api failed with:

ECCODES ERROR : Unable to load sample file 'GRIB2.tmpl'
  samples path='…/build/share/eccodes/samples:…/github-ci/install/eccodes-d2f8adae…/share/eccodes/samples'

The same applies to tarballs, containers and any other relocated install. The CMake package is already relocatable (eccodes-import.cmake derives the paths from eccodes_BASE_DIR); only the library isn't.

Change

  • CMake computes the data directories relative to the library directory (ECCODES_{DEFINITION,SAMPLES}_RELPATH, e.g. ../share/eccodes/definitions) and puts them in eccodes_config.h.
  • At context initialisation, grib_context.cc locates the loaded ecCodes library with dladdr and uses <libdir>/<relpath> if that directory exists. Otherwise it falls back to the compiled-in path.
  • Precedence: environment variable > library-relative path > compiled-in path. The ECC-1088 logic, which appends the default path, now appends the resolved default.
  • Links ${CMAKE_DL_LIBS} for dladdr (needed on glibc < 2.34).
  • Skipped on Windows and with MEMFS, where behaviour is unchanged.
  • The build tree has the same layout (build/lib, build/share/eccodes/…), so build-tree tests find their data without environment variables. This PR removes the exports from .ci/hpc/build.sh.j2, so CI exercises the new lookup.

Testing

Tested locally on macOS (shared build):

  • Installed, then moved the install prefix: codes_info -d/-s report <moved>/lib/../share/eccodes/…, and grib_ls decodes GRIB2.tmpl with no ECCODES_* variables set.
  • Build tree with the install prefix absent: data found under build/lib/../share/eccodes/….
  • ECCODES_SAMPLES_PATH set explicitly still wins.

The full test suite was not run locally; this relies on CI.

Open points

  • Static builds: dladdr resolves to the executable, so the lookup becomes <bindir>/../share/eccodes/…. That works for the installed tools and otherwise falls back to the compiled-in path because of the existence check.
  • Multiarch libdirs (lib/x86_64-linux-gnu): the install relpath is ../../share/…, which works for the install but not the build tree (always build/lib). The build tree then falls back to the current behaviour.
  • Windows support (GetModuleHandleEx + GetModuleFileName) could follow if wanted.

🤖 Generated with Claude Code

The default definitions and samples paths are absolute and baked in at
build time, so a relocated installation (CI staging, tarballs, containers)
cannot find its own data unless ECCODES_DEFINITION_PATH and
ECCODES_SAMPLES_PATH are set by hand.

Look for the data relative to the directory of the loaded ecCodes library
first (<libdir>/../share/eccodes/...), falling back to the compiled-in
path. The environment variables keep precedence. The build tree has the
same layout, so tests no longer need the paths exported either; drop
that from the HPC CI script.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.67%. Comparing base (d2f8ada) to head (0819cd9).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/eccodes/grib_context.cc 93.75% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #581   +/-   ##
========================================
  Coverage    87.66%   87.67%           
========================================
  Files          857      857           
  Lines        64110    64122   +12     
  Branches     11409    11412    +3     
========================================
+ Hits         56205    56216   +11     
- Misses        7905     7906    +1     

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcocdawc
mcocdawc marked this pull request as draft October 2, 2026 14:59
@sawom666

sawom666 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Why not stick with MEMFS in these situations? rather than adding extra options and hence complexity

@joobog

joobog commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Is it really necessary to introduce relative paths for the definition and sample files? I'm not sure if the added complexity is justified. Why not just set ECCODES_DEFINITION_PATH and ECCODES_SAMPLES_PATH to the new locations?

@joobog
joobog self-requested a review October 8, 2026 08:44
@mcocdawc

mcocdawc commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Yeah, It's a draft PR and I wanted to play around. Probably I will close it again.

@joobog

joobog commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Yeah, It's a draft PR and I wanted to play around. Probably I will close it again.

No problem. I just wanted to share my thoughts.

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