WIP: small but crucial fixes to ecbuild#139
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts ecbuild’s generated CMake package config behavior to avoid incorrectly skipping target imports in some find_package() scenarios, and reduces CMake variable-scope pollution by converting a helper macro into a function.
Changes:
- Refine the guard in
project-config.cmake.inso install-tree exports always load the*-targets.cmakefile, while build-tree exports can skip it when appropriate. - Convert
ecbuild_target_fortran_module_directoryfrom amacro()to afunction()to avoid leaking internal variables into the caller’s scope.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cmake/project-config.cmake.in |
Updates the logic that decides whether to include the exported targets file when a package config is loaded. |
cmake/ecbuild_target_fortran_module_directory.cmake |
Converts a macro to a function to limit variable pollution while keeping behavior equivalent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # always load the targets file. For build-tree exports the targets are already | ||
| # defined by the build system, so we only skip when we are genuinely inside that | ||
| # build (i.e. the variable was set by ecbuild's own project() hook, not by an | ||
| # unrelated project that happens to share the same name). | ||
| if(NOT (@PROJECT_NAME@_IS_BUILD_DIR_EXPORT AND @PROJECT_NAME@_BINARY_DIR)) |
There was a problem hiding this comment.
The new guard/comment says we only skip loading targets when we are “genuinely inside” the exporting build (i.e. not an unrelated project with the same name). However the condition only checks @PROJECT_NAME@_IS_BUILD_DIR_EXPORT and that @PROJECT_NAME@_BINARY_DIR is non-empty; any unrelated project named @PROJECT_NAME@ will also define @PROJECT_NAME@_BINARY_DIR, so for build-tree exports this would still skip loading the targets file. Consider either (a) tightening the condition to verify @PROJECT_NAME@_BINARY_DIR actually matches this export’s build tree (e.g., compare against @PROJECT_NAME@_BASE_DIR / PACKAGE_PREFIX_DIR), or (b) adjusting the comment to match the actual behavior.
| # always load the targets file. For build-tree exports the targets are already | |
| # defined by the build system, so we only skip when we are genuinely inside that | |
| # build (i.e. the variable was set by ecbuild's own project() hook, not by an | |
| # unrelated project that happens to share the same name). | |
| if(NOT (@PROJECT_NAME@_IS_BUILD_DIR_EXPORT AND @PROJECT_NAME@_BINARY_DIR)) | |
| # always load the targets file. For build-tree exports the targets are already | |
| # defined by the build system, so we only skip when we are genuinely inside that | |
| # build tree, i.e. when ecbuild's own project() hook has set | |
| # @PROJECT_NAME@_BINARY_DIR to the same location as this export's base directory. | |
| set(_@PROJECT_NAME@_INSIDE_EXPORTING_BUILD FALSE) | |
| if(@PROJECT_NAME@_IS_BUILD_DIR_EXPORT AND DEFINED @PROJECT_NAME@_BINARY_DIR AND NOT "@PROJECT_NAME@_BINARY_DIR" STREQUAL "") | |
| get_filename_component(_@PROJECT_NAME@_EXPORT_BASE_DIR "${@PROJECT_NAME@_BASE_DIR}" REALPATH) | |
| get_filename_component(_@PROJECT_NAME@_CURRENT_BINARY_DIR "${@PROJECT_NAME@_BINARY_DIR}" REALPATH) | |
| if(_@PROJECT_NAME@_EXPORT_BASE_DIR STREQUAL _@PROJECT_NAME@_CURRENT_BINARY_DIR) | |
| set(_@PROJECT_NAME@_INSIDE_EXPORTING_BUILD TRUE) | |
| endif() | |
| endif() | |
| if(NOT _@PROJECT_NAME@_INSIDE_EXPORTING_BUILD) |
Each dependency now exposes a use-<pkg> composite action in its own repo. Consumers call that action — they do not see or repeat caching logic. New composite actions: - ecbuild/.github/actions/use-ecbuild: installs ecbuild from its own repo checkout, cache-keyed on action SHA. Replaces all inline ecbuild blocks (4+ occurrences). - downstream-ci/actions/check-artifact: wraps check_artifact.py via github.action_path (no extra checkout needed). - downstream-ci/actions/download-artifact-from-store: gh api download + unzip + extract. - fortran/.github/actions/use-fortmath: 3-strategy acquire (artifact store -> local cache -> build from source); uploads artifact to the calling run. - cxx/.github/actions/use-cxxmath: same pattern; calls use-fortmath internally for the build-from-source path. Updated workflows: - fortran/build.yml: inline ecbuild block replaced by use-ecbuild. - fortran/ci.yml: ECBUILD_REF env removed; test job uses use-ecbuild. - cxx/ci.yml: build-fortmath workflow_call job replaced by thin get-fortmath job calling use-fortmath; all inline ecbuild blocks replaced by use-ecbuild. Deleted: - fortran/.github/workflows/use-cache-or-build.yml - cxx/.github/workflows/use-cache-or-build.yml Composite actions have no workflow-nesting limit, so this scales to arbitrary dependency trees without hitting GitHub Actions' 4-level cap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ecbuild repo has a symlink loop in tests/bundle_subproj/mybundle/ecbuild that creates an infinitely long path when GitHub Actions extracts the repo tarball. Building from github.action_path triggered this extraction. Fix: clone a fresh shallow copy at github.action_ref and build from there. Also add -DENABLE_TESTS=OFF to cmake to skip the bundle test setup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ecbuild repo has a symlink loop in tests/bundle_subproj/mybundle/ecbuild that causes GitHub Actions to fail during tarball extraction — this happens before any action code runs, so it cannot be worked around inside the action. Fix: move use-ecbuild to downstream-ci/actions/use-ecbuild (no symlinks). The action clones ecbuild fresh at the given ecbuild-ref input. Update all callers from: ecmwf/ecbuild/.github/actions/use-ecbuild@fix_interface_exports to: ecmwf-enterprise-sandbox/downstream-ci/actions/use-ecbuild@main Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Change the detection that avoids redundant project creation.
The current setup prevented a test-only build.
Change macro to function, that has equivalent behaviour, but less pollution.
Contributor Declaration
By opening this pull request, I affirm the following: