From 039377b6265f594c45a852ac079922e74be8d6f6 Mon Sep 17 00:00:00 2001 From: Simon Smart Date: Tue, 5 May 2026 19:46:52 +0200 Subject: [PATCH 1/4] ECKIT-680: Add target library, tags and lazy loading of plugins --- .gitignore | 1 + AGENTS.md | 103 ++++ CONTRIBUTING.md | 444 ++++++++++++++++++ docs/content/plugins.rst | 184 ++++++++ docs/content/string_tools.rst | 201 ++++++++ docs/index.rst | 2 + src/eckit/runtime/Main.cc | 17 +- src/eckit/runtime/Main.h | 8 +- src/eckit/system/Library.cc | 5 +- src/eckit/system/Library.h | 5 + src/eckit/system/LibraryManager.cc | 294 ++++++++++-- src/eckit/system/LibraryManager.h | 84 +++- src/eckit/system/Plugin.cc | 7 +- src/eckit/system/Plugin.h | 32 +- src/eckit/utils/StringTools.h | 66 ++- tests/system/CMakeLists.txt | 110 ++++- tests/system/plugin_template.cc | 46 ++ tests/system/plugin_test_helpers.h | 54 +++ tests/system/test_system_library.cc | 35 +- tests/system/test_system_plugin_disable.cc | 120 +++++ ...est_system_plugin_main_autoload_default.cc | 89 ++++ ...st_system_plugin_main_autoload_disabled.cc | 80 ++++ ...ystem_plugin_main_autoload_env_disabled.cc | 73 +++ ...st_system_plugin_main_autoload_explicit.cc | 87 ++++ .../test_system_plugin_manifest_loading.cc | 227 +++++++++ tests/system/test_system_plugin_version.cc | 168 +++++++ tests/utils/test_string_tools.cc | 57 +++ 27 files changed, 2536 insertions(+), 63 deletions(-) create mode 100644 AGENTS.md create mode 100644 CONTRIBUTING.md create mode 100644 docs/content/plugins.rst create mode 100644 docs/content/string_tools.rst create mode 100644 tests/system/plugin_template.cc create mode 100644 tests/system/plugin_test_helpers.h create mode 100644 tests/system/test_system_plugin_disable.cc create mode 100644 tests/system/test_system_plugin_main_autoload_default.cc create mode 100644 tests/system/test_system_plugin_main_autoload_disabled.cc create mode 100644 tests/system/test_system_plugin_main_autoload_env_disabled.cc create mode 100644 tests/system/test_system_plugin_main_autoload_explicit.cc create mode 100644 tests/system/test_system_plugin_manifest_loading.cc create mode 100644 tests/system/test_system_plugin_version.cc diff --git a/.gitignore b/.gitignore index fec77aad6..3b5c05277 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,4 @@ build/ tests/geo/eckit_geo_cache _build *.ccls-cache +Testing/ diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..02fae41c4 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,103 @@ +# AGENTS.md + +Guidance for AI coding agents working in this repository. **Human contributors +should read [`CONTRIBUTING.md`](CONTRIBUTING.md) first** — that document is +the canonical reference for project structure, style, build/test workflow, +and the C++ source-file template. This file only adds the agent-specific +notes that sit on top of it. + +--- + +## Source of truth + +When the prose in any document disagrees with the build files, **the +executable CMake/workflow files win**. In order of authority: + +1. Root `CMakeLists.txt` and per-module `CMakeLists.txt` files — they define + what is built, gated, installed, and tested. +2. `tests//CMakeLists.txt` — authoritative list of tests and their + environment. +3. `.pre-commit-config.yaml` — authoritative list of lint/format checks. +4. `docs/Makefile`, `.github/workflows/*` — authoritative CI behaviour. +5. `CONTRIBUTING.md` — the human-readable summary; aim to keep it + consistent with the above when you touch related areas. +6. `AGENTS.md` (this file) and `.github/copilot-instructions.md` — agent + guidance; subordinate to everything above. + +If you spot a real divergence, fix the prose, not the build files (unless +the user has asked for a behaviour change). + +--- + +## Required reading before non-trivial changes + +* [`CONTRIBUTING.md`](CONTRIBUTING.md) — full project overview, file + templates, naming, formatting, testing harness, pre-commit checklist. +* The `CMakeLists.txt` of any module you are about to modify, plus its + test directory. + +--- + +## House-keeping invariants agents tend to miss + +These are the recurring traps. Re-check them before declaring a task done. + +* **Tool target ↔ installed binary.** `src/tools/CMakeLists.txt` maps + underscored CMake targets to hyphenated installed names: target + `eckit_version` → installed binary `eckit-version`. Search for both + spellings when looking up usage. +* **Feature gates have two halves.** A new optional feature needs *both* + an `ecbuild_add_option(FEATURE ...)` block in the root `CMakeLists.txt` + *and* an `if(eckit_HAVE_)` guard in each consuming + `CMakeLists.txt` *and* an `eckit_HAVE_` C++ macro in the + generated `eckit_config.h`. Touching one without the others produces + silently broken builds. +* **Generated headers live in the build tree.** `eckit_config.h`, + `eckit_version.h`, and similar are written under `build/src/...` not the + source tree. Compile commands and any tooling you spawn must include + both `src/` and `build/src/` on the include path. +* Avoid unnecessary `eckit::` qualifiers in the code. When inside a + nested namespace (such as `namespace eckit::test`) redundant + qualifiers are just noise. +* **`eckit::Mutex` is recursive.** Nested locking is safe, but do not + assume that of any other mutex type. Recursive calls should be avoided + wherever possible in any case. + +--- + +## Workflow rules for agents + +* **Add tests via `ecbuild_add_test`, never via ad-hoc scripts.** The + CTest integration is the only contract CI honours. +* **Never edit pre-existing style on lines you are not otherwise + modifying.** Cleanups creep, reviews balloon. Limit refactors to the + scope explicitly requested by the user, or open a separate task for + them. +* **Format with `clang-format` directly, not by hand.** Run + `./format-sources.sh` (which enforces version 19) on every C/C++ file + you touched. Do not run it on `CMakeLists.txt`. +* **Verify with the real build, not with reasoning.** After non-trivial + edits, run `cmake --build build -j` and at minimum the tests in the + modules you touched. The user's machine has limited cores; if they + have asked you to use a specific `-j`, respect it. If not specified + do not exceed -j6. +* **Do not commit unless the user explicitly asks.** The default + expectation is "show me the diff, I will commit". +* **Do not push to remote unless the user explicitly asks.** Same as + above. +* **Untracked artefacts** (e.g. `Testing/`, ad-hoc local scratch + directories) should be left alone unless the user asks to clean them + up. + +--- + +## Hand-off + +When pausing or finishing a piece of work, leave the workspace ready for +either the user or another agent to resume: + +* Build is clean (no half-applied edits that fail to compile). +* Tests in the touched module pass; mention any pre-existing failures you + did not address. +* Summarise what changed, why, and what is left, in a few bullets — not + a wall of text. The user will read it; brevity matters. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..876b648a6 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,444 @@ +# Contributing to eckit + +This document is the practical reference for how to add or modify code in this +repository, written so that both new contributors and AI coding agents can +produce changes that match the existing house style on the first try. + +For deeper architectural context, see [`AGENTS.md`](AGENTS.md) and +[`docs/`](docs/). + +--- + +## Project structure + +`eckit` is a C++17 toolkit built with [`ecbuild`](https://github.com/ecmwf/ecbuild) +on top of CMake. The repository is organised as follows: + +| Path | Purpose | +| --- | --- | +| `src/eckit/` | Public sources of the main `eckit` shared library. Each subdirectory is a logical module (`config`, `container`, `filesystem`, `io`, `log`, `runtime`, `system`, `thread`, `utils`, ...). | +| `src/eckit//` | Optional libraries gated by `ecbuild_add_option(FEATURE ...)` in the root `CMakeLists.txt`: `eckit_mpi`, `eckit_linalg`, `eckit_geometry`, `eckit_maths`, `eckit_web`, `eckit_cmd`, `eckit_codec`, `eckit_spec`, `eckit_geo`, `eckit_sql`. Each has its own `CMakeLists.txt`. | +| `src/eckit/testing/` | Header-only testing harness (`Test.h` — `CASE`, `EXPECT`, `EXPECT_EQUAL`, `EXPECT_THROWS_AS`, `SetEnv`, `run_tests`). | +| `src/tools/` | Source of installed CLI tools. CMake target `eckit_foo` produces installed binary `eckit-foo`. | +| `tests//` | One CMakeLists per module. New tests are declared there with `ecbuild_add_test(TARGET eckit_test__ ...)`. | +| `docs/` | Sphinx documentation. `make -j html` from `docs/`; warnings are errors. | +| `python/eckit/` | Cython wrapper. Builds against an already-built C++ eckit. | +| `bamboo/`, `tools/` | CI helpers. | +| `cmake/` | Project-specific CMake modules. | + +Generated headers like `eckit_config.h` and `eckit_version.h` are written into +the build tree. Compilation needs both `src/` and `build/src/` on the include +path; `ecbuild` handles this for in-tree targets. + +### Where to put new code + +* **New class in an existing module** — add `Foo.h` and `Foo.cc` next to its + siblings under `src/eckit//` and append to that module's + `CMakeLists.txt`. +* **New module** — create `src/eckit//` with its own `CMakeLists.txt`, + add an `ecbuild_add_option` entry in the root `CMakeLists.txt`, and + define an `eckit_HAVE_` config macro. +* **New test** — add the source under `tests//` and register it in + that directory's `CMakeLists.txt`. + +--- + +## Build, test, format + +`ecbuild` must be on `PATH` (or discoverable through `ecbuild_ROOT` / +`CMAKE_PREFIX_PATH` if you call CMake directly). + +```sh +mkdir -p build +(cd build && ecbuild --prefix= -- ..) +cmake --build build -j +(cd build && ctest -j --output-on-failure) +``` + +Run a single test by target name: + +```sh +(cd build && ctest -R '^eckit_test_types_uuid$' --output-on-failure) +``` + +For debugging, run the test binary directly from the build dir, e.g. +`./bin/eckit_test_types_uuid`. + +Test targets are declared in `tests//CMakeLists.txt` with +`ecbuild_add_test(TARGET eckit_test__ ...)`. Add new tests +there rather than via ad-hoc scripts. + +Format C/C++ sources with the project script (clang-format major version 19): + +```sh +./format-sources.sh path/to/file.cc path/to/file.h +``` + +`pre-commit` enforces clang-format 19 on C/C++, plus whitespace/LF, isort, +ruff for Python, yamllint, JSON formatting, and cython-lint. + +### Tool target naming + +`src/tools/CMakeLists.txt` maps internal CMake targets that use underscores +to installed binaries that use hyphens. For instance, target `eckit_version` +produces the installed tool `eckit-version`. Keep this in mind when looking +up which CMake target builds a particular CLI tool. + +### Generated headers + +Configured headers like `eckit_config.h` and `eckit_version.h` are written +into the build tree, so include paths must contain both `src/` and +`build/src/`. `ecbuild` wires this up for in-tree targets; external consumers +get it via the installed CMake package config. + +--- + +## Python wrapper + +The `python/eckit` wrapper is Cython-based and expects an already-built +eckit: + +```sh +ECKIT_SOURCE_DIR= ECKIT_BUILD_DIR= python3 setup.py build_ext -i +``` + +`python/eckit/setup.py` links against `eckit` and `eckit_geo`. Override +paths with `ECKIT_LIB_DIR` and `ECKIT_INCLUDE_DIRS` when the default build +layout does not match your setup. + +`python/eckit/build_chain.sh` assumes `uv`, `PYVERSION`, and the +wheelmaker / setup_utils environment; its pytest step is currently commented +out and only `twine check` runs. + +--- + +## File naming and ownership + +* **One class per file**, named after the class. `class FooBar` lives in + `FooBar.h` and `FooBar.cc`. Keep small helper structs and free functions + beside the class they support if they are an implementation detail. +* **Header extension is `.h`**, source extension is `.cc`. Do not use `.hpp`, + `.cpp`, `.cxx`. +* **Test files** are named `test__.cc` and produce a CTest + target `eckit_test__`. +* **Header-only test helpers** live next to the tests that use them, e.g. + `tests/system/plugin_test_helpers.h`. + +--- + +## C++ source file template + +### Common header (`.h` and `.cc`) + +Every source file starts with the ECMWF Apache 2.0 boilerplate, exactly: + +```cpp +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ +``` + +A blank line follows, then for **header files** an author/date block: + +```cpp +/// @author Simon Smart +/// @date May 2026 +``` + +The author/date block is conventional on headers and omitted from `.cc` files. +Authorship identifies individuals who are conceptually responsible for and +aware of the relevant code. +Use the month and year you create the file. Multiple `@author` lines are +allowed when several people are responsible. + +### Header file (`.h`) + +```cpp +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +/// @author Simon Smart +/// @date May 2026 + +#pragma once + +#include +#include + +#include "eckit/system/Library.h" + +namespace eckit::system { + +class LibraryManager; // forward declarations as needed + +//---------------------------------------------------------------------------------------------------------------------- + +/// @brief One-line summary of what this class is. +/// +/// Multi-line description if the contract is non-trivial: lifecycle, threading, +/// preconditions, ownership. Useful to spell out invariants that are not obvious +/// from the signatures. +class Foo { +public: + + /// @param [in] name what the parameter means + explicit Foo(const std::string& name); + + ~Foo(); + + /// @brief Short doxygen on each public method that is not self-evident. + const std::string& name() const { return name_; } + +private: + + friend class LibraryManager; // friend declarations at the top of private: + + std::string name_; +}; + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::system +``` + +Key points: + +* **Include guard**: prefer `#pragma once` for new files. Older files + use Older files use old-style include guards - when significant modifications + are made to those files switch them to use `#pragma once`, but otherwise + leave them alone. +* **Includes** are grouped: standard library first (`<...>`), then + third-party, then project (`"eckit/..."`). A blank line separates groups. + The corresponding `.h` of a `.cc` file comes first in that `.cc`. +* **Namespaces** start at column 0 with no indentation of their contents. + Use the C++17 nested form `namespace eckit::system {` rather than nested + blocks. Close with `} // namespace eckit::system` (two spaces before the + comment). +* **Separator lines** of exactly 116 dashes (`//----...`) bracket each major + section: one after the opening `namespace` declaration and one before the + closing brace, plus one between top-level declarations when grouping helps + readability. Match the existing files; clang-format does not generate them. +* **`using namespace` is forbidden in headers.** Use the fully qualified name + or a narrow `using eckit::Foo;` after the namespace open. +* **Doxygen** uses `///` (triple slash) and the `@brief`/`@param`/`@return`/ + `@throws` tags. Document non-obvious behaviour, especially threading, + ownership, and exception guarantees. +* **`friend` declarations** go at the top of the `private:` section. +* **Class layout order** is `public:` then `protected:` then `private:`, with + methods before data members in each. + +### Source file (`.cc`) + +```cpp +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include "eckit/system/Foo.h" + +#include +#include + +#include "eckit/exception/Exceptions.h" +#include "eckit/log/Log.h" + +namespace eckit::system { + +//---------------------------------------------------------------------------------------------------------------------- + +Foo::Foo(const std::string& name) : name_(name) {} + +Foo::~Foo() = default; + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::system +``` + +Key differences from the header: + +* **No author/date block** — the convention is to keep that on the header + only. +* **First include is the matching header**, on its own line, separated by a + blank line from the rest of the includes. This forces the header to be + self-contained. +* **No declaration body in the header is duplicated here.** Default and + trivial inline definitions stay in the header. +* **File-local helpers** (free functions, `static` variables, internal + classes) go inside an anonymous namespace at the top of the `.cc`: + + ```cpp + namespace { + + int helper() { return 42; } + + } // namespace + ``` + +### Header-vs-source rule of thumb + +| Put in `.h` | Put in `.cc` | +| --- | --- | +| Class declaration, public/protected API, friend declarations | Out-of-line method definitions | +| Inline trivial accessors (one-liners returning a member) | Anything that would otherwise pull a heavy include into the `.h` | +| Templates and constexpr definitions | Free functions and statics that are implementation-only | +| Doxygen describing the contract | Comments explaining *why* a non-obvious implementation choice was made | +| Forward declarations of types only used by reference / pointer | `#include` of those types' headers | + +If you find yourself adding a heavyweight `#include` in a header just to make +something compile, consider whether a forward declaration in the header plus +a full include in the `.cc` would do. + +--- + +## Coding conventions + +### Language level + +C++17. Prefer modern idioms (`auto`, range-for, structured bindings, +`std::optional`, `if constexpr`) where they aid clarity. Do not introduce +C++20 features. + +### Formatting + +clang-format 19 with the project `.clang-format`. Do **not** hand-format; +run `./format-sources.sh` after editing. Notable settings the formatter +enforces: + +* 4-space indent, no tabs. +* Pointer/reference attaches to type: `int* p`, not `int *p`. +* Braces on the same line for functions, classes, and control flow + (`if (x) {`). +* `class Foo : public Bar {` on one line. +* Member initialiser lists wrap with the colon on a new line and one + initialiser per line when long. + +### Naming + +* Types: `UpperCamelCase` (`PluginManifest`, `LibraryManager`). +* Functions and variables: `lowerCamelCase` (`loadedPlugins`, `forLibrary`). +* Private and protected data members: trailing underscore (`name_`, + `handle_`). +* Macros and preprocessor constants: `SCREAMING_SNAKE_CASE` + (`REGISTER_LIBRARY`, `eckit_HAVE_ECKIT_MPI`). +* File names match the primary type they declare. + +### Errors + +Throw `eckit` exceptions from `eckit/exception/Exceptions.h` +(`BadValue`, `SeriousBug`, `AssertionFailed`, ...) rather than standard +ones. Use the `Here()` helper to attach source location where to helps +diagnostics and wherever reasonable. `ASSERT(...)` for invariants, +`EXPECT(...)` only inside test `CASE` bodies. + +### Threading + +Use `std::mutex` and related `std` types where appropriate. There is +a plethora of existing use of `eckit::Mutex` / `eckit::AutoLock<...>` / +`eckit::StaticMutex` in the code base - it is not necessary to use those +for new code and old code is being gradually migrated. +Document threading expectations in doxygen. + +### Includes + +* Use `"eckit//.h"` form (with the `eckit/` prefix) for any + include from this project, even within the same module. +* Sort within each group; clang-format will do this for you. +* Never include `` or other libstdc++ internals. + +### Feature gates + +Feature-gated code uses both a CMake `if(eckit_HAVE_)` to compile +in/out the relevant translation units, and a generated `eckit_HAVE_` +macro from `eckit_config.h` for runtime conditional logic. New features get +both halves wired up. + +--- + +## Tests + +* New tests live in `tests//test__.cc` and are + registered with `ecbuild_add_test(TARGET eckit_test__ ...)`. +* Tests use the harness in `eckit/testing/Test.h`. A typical test file: + + ```cpp + #include "eckit/testing/Test.h" + + using namespace eckit::testing; + + namespace eckit::test { + + //---------------------------------------------------------------------------------------------------------------------- + + CASE("descriptive sentence") { + EXPECT(condition); + EXPECT_EQUAL(actual, expected); + EXPECT_THROWS_AS(badCall(), BadValue); + } + + //---------------------------------------------------------------------------------------------------------------------- + + } // namespace eckit::test + + int main(int argc, char** argv) { + return run_tests(argc, argv); + } + ``` + +* When a test must observe state both before and after a global + initialisation (e.g. before/after `Main` construction), put plain + conditional aborts in `main()` around the construction — `EXPECT` only + works reliably inside `CASE`. +* Do not silently rely on environment defaults. Make every relevant env var + explicit with `SetEnv` so the test is hermetic. + +--- + +## Documentation + +* User-facing docs live under `docs/content/` as reStructuredText files + consumed by Sphinx with the Doxygen-generated API reference. Add new + pages to the appropriate `toctree` in `docs/index.rst` or a + parent page. +* Doxygen comments in headers are extracted into the API docs; keep them + current when changing signatures or contracts. +* The Sphinx build runs with `-W` (warnings as errors). A broken cross- + reference will fail CI. +* Docs CI uses Python 3.13, installs `docs/requirements.txt`, prepares + Doxygen 1.14.0, and then runs `cd docs && make -j html`. Reproduce + locally with the same Python version when chasing CI-only failures. + +--- + +## Pre-commit checklist + +Before opening a PR: + +1. `./format-sources.sh` on every changed C/C++ file. +2. `cmake --build build -j` clean. +3. `ctest -j --output-on-failure` from the build dir, or at minimum the + tests in the modules you touched. +4. If you added Sphinx content, `cd docs && make -j html` clean. +5. Headers you created carry the licence header *and* `/// @author` / + `/// @date` lines. +6. No `using namespace` in headers; no stray `#include`s; no commented-out + code. diff --git a/docs/content/plugins.rst b/docs/content/plugins.rst new file mode 100644 index 000000000..c43e3a532 --- /dev/null +++ b/docs/content/plugins.rst @@ -0,0 +1,184 @@ +Plugins +======= + +eckit supports loading additional functionality at runtime via plugins: +shared libraries that derive from :cpp:class:`eckit::system::Plugin` and +self-register with :cpp:class:`eckit::system::LibraryManager` as their +static instance is constructed. + +Plugins are described by YAML manifest files. The loader discovers +manifests, validates them against the running process and ``dlopen``\ s +the corresponding shared object on demand. + +Once loaded, the `init()` method of the plugin is called. Prior to unloading +the plugin, during rollback of a failed load, or during clean shutdown of +the process, the plugin's `finalise()` method is called. + +Many plugins create global resources, which self-register with appropriate +global factories (in eckit or elsewhere). The self-registering resources +should be instantiated in the plugin's `init()` method, and cleaned up in +`finalise()`, rather than relying on static (global) initialisation and +destruction, if this is at all possible. + +Manifest discovery +------------------ + +When ``LibraryManager`` scans for manifests it visits the following +directories, in this order, stopping at the first match per fully +qualified plugin name: + +#. Each directory listed in ``$PLUGINS_MANIFEST_PATH`` + (or the ``pluginManifestPath`` resource), ``:``-separated. +#. ``/share/plugins`` for every directory registered with + :cpp:func:`eckit::system::LibraryManager::addPluginSearchPath`. +#. The directories returned by + :cpp:func:`eckit::system::Library::pluginManifestPaths` for each + currently registered library (default: ``~/share/plugins``). +#. ``~eckit/share/plugins`` +#. ``~/share/plugins`` + +Symlinks resolve to canonical paths and the same manifest directory is +never scanned twice. The first manifest seen for a fully qualified name +``namespace.name`` wins; subsequent duplicates are logged and ignored. + +Manifest schema +--------------- + +A manifest is a YAML file with a single top-level ``plugin:`` mapping. + +.. list-table:: + :header-rows: 1 + :widths: 20 15 65 + + * - Key + - Required + - Meaning + * - ``name`` + - yes + - Plugin name (matches the argument passed to the + :cpp:class:`Plugin ` constructor in C++). + * - ``namespace`` + - yes + - Namespace used to build the fully qualified name (example int.ecmwf) + ``namespace.name``. + * - ``library`` + - yes + - Shared library file stem, without the ``lib`` prefix or + platform-specific extension. + * - ``for-library`` + - no + - Owning library name. When set, the plugin is *scoped* and is + only loaded by an explicit request from that library or by + fully-qualified name (see below). When absent the plugin is + *global*. + * - ``min-version`` + - no + - Minimum version of ``for-library`` required for the plugin to + load. Compared as semver-ish numeric ``major.minor[.patch]``; + if the registered library's version is lower throw ``BadValue``. + * - ``version`` + - no + - Pinned exact value of the plugin's self-reported + :cpp:func:`Plugin::version`. Compared verbatim (string equality, + not semver) against the version reported by the Plugin object + at runtime. On mismatch the loader unloads the offending shared + object again and throws ``BadValue`` without calling the Plugin's + ``init()`` function. + * - ``tags`` + - no + - Free-form list of labels used by ``loadPluginsFor`` for filtering. + +Example: + +.. code-block:: yaml + + plugin: + name: my-plugin + namespace: int.ecmwf + library: my-plugin + for-library: eckit + min-version: 1.20.0 + version: 0.4.2 + tags: [grids, io] + +Loading model +------------- + +There are three orthogonal ways a plugin can be loaded. + +**Global auto-load (Main startup)** + Plugins without ``for-library`` are loaded by :cpp:class:`eckit::Main` + on construction when both: + + * the ``Main`` constructor's ``autoLoadPlugins`` parameter is + ``true`` (the default), and + * the resource ``$AUTO_LOAD_PLUGINS`` (default ``true``) is true. + + Scoped plugins are *not* loaded by this path. + + ***Note: This mechanism is deprecated and will be removed in the future.*** + Plugins should be explictily loaded by their owning library via the scoped + mechanism described below. + +**Scoped load (library-driven)** + A library opts in to loading its scoped plugins by calling + :cpp:func:`eckit::system::LibraryManager::loadPluginsFor` from its + initialisation routine, or lazily during execution when required, e.g.:: + + LibraryManager::loadPluginsFor("my-lib", {"grids"}); + + Only manifests whose ``for-library`` matches and whose ``tags`` + include *all* of the requested tags are considered. An empty tag + list matches every scoped manifest for the library. ``min-version`` + and ``version`` checks apply. + +**Explicit list** + Setting ``$LOAD_PLUGINS`` (or the ``loadPlugins`` resource) to a + comma-separated list of fully qualified names loads exactly those + plugins, scoped or not, at ``Main`` startup. A name with no matching + manifest, or a scoped entry whose owning library is not registered, + throws ``BadValue``. + +Disabling plugins +----------------- + +Setting ``$DISABLE_PLUGINS`` (or the ``disablePlugins`` resource) to a +comma- or colon-separated list of fully qualified plugin names +suppresses both auto-load and scoped load for those plugins. + +Subclassing ``Main`` +-------------------- + +A subclass that should not perform global plugin auto-load passes +``false`` for the ``autoLoadPlugins`` constructor argument:: + + class MyMain : public eckit::Main { + public: + MyMain(int argc, char** argv) : + Main(argc, argv, /*homeenv=*/nullptr, /*autoLoadPlugins=*/false) {} + }; + +The default value of ``true`` is expected to flip to ``false`` in a +future release once existing plugins have migrated to scoped +(``for-library``) manifests. + +Migration notes +--------------- + +Behavioural changes introduced with the scoped-plugin support: + +* ``LibraryManager::autoLoadPlugins({})`` no longer loads scoped + plugins. Previously every discovered manifest was loaded + indiscriminately. Library authors must now load their scoped plugins + via ``loadPluginsFor`` from their library's initialisation path. +* A name listed in ``$LOAD_PLUGINS`` that has no matching manifest now + throws ``BadValue``. Previously the loader emitted a warning and + continued. +* A manifest declaring ``version:`` whose value differs from the + installed shared object's :cpp:func:`Plugin::version` now causes the + shared object to be unloaded again and ``BadValue`` is thrown. This + helps detect cases where a manifest on disk no longer matches the + binary that satisfies its ``library:`` entry. +* :cpp:func:`Plugin::finalise` may be invoked even when + :cpp:func:`Plugin::init` was never called (the rollback path of a + failed ``version:`` pin check). Implementations must tolerate this. diff --git a/docs/content/string_tools.rst b/docs/content/string_tools.rst new file mode 100644 index 000000000..2f6a25037 --- /dev/null +++ b/docs/content/string_tools.rst @@ -0,0 +1,201 @@ +String tools +============ + +:cpp:class:`eckit::StringTools` is a small static utility class that +collects the ``std::string`` manipulation primitives the rest of eckit +relies on. It lives in ``eckit/utils/StringTools.h`` and is part of the +core ``eckit`` library — no extra dependency is needed to use it. + +The class has a deleted default constructor: every entry point is a +``static`` member. Most operations return a new ``std::string`` and leave +their inputs untouched; the lazy streaming helper :ref:`stringtools-joinostream` +is the one exception and returns a non-owning proxy instead. + +Quick reference +--------------- + +.. list-table:: + :widths: 30 70 + :header-rows: 1 + + * - Function + - Purpose + * - :cpp:func:`eckit::StringTools::upper`, + :cpp:func:`eckit::StringTools::lower` + - ASCII case conversion (returns a new string). + * - :cpp:func:`eckit::StringTools::trim`, + :cpp:func:`eckit::StringTools::front_trim`, + :cpp:func:`eckit::StringTools::back_trim` + - Strip whitespace (or a custom set of characters) from both ends, + the front, or the back of a string. + * - :cpp:func:`eckit::StringTools::startsWith`, + :cpp:func:`eckit::StringTools::beginsWith`, + :cpp:func:`eckit::StringTools::endsWith` + - Prefix / suffix tests; ``startsWith`` and ``beginsWith`` are + synonyms. + * - :cpp:func:`eckit::StringTools::split` + - Split a string on every occurrence of a delimiter character set, + returning a ``std::vector``. + * - :cpp:func:`eckit::StringTools::join` + - Concatenate a sequence of strings into a single ``std::string``, + inserting a delimiter between consecutive elements. + * - :cpp:func:`eckit::StringTools::joinOstream` + - As ``join``, but lazily streamed to an existing ``std::ostream`` + without ever materialising a temporary ``std::string``. + * - :cpp:func:`eckit::StringTools::substitute`, + :cpp:func:`eckit::StringTools::listVariables` + - Substitute ``{name}`` placeholders from a dictionary; or list the + placeholders found in a template. + * - :cpp:func:`eckit::StringTools::isQuoted`, + :cpp:func:`eckit::StringTools::unQuote` + - Detect and strip matching surrounding quote characters. + +Case conversion and trimming +---------------------------- + +.. code-block:: cpp + + #include "eckit/utils/StringTools.h" + + const std::string s = " Hello, World! "; + + eckit::StringTools::trim(s); // "Hello, World!" + eckit::StringTools::front_trim(s); // "Hello, World! " + eckit::StringTools::back_trim(s); // " Hello, World!" + + eckit::StringTools::trim("xxxhelloxxx", "x"); // "hello" + eckit::StringTools::lower("Hello"); // "hello" + eckit::StringTools::upper("Hello"); // "HELLO" + +The two-argument trim variants take a string of *characters to strip*, +not a substring; ``trim("0001000", "0") == "1"``. + +Searching and splitting +----------------------- + +.. code-block:: cpp + + eckit::StringTools::startsWith("/etc/passwd", "/etc/"); // true + eckit::StringTools::endsWith("file.tar.gz", ".gz"); // true + + auto parts = eckit::StringTools::split(":/", "a:b/c:d"); + // parts == { "a", "b", "c", "d" } + +The delimiter passed to ``split`` is treated as a *set of characters*: +any character in the delimiter string ends the current token. Empty +tokens between consecutive delimiters are collapsed. + +Joining +------- + +``StringTools::join`` builds a single ``std::string`` from any iterable +container of strings: + +.. code-block:: cpp + + std::vector v{"alpha", "beta", "gamma"}; + + eckit::StringTools::join(", ", v); + // "alpha, beta, gamma" + + eckit::StringTools::join("/", v.begin() + 1, v.end()); + // "beta/gamma" + +Both the iterator-pair and the container overloads are templates, so a +``std::list``, ``std::set``, or any other range-iterable container of +strings works without conversion. + +.. _stringtools-joinostream: + +Lazy joining for streams +------------------------ + +When the result of a join is going to be written to an ``std::ostream`` +(such as a log channel or a ``std::ostringstream``), allocating a +temporary ``std::string`` first is wasteful. +:cpp:func:`eckit::StringTools::joinOstream` returns a small proxy object +that streams the elements one at a time, separated by the delimiter, the +moment it is itself streamed: + +.. code-block:: cpp + + #include "eckit/log/Log.h" + #include "eckit/utils/StringTools.h" + + std::vector values{"alpha", "beta", "gamma"}; + + eckit::Log::error() << "values: [" + << eckit::StringTools::joinOstream(values, ", ") + << "]" << std::endl; + // values: [alpha, beta, gamma] + +The proxy is generic over both the container type and the element type; +anything streamable with ``operator<<`` works, so numeric containers and +custom types are equally fine: + +.. code-block:: cpp + + std::set s{"gamma", "alpha", "beta"}; + std::vector nums{1, 2, 3, 4}; + + std::ostringstream oss; + oss << eckit::StringTools::joinOstream(s, "-") << '\n' + << eckit::StringTools::joinOstream(nums, ", "); + // alpha-beta-gamma + // 1, 2, 3, 4 + +The delimiter is a ``std::string_view``, so ``const char *``, +``std::string``, and any ``string_view``-convertible type are all +accepted with no copy. + +The proxy holds *references* to the container and the delimiter, so it +must be consumed in the same full-expression in which it is created. +This is the natural ``os << joinOstream(...)`` idiom; the type is +intentionally non-copyable to make accidental misuse harder. **Do not** +store the proxy in a variable that outlives the source container. + +Template substitution +--------------------- + +``StringTools::substitute`` performs ``{name}``-style placeholder +expansion against a ``std::map``. ``listVariables`` returns the names +that appear in such a template, in left-to-right order with duplicates +preserved: + +.. code-block:: cpp + + std::map m{ + {"user", "alice"}, + {"host", "example.org"}, + }; + + eckit::StringTools::substitute("{user}@{host}", m); + // "alice@example.org" + + eckit::StringTools::listVariables("{user}@{host}/{user}"); + // { "user", "host", "user" } + +Unbalanced braces, nested braces, and references to keys missing from +the dictionary all raise an exception derived from +:cpp:class:`eckit::Exception`. + +Quoting +------- + +``isQuoted`` returns true when a string starts and ends with a matching +``'`` or ``"`` character; ``unQuote`` strips one such pair off: + +.. code-block:: cpp + + eckit::StringTools::isQuoted("\"hello\""); // true + eckit::StringTools::isQuoted("'hello'"); // true + eckit::StringTools::isQuoted("'hello\""); // false (mismatched) + eckit::StringTools::unQuote("\"hello\""); // "hello" + +Mismatched or absent quotes leave the input unchanged. + +API reference +------------- + +.. doxygenclass:: eckit::StringTools + :members: diff --git a/docs/index.rst b/docs/index.rst index c6ced12b2..24f5f5d8d 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -9,4 +9,6 @@ Welcome to EcKit’s Documentation! :caption: Contents content/concepts + content/plugins + content/string_tools content/reference diff --git a/src/eckit/runtime/Main.cc b/src/eckit/runtime/Main.cc index fee23abae..9b0d32633 100644 --- a/src/eckit/runtime/Main.cc +++ b/src/eckit/runtime/Main.cc @@ -9,7 +9,6 @@ */ #include -#include #include #include @@ -24,7 +23,6 @@ #include "eckit/system/LibraryManager.h" #include "eckit/system/SystemInfo.h" #include "eckit/thread/AutoLock.h" -#include "eckit/thread/Mutex.h" #include "eckit/thread/StaticMutex.h" #include "eckit/utils/Translator.h" @@ -37,7 +35,7 @@ static Main* instance_ = nullptr; //---------------------------------------------------------------------------------------------------------------------- -Main::Main(int argc, char** argv, const char* homeenv) : +Main::Main(int argc, char** argv, const char* homeenv, bool autoLoadPlugins) : taskID_(-1), argc_(argc), argv_(argv), home_("/"), debug_(false) { if (instance_) { @@ -121,12 +119,13 @@ Main::Main(int argc, char** argv, const char* homeenv) : } } - // Load eckit::Plugin libraries - std::vector plugins = Resource>("$LOAD_PLUGINS;loadPlugins", {}); - bool autoLoadPlugins = Resource("$AUTO_LOAD_PLUGINS;autoLoadPlugins;-autoLoadPlugins", true); - if (autoLoadPlugins or plugins.size()) { - Log::debug() << "Configured to load plugins " << plugins << std::endl; - system::LibraryManager::autoLoadPlugins(plugins); + if (autoLoadPlugins) { + std::vector plugins = Resource>("$LOAD_PLUGINS;loadPlugins", {}); + const bool autoLoadResource = Resource("$AUTO_LOAD_PLUGINS;autoLoadPlugins;-autoLoadPlugins", true); + if (autoLoadResource || !plugins.empty()) { + Log::debug() << "Configured to load plugins " << plugins << std::endl; + system::LibraryManager::autoLoadPlugins(plugins); + } } Log::debug() << "Application " << name_ << " loaded libraries: " << system::LibraryManager::list() << std::endl; diff --git a/src/eckit/runtime/Main.h b/src/eckit/runtime/Main.h index 0ccdf0837..811beec3c 100644 --- a/src/eckit/runtime/Main.h +++ b/src/eckit/runtime/Main.h @@ -25,7 +25,13 @@ class PathName; class Main { protected: - Main(int argc, char** argv, const char* homeenv = nullptr); + /// @param autoLoadPlugins When true (default), Main consults the + /// $AUTO_LOAD_PLUGINS / $LOAD_PLUGINS resources and triggers plugin + /// loading at construction. Subclasses that should not perform global + /// auto-load can pass false; the default is expected to flip to false + /// in a future release once existing plugins have migrated to scoped + /// (for-library) manifests. + Main(int argc, char** argv, const char* homeenv = nullptr, bool autoLoadPlugins = true); Main(const Main&) = delete; Main& operator=(const Main&) = delete; diff --git a/src/eckit/system/Library.cc b/src/eckit/system/Library.cc index 3e490ba41..7f4625340 100644 --- a/src/eckit/system/Library.cc +++ b/src/eckit/system/Library.cc @@ -23,7 +23,6 @@ #include "eckit/exception/Exceptions.h" #include "eckit/filesystem/LocalPathName.h" #include "eckit/log/Log.h" -#include "eckit/log/OStreamTarget.h" #include "eckit/log/PrefixTarget.h" #include "eckit/os/System.h" #include "eckit/system/LibraryManager.h" @@ -195,6 +194,10 @@ void Library::print(std::ostream& out) const { << "name=" << name_ << ", path=" << libraryPath() << ", prefix=" << prefixDirectory() << ")"; } +std::vector Library::pluginManifestPaths() const { + return {"~" + name_ + "/share/plugins"}; +} + //---------------------------------------------------------------------------------------------------------------------- const void* Library::addr() const { diff --git a/src/eckit/system/Library.h b/src/eckit/system/Library.h index 007b4dcd7..4ae9090a8 100644 --- a/src/eckit/system/Library.h +++ b/src/eckit/system/Library.h @@ -17,6 +17,7 @@ #include #include #include +#include #include "eckit/thread/Mutex.h" @@ -51,6 +52,10 @@ class Library { virtual std::string expandPath(const std::string& path) const; + /// @brief Paths to scan for plugin manifest files for this library. + /// Default returns {"~/share/plugins"}. + virtual std::vector pluginManifestPaths() const; + std::string libraryPath() const; virtual std::string version() const = 0; diff --git a/src/eckit/system/LibraryManager.cc b/src/eckit/system/LibraryManager.cc index 784cd1eac..5e12fa1ac 100644 --- a/src/eckit/system/LibraryManager.cc +++ b/src/eckit/system/LibraryManager.cc @@ -14,13 +14,14 @@ #include #include #include +#include +#include #include // for dlopen #include // for PATH_MAX #include "eckit/system/LibraryManager.h" - #include "eckit/config/LocalConfiguration.h" #include "eckit/config/Resource.h" #include "eckit/config/YAMLConfiguration.h" @@ -29,21 +30,28 @@ #include "eckit/filesystem/LocalPathName.h" #include "eckit/filesystem/PathName.h" #include "eckit/log/Log.h" -#include "eckit/log/OStreamTarget.h" -#include "eckit/log/PrefixTarget.h" -#include "eckit/os/System.h" #include "eckit/system/Library.h" #include "eckit/system/Plugin.h" #include "eckit/system/SystemInfo.h" #include "eckit/thread/AutoLock.h" #include "eckit/thread/Mutex.h" #include "eckit/utils/Tokenizer.h" -#include "eckit/utils/Translator.h" namespace eckit::system { //---------------------------------------------------------------------------------------------------------------------- +bool PluginManifest::matchesTags(const std::vector& required) const { + for (const auto& tag : required) { + if (std::find(tags.begin(), tags.end(), tag) == tags.end()) { + return false; + } + } + return true; +} + +//---------------------------------------------------------------------------------------------------------------------- + static std::string path_from_libhandle(const std::string& libname, void* handle) { #if eckit_HAVE_DLINFO char path[PATH_MAX]; @@ -60,6 +68,68 @@ static std::string path_from_libhandle(const std::string& libname, void* handle) //---------------------------------------------------------------------------------------------------------------------- +static PluginManifest parseManifest(const LocalConfiguration& conf) { + PluginManifest m; + m.name = conf.getString("name"); + m.ns = conf.getString("namespace"); + m.library = conf.getString("library"); + + if (conf.has("for-library")) { + m.forLibrary = conf.getString("for-library"); + } + if (conf.has("min-version")) { + m.minVersion = conf.getString("min-version"); + } + if (conf.has("version")) { + m.version = conf.getString("version"); + } + if (conf.has("tags")) { + m.tags = conf.getStringVector("tags"); + } + return m; +} + +//---------------------------------------------------------------------------------------------------------------------- + +/// Parse a semver-ish version string into (major, minor, patch). +/// Accepts "major.minor" (patch defaults to 0) or "major.minor.patch". +/// Each component must be a non-empty run of digits parsable as an int. +/// Returns true on success, false on any other input (including overflow). +static bool parseSemver(const std::string& v, int& major, int& minor, int& patch) { + major = minor = patch = 0; + + std::vector parts; + Tokenizer(".")(v, parts); + if (parts.size() < 2 || parts.size() > 3) { + return false; + } + + try { + size_t consumed = 0; + major = std::stoi(parts[0], &consumed); + if (consumed != parts[0].size()) { + return false; + } + minor = std::stoi(parts[1], &consumed); + if (consumed != parts[1].size()) { + return false; + } + if (parts.size() == 3) { + patch = std::stoi(parts[2], &consumed); + if (consumed != parts[2].size()) { + return false; + } + } + } + catch (const std::exception&) { + // std::invalid_argument (non-numeric) or std::out_of_range (overflow) + return false; + } + return true; +} + +//---------------------------------------------------------------------------------------------------------------------- + /// Registry for all libraries /// class LibraryRegistry { @@ -222,7 +292,7 @@ class LibraryRegistry { return nullptr; } - Plugin& loadPlugin(const std::string& name, const std::string& libname = std::string()) { + Plugin& loadPlugin(const std::string& name, const std::string& libname = std::string(), bool initialise = true) { AutoLock lockme(mutex_); @@ -245,7 +315,9 @@ class LibraryRegistry { if (plugin) { Log::debug() << "Loaded plugin [" << name << "] from library [" << lib << "]" << std::endl; plugin->handle(libhandle); - initPlugin(plugin); + if (initialise) { + initPlugin(plugin); + } return *plugin; } // If the plugin library still doesn't exist after a successful call of dlopen, then @@ -259,7 +331,9 @@ class LibraryRegistry { if (plugin) { // Plugin is already loaded, likely because it is explicitly linked into the executable Log::debug() << "Plugin [" << name << "] already loaded" << std::endl; - initPlugin(plugin); + if (initialise) { + initPlugin(plugin); + } return *plugin; } @@ -268,6 +342,60 @@ class LibraryRegistry { throw UnexpectedState(ss.str(), Here()); } + /// Load a plugin from a manifest, performing version check and setting metadata + Plugin& loadPluginFromManifest(const PluginManifest& manifest, bool requireOwningLibrary = false) { + // Version check: if for-library and min-version are set, verify the library meets the minimum + if (!manifest.forLibrary.empty()) { + if (!exists(manifest.forLibrary)) { + if (requireOwningLibrary) { + std::ostringstream ss; + ss << "Plugin " << manifest.fqName() << " requires owning library " << manifest.forLibrary + << " but it is not registered"; + throw BadValue(ss.str(), Here()); + } + } + else if (!manifest.minVersion.empty()) { + const Library& owningLib = lookup(manifest.forLibrary); + std::string actualVersion = owningLib.version(); + int cmp = LibraryManager::compareVersions(actualVersion, manifest.minVersion); + if (cmp < 0) { + std::ostringstream ss; + ss << "Plugin " << manifest.fqName() << " requires " << manifest.forLibrary + << " >= " << manifest.minVersion << " but found version " << actualVersion; + throw BadValue(ss.str(), Here()); + } + } + } + + const bool wasAlreadyLoaded = exists(manifest.library); + + Plugin& plugin = loadPlugin(manifest.name, manifest.library, false); + + if (!manifest.version.empty() && plugin.version() != manifest.version) { + std::ostringstream ss; + ss << "Plugin " << manifest.fqName() << " self-reported version " << plugin.version() + << " does not match manifest version " << manifest.version; + + if (!wasAlreadyLoaded) { + const std::string pluginName = plugin.name(); + try { + unloadPlugin(pluginName); + } + catch (const std::exception& e) { + // Do not let a rollback failure mask the original cause. + Log::error() << "Failed to roll back plugin " << manifest.fqName() + << " after manifest version mismatch: " << e.what() << std::endl; + } + } + + throw BadValue(ss.str(), Here()); + } + + LibraryManager::setPluginManifestMetadata(plugin, manifest.forLibrary, manifest.tags); + initPlugin(&plugin); + return plugin; + } + bool unloadPlugin(const std::string& name) { Plugin* plugin = lookupPlugin(name); if (plugin) { @@ -297,15 +425,24 @@ class LibraryRegistry { std::vector pluginManifestScanPaths() { std::vector scanPaths; - eckit::Tokenizer tokenize(":"); static std::string pluginManifestPath = Resource("$PLUGINS_MANIFEST_PATH;pluginManifestPath", ""); - tokenize(pluginManifestPath, scanPaths); + Tokenizer(":")(pluginManifestPath, scanPaths); for (const auto& p : pluginSearchPaths()) { scanPaths.push_back(p + "/share/plugins"); } + // Collect manifest paths from all registered libraries + { + AutoLock lockme(mutex_); + for (const auto& kv : libs_) { + for (const auto& path : kv.second->pluginManifestPaths()) { + scanPaths.push_back(path); + } + } + } + // always scan ~eckit/share/plugins and ~/share/plugins as a last resort scanPaths.push_back("~eckit/share/plugins"); scanPaths.push_back("~/share/plugins"); @@ -313,8 +450,8 @@ class LibraryRegistry { return scanPaths; } - std::map scanManifestPaths() { - std::map manifests; + std::map scanManifestPaths() { + std::map manifests; std::vector scanPaths = pluginManifestScanPaths(); Log::debug() << "Plugins manifest candidate paths " << scanPaths << std::endl; @@ -344,21 +481,20 @@ class LibraryRegistry { std::vector dirs; realdir.children(files, dirs); for (const auto& p : files) { - PathName path(p); - Log::debug() << "Found plugin manifest " << path << std::endl; - YAMLConfiguration conf(path); + PathName fpath(p); + Log::debug() << "Found plugin manifest " << fpath << std::endl; + YAMLConfiguration conf(fpath); if (conf.has("plugin")) { - LocalConfiguration manifest = conf.getSubConfiguration("plugin"); - Log::debug() << "Loaded plugin manifest " << manifest << std::endl; - std::string name = manifest.getString("name"); - std::string namespce = manifest.getString("namespace"); - std::string fullQualifiedName = namespce + "." + name; + LocalConfiguration manifestConf = conf.getSubConfiguration("plugin"); + Log::debug() << "Loaded plugin manifest " << manifestConf << std::endl; + PluginManifest manifest = parseManifest(manifestConf); + std::string fullQualifiedName = manifest.fqName(); if (manifests.find(fullQualifiedName) == manifests.end()) { manifests[fullQualifiedName] = manifest; } else { Log::debug() << "The plugin " << fullQualifiedName - << " was already found before, skipping plugin defined in " << path << std::endl; + << " was already found before, skipping plugin defined in " << fpath << std::endl; } } } @@ -370,10 +506,11 @@ class LibraryRegistry { void autoLoadPlugins(const std::vector& inlist) { std::vector plugins = inlist; + const bool explicitList = !plugins.empty(); AutoLock lockme(mutex_); - std::map manifests = scanManifestPaths(); + std::map manifests = scanManifestPaths(); // if no plugins configured we load all what was found in the manifests if (plugins.empty()) { @@ -386,21 +523,67 @@ class LibraryRegistry { // loop over full qualified plugin names for (const auto& fqname : plugins) { - if (manifests.find(fqname) != manifests.end()) { - LocalConfiguration manifest = manifests[fqname]; - std::string name = manifest.getString("name"); - std::string namespce = manifest.getString("namespace"); - std::string fullQualifiedName = namespce + "." + name; - ASSERT(fqname == fullQualifiedName); - std::string lib = manifest.getString("library"); - Plugin& plugin = loadPlugin(name, lib); + if (LibraryManager::isPluginDisabled(fqname)) { + Log::debug() << "Plugin " << fqname << " is disabled" << std::endl; + continue; } - else { - Log::warning() << "Could not find manifest file for plugin " << fqname << std::endl; + auto it = manifests.find(fqname); + if (it != manifests.end()) { + const PluginManifest& manifest = it->second; + ASSERT(fqname == manifest.fqName()); + + // Skip scoped plugins during broad auto-load. Explicit LOAD_PLUGINS entries + // are different: the user asked for this exact plugin, so load it if its + // owning library is registered and satisfies any min-version constraint. + if (!explicitList && !manifest.forLibrary.empty()) { + Log::debug() << "Skipping scoped plugin " << fqname << " (for-library: " << manifest.forLibrary + << ") during auto-load" << std::endl; + continue; + } + + // When the user asks for a scoped plugin by name, the owning library must + // already be registered - otherwise we'd silently load a plugin whose + // version constraint cannot be checked. + const bool requireOwningLibrary = explicitList && !manifest.forLibrary.empty(); + loadPluginFromManifest(manifest, requireOwningLibrary); + continue; } + + if (explicitList) { + throw BadValue("Could not find manifest file for plugin " + fqname, Here()); + } + Log::warning() << "Could not find manifest file for plugin " << fqname << std::endl; } } + std::vector> loadPluginsFor(const std::string& libraryName, + const std::vector& tags) { + std::vector> loaded; + + AutoLock lockme(mutex_); + + std::map manifests = scanManifestPaths(); + + for (const auto& kv : manifests) { + const PluginManifest& manifest = kv.second; + + if (manifest.forLibrary != libraryName) { + continue; + } + if (!manifest.matchesTags(tags)) { + continue; + } + if (LibraryManager::isPluginDisabled(manifest.fqName())) { + Log::debug() << "Plugin " << manifest.fqName() << " is disabled" << std::endl; + continue; + } + + Plugin& plugin = loadPluginFromManifest(manifest); + loaded.push_back(plugin); + } + + return loaded; + } void enregisterPlugin(const std::string& name, const std::string& libname) { AutoLock lockme(mutex_); @@ -419,8 +602,8 @@ class LibraryRegistry { } void addPluginSearchPath(const std::string& path) { - Tokenizer tokenizer(":"); - tokenizer(path, plugin_search_paths_); + AutoLock lockme(mutex_); + Tokenizer(":")(path, plugin_search_paths_); } private: // members @@ -478,6 +661,11 @@ void LibraryManager::autoLoadPlugins(const std::vector& plugins) { LibraryRegistry::instance().autoLoadPlugins(plugins); } +std::vector> LibraryManager::loadPluginsFor(const std::string& libraryName, + const std::vector& tags) { + return LibraryRegistry::instance().loadPluginsFor(libraryName, tags); +} + void LibraryManager::enregisterPlugin(const std::string& name, const std::string& libname) { LibraryRegistry::instance().enregisterPlugin(name, libname); } @@ -490,6 +678,44 @@ void LibraryManager::addPluginSearchPath(const std::string& path) { LibraryRegistry::instance().addPluginSearchPath(path); } +int LibraryManager::compareVersions(const std::string& a, const std::string& b) { + int aMajor, aMinor, aPatch; + int bMajor, bMinor, bPatch; + + if (!parseSemver(a, aMajor, aMinor, aPatch)) { + throw BadValue("Invalid version string: " + a, Here()); + } + if (!parseSemver(b, bMajor, bMinor, bPatch)) { + throw BadValue("Invalid version string: " + b, Here()); + } + + if (aMajor != bMajor) { + return aMajor - bMajor; + } + if (aMinor != bMinor) { + return aMinor - bMinor; + } + return aPatch - bPatch; +} + +bool LibraryManager::isPluginDisabled(const std::string& fqName) { + static std::vector disabled = [] { + std::string env = Resource("disablePlugins;$DISABLE_PLUGINS", ""); + std::vector result; + if (!env.empty()) { + Tokenizer(",:")(env, result); + } + return result; + }(); + + return std::find(disabled.begin(), disabled.end(), fqName) != disabled.end(); +} + +void LibraryManager::setPluginManifestMetadata(Plugin& plugin, const std::string& forLibrary, + const std::vector& tags) { + plugin.setManifestMetadata(forLibrary, tags); +} + //---------------------------------------------------------------------------------------------------------------------- } // namespace eckit::system diff --git a/src/eckit/system/LibraryManager.h b/src/eckit/system/LibraryManager.h index 574e08c04..02fd0d3be 100644 --- a/src/eckit/system/LibraryManager.h +++ b/src/eckit/system/LibraryManager.h @@ -13,16 +13,36 @@ #pragma once +#include #include #include #include -#include "eckit/filesystem/LocalPathName.h" - namespace eckit::system { class Library; class Plugin; +class LibraryRegistry; + +//---------------------------------------------------------------------------------------------------------------------- + +/// @brief Parsed plugin manifest metadata +struct PluginManifest { + std::string name; ///< Plugin name (matches Plugin("name") in C++) + std::string ns; ///< Namespace for fully-qualified name + std::string library; ///< Shared library filename stem (no lib prefix / .so extension) + std::string forLibrary; ///< Owning library name, empty means global/unscoped + std::string minVersion; ///< Minimum version of forLibrary required, empty means no check + std::string version; ///< Expected exact value of the plugin's self-reported version() + ///< (compared verbatim, not as semver). Empty means no check. + std::vector tags; ///< Arbitrary labels for filtering + + /// @brief Fully-qualified plugin name: namespace.name + std::string fqName() const { return ns + "." + name; } + + /// @brief Check if this plugin has all of the given tags + bool matchesTags(const std::vector& required) const; +}; //---------------------------------------------------------------------------------------------------------------------- @@ -78,10 +98,48 @@ class LibraryManager { /// @returns Plugin object loaded static Plugin& loadPlugin(const std::string& name, const std::string& library = std::string()); - /// @brief Scans and Auto loads Plugins - /// @param [in] dir path to scan for plugin manifests + /// @brief Scans manifest paths and loads plugins. + /// + /// Behaviour depends on @p plugins: + /// - empty: discovers all manifests but loads only GLOBAL plugins + /// (those without `for-library` in their manifest). Scoped plugins + /// are skipped here and must be loaded via loadPluginsFor() from + /// their owning library, typically from that library's + /// initialisation path. + /// - non-empty: loads exactly the listed fully-qualified names + /// (`namespace.name`), scoped or not. A name with no matching + /// manifest throws BadValue. A scoped entry whose owning library + /// is not registered also throws. + /// + /// Plugins listed in $DISABLE_PLUGINS are silently skipped in both modes. + /// + /// @see loadPluginsFor + /// @see docs/content/plugins.rst for the plugin manifest schema. + /// @throws BadValue static void autoLoadPlugins(const std::vector& plugins); + /// @brief Load all plugins scoped to the given library, optionally filtering by tags. + /// + /// A manifest is considered iff its `for-library` equals @p libraryName + /// and ALL of @p tags are present in its `tags` list (an empty @p tags + /// matches every scoped manifest for the library). + /// + /// If the manifest declares `min-version`, the registered owning + /// library's version() must satisfy it (semver-ish numeric compare). + /// If the manifest declares `version:`, it is compared verbatim against + /// Plugin::version(); on mismatch the offending shared object is + /// unloaded again (rolling back the dlopen) and BadValue is thrown. + /// Plugins listed in $DISABLE_PLUGINS are skipped. + /// + /// Typically called from a library's initialisation routine. + /// + /// @param [in] libraryName The Library::name() of the owning library + /// @param [in] tags If non-empty, only load plugins that have ALL specified tags + /// @returns Loaded Plugin references + /// @throws BadValue + static std::vector> loadPluginsFor(const std::string& libraryName, + const std::vector& tags = {}); + /// @brief Registers a library as a plugin /// To be called from the Plugin constructor /// @param [in] name Name of the library plugin to register @@ -95,6 +153,24 @@ class LibraryManager { /// @brief Adds plugin search paths /// @param [in] ":" separated list of search paths static void addPluginSearchPath(const std::string& path); + + /// @brief Compare two semver version strings + /// @returns negative if a < b, 0 if equal, positive if a > b + static int compareVersions(const std::string& a, const std::string& b); + + /// @brief Check if a plugin is disabled via $DISABLE_PLUGINS environment variable + /// @param [in] fqName Fully-qualified plugin name to check + /// @returns true if the plugin should be suppressed + static bool isPluginDisabled(const std::string& fqName); + +private: // class methods + + friend class LibraryRegistry; + + /// @brief Apply manifest metadata to a Plugin. + /// Single internal call site keeps Plugin::setManifestMetadata private. + static void setPluginManifestMetadata(Plugin& plugin, const std::string& forLibrary, + const std::vector& tags); }; //---------------------------------------------------------------------------------------------------------------------- diff --git a/src/eckit/system/Plugin.cc b/src/eckit/system/Plugin.cc index 19112cb8c..f6f14c1bb 100644 --- a/src/eckit/system/Plugin.cc +++ b/src/eckit/system/Plugin.cc @@ -17,7 +17,7 @@ namespace eckit::system { //---------------------------------------------------------------------------------------------------------------------- Plugin::Plugin(const std::string& name, const std::string& libname) : - eckit::system::Library(libname.size() ? libname : name), name_(name), libname_(libname.size() ? libname : name) { + Library(libname.empty() ? name : libname), name_(name), libname_(libname.empty() ? name : libname) { LibraryManager::enregisterPlugin(name_, libname_); } @@ -25,6 +25,11 @@ Plugin::~Plugin() { LibraryManager::deregisterPlugin(name_); } +void Plugin::setManifestMetadata(const std::string& forLibrary, const std::vector& tags) { + forLibrary_ = forLibrary; + tags_ = tags; +} + void Plugin::init() {} void Plugin::finalise() {} diff --git a/src/eckit/system/Plugin.h b/src/eckit/system/Plugin.h index abf5ae656..9bcb16133 100644 --- a/src/eckit/system/Plugin.h +++ b/src/eckit/system/Plugin.h @@ -11,14 +11,26 @@ #pragma once #include +#include #include "eckit/system/Library.h" namespace eckit::system { +class LibraryManager; + //---------------------------------------------------------------------------------------------------------------------- -class Plugin : public eckit::system::Library { +/// @brief Base class for an eckit Plugin. +/// +/// A Plugin is a Library backed by a shared object that is typically dlopen'd +/// at runtime. The constructor self-registers the plugin with LibraryManager. +/// When the plugin is loaded through a manifest, LibraryManager applies the +/// manifest metadata (forLibrary, tags) before calling init(), so init() +/// implementations may rely on those accessors. finalise() is invoked before +/// dlclose() and MUST tolerate being called even if init() never ran (this +/// happens on the rollback path of a failed manifest version-pin check). +class Plugin : public Library { public: /// @param [in] name Plugin name @@ -33,10 +45,18 @@ class Plugin : public eckit::system::Library { /// @brief Library name as will be used in file system const std::string& libraryName() const { return libname_; } + /// @brief Owning library name from manifest (empty if global/unscoped) + const std::string& forLibrary() const { return forLibrary_; } + + /// @brief Tags from manifest + const std::vector& tags() const { return tags_; } + /// @brief Initialisation function called after loading the plugin dynamically with dlopen() virtual void init(); - /// @brief Finalisation function called before unloading the plugin dynamically with dlclose() + /// @brief Finalisation function called before unloading the plugin dynamically with dlclose(). + /// Implementations MUST tolerate being called without a prior init() — the rollback + /// path on a failed manifest version-pin check unloads the plugin without initialising. virtual void finalise(); void handle(void* h) { handle_ = h; } @@ -48,8 +68,16 @@ class Plugin : public eckit::system::Library { private: + friend class LibraryManager; + + /// @brief Set owning library and tags from manifest metadata. + /// Reachable only via LibraryManager (friend); see LibraryManager::setPluginManifestMetadata. + void setManifestMetadata(const std::string& forLibrary, const std::vector& tags); + std::string name_; std::string libname_; + std::string forLibrary_; + std::vector tags_; }; //---------------------------------------------------------------------------------------------------------------------- diff --git a/src/eckit/utils/StringTools.h b/src/eckit/utils/StringTools.h index d597f5308..89c3a5006 100644 --- a/src/eckit/utils/StringTools.h +++ b/src/eckit/utils/StringTools.h @@ -11,12 +11,11 @@ /// @author Baudouin Raoult /// @author Tiago Quintino -#ifndef eckit_StringTools_h -#define eckit_StringTools_h - +#pragma once #include #include +#include #include @@ -24,6 +23,50 @@ namespace eckit { //---------------------------------------------------------------------------------------------------------------------- +namespace detail { + +/// @brief Lazy ostream-formatter for a delimited sequence. +/// +/// Produced by StringTools::joinOstream; written to an ostream with operator<< +/// and never materialised as a std::string. Holds a reference to the source +/// container and delimiter, so it must be consumed in the same full-expression +/// in which it was created (the typical `os << join(...)` pattern). It is +/// intentionally non-copyable to make stale references harder to write +/// accidentally. +template +class JoinOstreamProxy { +public: + + JoinOstreamProxy(const Container& container, std::string_view delimiter) : + container_(container), delimiter_(delimiter) {} + + JoinOstreamProxy(const JoinOstreamProxy&) = delete; + JoinOstreamProxy& operator=(const JoinOstreamProxy&) = delete; + JoinOstreamProxy(JoinOstreamProxy&&) = default; + JoinOstreamProxy& operator=(JoinOstreamProxy&&) = delete; + + friend std::ostream& operator<<(std::ostream& os, const JoinOstreamProxy& p) { + bool first = true; + for (const auto& e : p.container_) { + if (!first) { + os << p.delimiter_; + } + os << e; + first = false; + } + return os; + } + +private: + + const Container& container_; + std::string_view delimiter_; +}; + +} // namespace detail + +//---------------------------------------------------------------------------------------------------------------------- + class StringTools { public: @@ -54,6 +97,21 @@ class StringTools { template static std::string join(const std::string&, Iterator begin, Iterator end); + /// @brief Stream a delimited sequence directly to an ostream without building a temporary string. + /// + /// Returns a lightweight proxy that, when streamed, walks @p container and writes each + /// element with `operator<<` separated by @p delimiter. The proxy holds references to + /// both arguments and must be used in the same full-expression in which it was created; + /// do not store it. Typical use: + /// + /// Log::error() << "values: [" << StringTools::joinOstream(values, ", ") << "]"; + /// + /// @tparam Container any range-iterable container whose elements have `operator<<(ostream&, ...)`. + template + static detail::JoinOstreamProxy joinOstream(const Container& container, std::string_view delimiter) { + return detail::JoinOstreamProxy(container, delimiter); + } + static bool startsWith(const std::string& str, const std::string& substr); static bool beginsWith(const std::string& str, const std::string& substr); static bool endsWith(const std::string& str, const std::string& substr); @@ -85,5 +143,3 @@ std::string StringTools::join(const std::string& delimiter, const T& words) { //---------------------------------------------------------------------------------------------------------------------- } // namespace eckit - -#endif diff --git a/tests/system/CMakeLists.txt b/tests/system/CMakeLists.txt index 1e5acee3a..cb742614a 100644 --- a/tests/system/CMakeLists.txt +++ b/tests/system/CMakeLists.txt @@ -2,6 +2,114 @@ file( WRITE "${CMAKE_BINARY_DIR}/etc/eckit/test/test.cfg" "Just for testing" ) +set(_system_library_depends) +if(eckit_HAVE_ECKIT_CMD) + list(APPEND _system_library_depends eckit_cmd) +endif() +if(eckit_HAVE_ECKIT_SQL) + list(APPEND _system_library_depends eckit_sql) +endif() +if(eckit_HAVE_ECKIT_MPI) + list(APPEND _system_library_depends eckit_mpi) +endif() + ecbuild_add_test( TARGET eckit_test_system_library SOURCES test_system_library.cc - LIBS eckit ) + LIBS eckit + DEPENDS ${_system_library_depends} ) + +#---------------------------------------------------------------------------------------------------------------------- +# Test-only plugin shared libraries. +# +# Each entry below is a single CMake string of the form +# | +# encoding the data that varies per test plugin. Every plugin is built from the +# single shared source plugin_template.cc, parameterised at compile time by the +# ECKIT_TEST_PLUGIN_NAME and ECKIT_TEST_PLUGIN_VERSION macros. +# +# Versions are intentionally varied to exercise every component of the semver +# triple (major / minor / patch) and a two-digit field, so a future bug in the +# version comparison or the manifest version-pin check is more likely to be +# caught by these tests. +# +# The libraries land in a dedicated build directory and are NOT installed nor +# linked into the test executables — they are dlopen'd via DYNAMIC_LIBRARY_PATH +# set per-test below, so the loading mechanism is genuinely exercised. +#---------------------------------------------------------------------------------------------------------------------- + +set(_plugin_specs + "disabled-global-plugin|1.0.0" + "disabled-scoped-plugin|1.2.0" + "disable-test-enabled-scoped-plugin|1.2.3" + "manifest-global-plugin|2.0.0" + "manifest-scoped-plugin|2.5.0" + "manifest-tagged-grids-plugin|3.0.1" + "manifest-tagged-visual-plugin|3.4.0" + "manifest-duplicate-plugin|4.0.0" + "version-ok-plugin|5.0.0" + "version-high-plugin|5.1.0" + "version-missing-owner-plugin|6.0.0" + "version-mismatch-plugin|1.0.0" + "main-autoload-default-plugin|7.0.0" + "main-autoload-disabled-plugin|7.1.0" + "main-autoload-explicit-plugin|7.2.4" + "main-autoload-env-disabled-plugin|8.9.10") + +set(_plugin_dir "${CMAKE_CURRENT_BINARY_DIR}/plugins") + +set(_plugin_targets) +foreach(_spec IN LISTS _plugin_specs) + string(REPLACE "|" ";" _pair "${_spec}") + list(GET _pair 0 _p) + list(GET _pair 1 _v) + string(REPLACE "-" "_" _src "${_p}") + set(_target "eckit_test_plugin_${_src}") + + ecbuild_add_library( + TARGET ${_target} + TYPE SHARED + NOINSTALL + SOURCES plugin_template.cc + PRIVATE_DEFINITIONS ECKIT_TEST_PLUGIN_NAME="${_p}" + ECKIT_TEST_PLUGIN_VERSION="${_v}" + PUBLIC_LIBS eckit) + + # Force isolated output directory: the test plugin shared libraries must + # NOT land in ${CMAKE_BINARY_DIR}/lib (the default ecbuild_add_library sets) + # because that would make them visible to other tests/binaries through the + # default loader search paths. They live only here, and are only reachable + # via the per-test DYNAMIC_LIBRARY_PATH environment variable. + set_target_properties(${_target} PROPERTIES + OUTPUT_NAME "${_p}" + LIBRARY_OUTPUT_DIRECTORY "${_plugin_dir}" + LIBRARY_OUTPUT_DIRECTORY_DEBUG "${_plugin_dir}" + LIBRARY_OUTPUT_DIRECTORY_RELEASE "${_plugin_dir}" + LIBRARY_OUTPUT_DIRECTORY_RELWITHDEBINFO "${_plugin_dir}" + LIBRARY_OUTPUT_DIRECTORY_MINSIZEREL "${_plugin_dir}" + ARCHIVE_OUTPUT_DIRECTORY "${_plugin_dir}" + ARCHIVE_OUTPUT_DIRECTORY_DEBUG "${_plugin_dir}" + ARCHIVE_OUTPUT_DIRECTORY_RELEASE "${_plugin_dir}" + ARCHIVE_OUTPUT_DIRECTORY_RELWITHDEBINFO "${_plugin_dir}" + ARCHIVE_OUTPUT_DIRECTORY_MINSIZEREL "${_plugin_dir}") + + list(APPEND _plugin_targets ${_target}) +endforeach() + +#---------------------------------------------------------------------------------------------------------------------- +# Plugin tests +#---------------------------------------------------------------------------------------------------------------------- + +foreach(_test + plugin_manifest_loading + plugin_disable + plugin_version + plugin_main_autoload_default + plugin_main_autoload_disabled + plugin_main_autoload_explicit + plugin_main_autoload_env_disabled) + ecbuild_add_test( TARGET eckit_test_system_${_test} + SOURCES test_system_${_test}.cc + LIBS eckit + DEPENDS ${_plugin_targets} + ENVIRONMENT "DYNAMIC_LIBRARY_PATH=${_plugin_dir}" ) +endforeach() diff --git a/tests/system/plugin_template.cc b/tests/system/plugin_template.cc new file mode 100644 index 000000000..179c094a1 --- /dev/null +++ b/tests/system/plugin_template.cc @@ -0,0 +1,46 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +// Single source for every test plugin shared library. The plugin's name and +// self-reported version are injected by the build via target_compile_definitions +// in tests/system/CMakeLists.txt. Each plugin .so therefore differs only by +// these compile-time strings, removing duplication across the test plugins. + +#include +#include + +#include "eckit/system/Plugin.h" + +#ifndef ECKIT_TEST_PLUGIN_NAME +#error "ECKIT_TEST_PLUGIN_NAME must be defined by the build (see tests/system/CMakeLists.txt)" +#endif +#ifndef ECKIT_TEST_PLUGIN_VERSION +#error "ECKIT_TEST_PLUGIN_VERSION must be defined by the build (see tests/system/CMakeLists.txt)" +#endif + +namespace { + +class TestPlugin : public eckit::system::Plugin { +public: + + TestPlugin() : eckit::system::Plugin(ECKIT_TEST_PLUGIN_NAME) {} + + std::string version() const override { return ECKIT_TEST_PLUGIN_VERSION; } + std::string gitsha1(unsigned int) const override { return "undefined"; } + + void init() override { + std::string env = std::string("ECKIT_TEST_PLUGIN_INIT_") + ECKIT_TEST_PLUGIN_NAME; + ::setenv(env.c_str(), "1", 1); + } +}; + +static TestPlugin test_plugin; + +} // namespace diff --git a/tests/system/plugin_test_helpers.h b/tests/system/plugin_test_helpers.h new file mode 100644 index 000000000..39ea51e68 --- /dev/null +++ b/tests/system/plugin_test_helpers.h @@ -0,0 +1,54 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +/// @author Simon Smart +/// @date May 2026 + +#pragma once + +#include +#include +#include + +#include "eckit/log/Log.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/testing/Test.h" +#include "eckit/utils/StringTools.h" + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- +/// Assert that LibraryManager::loadedPlugins() contains exactly the names in @p expected (order-insensitive). +/// +/// Designed for the plugin tests that need to verify the registry state both BEFORE and AFTER Main +/// construction, where standard testing macros are not available (we are still in main(), no Main yet). +/// On mismatch, prints a diagnostic to std::cerr listing actual vs expected and aborts the process so +/// the test fails loudly rather than silently passing because the wrong plugin happened to be loaded. +/// +/// @param expected the exact set of short plugin names (the C++ Plugin("name") argument) that must be +/// currently registered. Pass {} to assert no plugin is loaded. +/// @param phase a short label printed on failure to identify the call site (e.g. "before Main ctor"). +inline void expectLoadedPluginsEqual(std::vector expected, const char* phase) { + std::vector actual = system::LibraryManager::loadedPlugins(); + std::sort(actual.begin(), actual.end()); + std::sort(expected.begin(), expected.end()); + if (actual == expected) { + return; + } + + Log::error() << "FATAL: loadedPlugins mismatch [" << phase << "]\n" + << " expected: [" << StringTools::joinOstream(expected, ", ") << "]\n" + << " actual: [" << StringTools::joinOstream(actual, ", ") << "]" << std::endl; + EXPECT(actual == expected); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::test diff --git a/tests/system/test_system_library.cc b/tests/system/test_system_library.cc index 3c8da70e7..6a1252b40 100644 --- a/tests/system/test_system_library.cc +++ b/tests/system/test_system_library.cc @@ -8,7 +8,6 @@ * does it submit to any jurisdiction. */ -#include #include #include @@ -25,10 +24,8 @@ using namespace std; using namespace eckit; +using namespace eckit::system; using namespace eckit::testing; -using eckit::system::Library; -using eckit::system::LibraryManager; -using eckit::system::Plugin; namespace eckit::test { @@ -43,7 +40,7 @@ class TestPlugin : Plugin { public: TestPlugin() : Plugin("test-plugin") {} - ~TestPlugin() { std::cout << "~TestPlugin()" << std::endl; } + ~TestPlugin() override { std::cout << "~TestPlugin()" << std::endl; } static const TestPlugin& instance() { static TestPlugin instance; return instance; @@ -136,6 +133,34 @@ CASE("Fails to load a plugin without a eckit::Plugin object") { } #endif +//---------------------------------------------------------------------------------------------------------------------- +// Tests for scoped plugin loading infrastructure +// +// PluginManifest's fqName/matchesTags and LibraryManager::compareVersions are +// covered by the dedicated test_system_plugin_manifest_loading and +// test_system_plugin_version programs respectively; this file only verifies +// the Library/LibraryManager surface that does not require manifest scanning. +//---------------------------------------------------------------------------------------------------------------------- + +CASE("Plugin metadata accessors") { + // The TestPlugin registered above should have empty forLibrary/tags by default + const Plugin& p = LibraryManager::lookupPlugin("test-plugin"); + EXPECT(p.forLibrary().empty()); + EXPECT(p.tags().empty()); +} + +CASE("isPluginDisabled without env var") { + // Without DISABLE_PLUGINS set, nothing should be disabled + EXPECT(!LibraryManager::isPluginDisabled("int.ecmwf.some-plugin")); +} + +CASE("Library pluginManifestPaths default") { + // LibEcKit should return ~eckit/share/plugins by default + auto paths = LibEcKit::instance().pluginManifestPaths(); + EXPECT(paths.size() == 1); + EXPECT(paths[0] == "~eckit/share/plugins"); +} + //---------------------------------------------------------------------------------------------------------------------- diff --git a/tests/system/test_system_plugin_disable.cc b/tests/system/test_system_plugin_disable.cc new file mode 100644 index 000000000..8d3c54d80 --- /dev/null +++ b/tests/system/test_system_plugin_disable.cc @@ -0,0 +1,120 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include +#include + +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::testing; +using system::LibraryManager, system::Plugin; + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- + +CASE("DISABLE_PLUGINS suppresses explicit global auto-load") { + LibraryManager::autoLoadPlugins({"int.ecmwf.test.disabled-global-plugin"}); + + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("disabled-global-plugin"), BadValue); + + // Nothing else may have slipped in via the explicit-list path. + expectLoadedPluginsEqual({}, "after autoLoadPlugins of disabled global"); +} + +CASE("DISABLE_PLUGINS suppresses scoped loading but leaves other plugins loadable") { + auto loaded = LibraryManager::loadPluginsFor("eckit"); + + EXPECT(loaded.size() == 1); + EXPECT(loaded.front().get().name() == "disable-test-enabled-scoped-plugin"); + EXPECT(loaded.front().get().forLibrary() == "eckit"); + // proves the plugin really came through dlopen() of its shared library + EXPECT(loaded.front().get().handle() != nullptr); + + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("disabled-scoped-plugin"), BadValue); + + // The disabled scoped plugin must NOT be in the registry, and only the enabled one is. + expectLoadedPluginsEqual({"disable-test-enabled-scoped-plugin"}, "after loadPluginsFor(eckit)"); +} + +CASE("non-disabled plugin still loads via dlopen (regression guard)") { + // Without this case, a future bug that disables ALL plugins would still pass + // the suppression cases above. This one fails loudly if loading itself broke. + auto loaded = LibraryManager::loadPluginsFor("eckit", {"enabled-scoped"}); + EXPECT(loaded.size() == 1); + EXPECT(loaded.front().get().name() == "disable-test-enabled-scoped-plugin"); + EXPECT(loaded.front().get().forLibrary() == "eckit"); + EXPECT(loaded.front().get().handle() != nullptr); + + // Tag-filtered call did not pull in any extra plugin. + expectLoadedPluginsEqual({"disable-test-enabled-scoped-plugin"}, "after tag-filtered loadPluginsFor"); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::test + +namespace { + +void writeManifest(const eckit::PathName& path, const std::string& body) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << body; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + + writeManifest(manifests / "disabled-global.yaml", R"YAML( +plugin: + name: disabled-global-plugin + namespace: int.ecmwf.test + library: disabled-global-plugin + version: 1.0.0 + tags: [disabled-global] +)YAML"); + + writeManifest(manifests / "disabled-scoped.yaml", R"YAML( +plugin: + name: disabled-scoped-plugin + namespace: int.ecmwf.test + library: disabled-scoped-plugin + for-library: eckit + version: 1.2.0 + tags: [disabled-scoped] +)YAML"); + + writeManifest(manifests / "enabled-scoped.yaml", R"YAML( +plugin: + name: disable-test-enabled-scoped-plugin + namespace: int.ecmwf.test + library: disable-test-enabled-scoped-plugin + for-library: eckit + version: 1.2.3 + tags: [enabled-scoped] +)YAML"); + + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + SetEnv disabled("DISABLE_PLUGINS", "int.ecmwf.test.disabled-global-plugin,int.ecmwf.test.disabled-scoped-plugin"); + + return run_tests(argc, argv); +} diff --git a/tests/system/test_system_plugin_main_autoload_default.cc b/tests/system/test_system_plugin_main_autoload_default.cc new file mode 100644 index 000000000..4b22d2052 --- /dev/null +++ b/tests/system/test_system_plugin_main_autoload_default.cc @@ -0,0 +1,89 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include +#include +#include + +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/runtime/Main.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- + +CASE("Main startup auto-loads global plugins by default") { + const Plugin& plugin = LibraryManager::lookupPlugin("main-autoload-default-plugin"); + const auto& tags = plugin.tags(); + + EXPECT(plugin.forLibrary().empty()); + EXPECT(std::find(tags.begin(), tags.end(), "startup-default") != tags.end()); + EXPECT(plugin.handle() != nullptr); +} + +} // namespace eckit::test + +namespace { + +class TestMain : public eckit::Main { +public: + + TestMain(int argc, char** argv) : Main(argc, argv) {} + ~TestMain() override = default; +}; + +void writeManifest(const eckit::PathName& path) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << R"YAML( +plugin: + name: main-autoload-default-plugin + namespace: int.ecmwf.test + library: main-autoload-default-plugin + version: 7.0.0 + tags: [startup-default] +)YAML"; + ASSERT(out.good()); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + writeManifest(manifests / "main-autoload-default.yaml"); + + SetEnv home("HOME", tmp); + SetEnv eckitHome("ECKIT_HOME", tmp); + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + SetEnv autoLoad("AUTO_LOAD_PLUGINS", "true"); + SetEnv explicitLoad("LOAD_PLUGINS", ""); + + test::expectLoadedPluginsEqual({}, "before Main ctor (autoload default)"); + + TestMain app(argc, argv); + + test::expectLoadedPluginsEqual({"main-autoload-default-plugin"}, "after Main ctor (autoload default)"); + + return run_tests(argc, argv, false); +} diff --git a/tests/system/test_system_plugin_main_autoload_disabled.cc b/tests/system/test_system_plugin_main_autoload_disabled.cc new file mode 100644 index 000000000..a3eab601c --- /dev/null +++ b/tests/system/test_system_plugin_main_autoload_disabled.cc @@ -0,0 +1,80 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include + +#include "eckit/exception/Exceptions.h" +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/runtime/Main.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +CASE("Main subclass can disable global plugin auto-load on startup") { + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("main-autoload-disabled-plugin"), BadValue); +} + +} // namespace eckit::test + +namespace { + +class NoAutoLoadMain : public Main { +public: + + NoAutoLoadMain(int argc, char** argv) : Main(argc, argv, /*homeenv=*/nullptr, /*autoLoadPlugins=*/false) {} + ~NoAutoLoadMain() override = default; +}; + +void writeManifest(const eckit::PathName& path) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << R"YAML( +plugin: + name: main-autoload-disabled-plugin + namespace: int.ecmwf.test + library: main-autoload-disabled-plugin + version: 7.1.0 + tags: [startup-disabled] +)YAML"; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + writeManifest(manifests / "main-autoload-disabled.yaml"); + + SetEnv home("HOME", tmp); + SetEnv eckitHome("ECKIT_HOME", tmp); + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + SetEnv autoLoad("AUTO_LOAD_PLUGINS", "true"); + SetEnv explicitLoad("LOAD_PLUGINS", ""); + + test::expectLoadedPluginsEqual({}, "before Main ctor (autoload ctor-disabled)"); + + NoAutoLoadMain app(argc, argv); + + // Ctor flag autoLoadPlugins=false must fully suppress discovery, even though the env says otherwise. + test::expectLoadedPluginsEqual({}, "after Main ctor (autoload ctor-disabled)"); + + return run_tests(argc, argv, false); +} diff --git a/tests/system/test_system_plugin_main_autoload_env_disabled.cc b/tests/system/test_system_plugin_main_autoload_env_disabled.cc new file mode 100644 index 000000000..50b3789b1 --- /dev/null +++ b/tests/system/test_system_plugin_main_autoload_env_disabled.cc @@ -0,0 +1,73 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include + +#include "eckit/exception/Exceptions.h" +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/runtime/Main.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +CASE("AUTO_LOAD_PLUGINS=false disables Main startup auto-load") { + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("main-autoload-env-disabled-plugin"), BadValue); +} + +} // namespace eckit::test + +namespace { + +void writeManifest(const eckit::PathName& path) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << R"YAML( +plugin: + name: main-autoload-env-disabled-plugin + namespace: int.ecmwf.test + library: main-autoload-env-disabled-plugin + version: 8.9.10 + tags: [startup-env-disabled] +)YAML"; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + writeManifest(manifests / "main-autoload-env-disabled.yaml"); + + SetEnv home("HOME", tmp); + SetEnv eckitHome("ECKIT_HOME", tmp); + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + SetEnv autoLoad("AUTO_LOAD_PLUGINS", "false"); + SetEnv explicitLoad("LOAD_PLUGINS", ""); + + test::expectLoadedPluginsEqual({}, "before Main ctor (autoload env-disabled)"); + + Main::initialise(argc, argv); + + // AUTO_LOAD_PLUGINS=false must suppress discovery just like the ctor flag does. + test::expectLoadedPluginsEqual({}, "after Main ctor (autoload env-disabled)"); + + return run_tests(argc, argv, false); +} diff --git a/tests/system/test_system_plugin_main_autoload_explicit.cc b/tests/system/test_system_plugin_main_autoload_explicit.cc new file mode 100644 index 000000000..94b151c41 --- /dev/null +++ b/tests/system/test_system_plugin_main_autoload_explicit.cc @@ -0,0 +1,87 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include +#include + +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/runtime/Main.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- + +CASE("explicit LOAD_PLUGINS is honoured when global auto-load is disabled") { + const Plugin& plugin = LibraryManager::lookupPlugin("main-autoload-explicit-plugin"); + const auto& tags = plugin.tags(); + + EXPECT(plugin.forLibrary() == "eckit"); + EXPECT(std::find(tags.begin(), tags.end(), "startup-explicit") != tags.end()); + EXPECT(plugin.handle() != nullptr); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::test + +namespace { + +void writeManifest(const eckit::PathName& path) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << R"YAML( +plugin: + name: main-autoload-explicit-plugin + namespace: int.ecmwf.test + library: main-autoload-explicit-plugin + for-library: eckit + min-version: 0.0.0 + version: 7.2.4 + tags: [startup-explicit] +)YAML"; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + writeManifest(manifests / "main-autoload-explicit.yaml"); + + SetEnv home("HOME", tmp); + SetEnv eckitHome("ECKIT_HOME", tmp); + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + SetEnv autoLoad("AUTO_LOAD_PLUGINS", "false"); + SetEnv explicitLoad("LOAD_PLUGINS", "int.ecmwf.test.main-autoload-explicit-plugin"); + + test::expectLoadedPluginsEqual({}, "before Main ctor (autoload explicit list)"); + + Main::initialise(argc, argv); + + // AUTO_LOAD_PLUGINS=false MUST be honoured: nothing else may be loaded apart from the + // single fqname listed in LOAD_PLUGINS. Without this assertion, the test would silently + // pass even if global autoload were broken (the explicit plugin would be loaded too). + test::expectLoadedPluginsEqual({"main-autoload-explicit-plugin"}, + "after Main ctor (autoload explicit list)"); + + return run_tests(argc, argv, false); +} diff --git a/tests/system/test_system_plugin_manifest_loading.cc b/tests/system/test_system_plugin_manifest_loading.cc new file mode 100644 index 000000000..db3fe76aa --- /dev/null +++ b/tests/system/test_system_plugin_manifest_loading.cc @@ -0,0 +1,227 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include +#include +#include +#include + +#include "eckit/exception/Exceptions.h" +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- + +static bool hasTag(const Plugin& plugin, const std::string& tag) { + const auto& tags = plugin.tags(); + return std::find(tags.begin(), tags.end(), tag) != tags.end(); +} + +//---------------------------------------------------------------------------------------------------------------------- + +CASE("PluginManifest fqName") { + PluginManifest m; + m.name = "my-plugin"; + m.ns = "int.ecmwf"; + + EXPECT(m.fqName() == "int.ecmwf.my-plugin"); +} + +CASE("PluginManifest matchesTags requires all requested tags") { + PluginManifest m; + m.tags = {"grids", "interpolation", "io"}; + + EXPECT(m.matchesTags({})); + EXPECT(m.matchesTags({"grids"})); + EXPECT(m.matchesTags({"grids", "io"})); + EXPECT(!m.matchesTags({"visualization"})); + EXPECT(!m.matchesTags({"grids", "visualization"})); + + PluginManifest empty; + EXPECT(empty.matchesTags({})); + EXPECT(!empty.matchesTags({"grids"})); +} + +CASE("autoLoadPlugins loads unscoped manifests and applies metadata") { + LibraryManager::autoLoadPlugins({"int.ecmwf.test.manifest-global-plugin"}); + + const Plugin& plugin = LibraryManager::lookupPlugin("manifest-global-plugin"); + EXPECT(plugin.forLibrary().empty()); + EXPECT(hasTag(plugin, "global")); + EXPECT(plugin.handle() != nullptr); + // The plugin's self-reported version() must equal what the manifest pinned. + // This is the only place we assert the actual version string explicitly; + // every other plugin load in these tests goes through the same manifest + // version-pin check inside LibraryManager::loadPluginFromManifest, so a + // discrepancy anywhere else would surface as a load-time exception. + EXPECT(plugin.version() == "2.0.0"); + + expectLoadedPluginsEqual({"manifest-global-plugin"}, "after autoLoad of manifest-global"); +} + +CASE("autoLoadPlugins loads explicitly requested scoped manifests") { + LibraryManager::autoLoadPlugins({"int.ecmwf.test.manifest-scoped-plugin"}); + + const Plugin& plugin = LibraryManager::lookupPlugin("manifest-scoped-plugin"); + EXPECT(plugin.name() == "manifest-scoped-plugin"); + EXPECT(plugin.forLibrary() == "eckit"); + EXPECT(hasTag(plugin, "scoped-load")); + EXPECT(plugin.handle() != nullptr); + + expectLoadedPluginsEqual({"manifest-global-plugin", "manifest-scoped-plugin"}, "after autoLoad of manifest-scoped"); +} + +CASE("loadPluginsFor loads scoped manifests for the owning library") { + auto loaded = LibraryManager::loadPluginsFor("eckit", {"scoped-load"}); + + EXPECT(loaded.size() == 1); + const Plugin& plugin = loaded.front().get(); + EXPECT(plugin.name() == "manifest-scoped-plugin"); + EXPECT(plugin.forLibrary() == "eckit"); + EXPECT(hasTag(plugin, "scoped-load")); + EXPECT(plugin.handle() != nullptr); + + // Tag-filtered loadPluginsFor must not pull in any other plugin: the registry is unchanged. + expectLoadedPluginsEqual({"manifest-global-plugin", "manifest-scoped-plugin"}, "after loadPluginsFor scoped-load"); +} + +CASE("loadPluginsFor filters scoped manifests by requiring all requested tags") { + auto grids = LibraryManager::loadPluginsFor("eckit", {"grids"}); + EXPECT(grids.size() == 1); + EXPECT(grids.front().get().name() == "manifest-tagged-grids-plugin"); + EXPECT(grids.front().get().handle() != nullptr); + + auto gridsAndIO = LibraryManager::loadPluginsFor("eckit", {"grids", "io"}); + EXPECT(gridsAndIO.size() == 1); + EXPECT(gridsAndIO.front().get().name() == "manifest-tagged-grids-plugin"); + EXPECT(gridsAndIO.front().get().handle() != nullptr); + + auto visual = LibraryManager::loadPluginsFor("eckit", {"visualization"}); + EXPECT(visual.size() == 1); + EXPECT(visual.front().get().name() == "manifest-tagged-visual-plugin"); + EXPECT(visual.front().get().handle() != nullptr); + + auto missing = LibraryManager::loadPluginsFor("eckit", {"missing"}); + EXPECT(missing.empty()); + + // Exactly the two newly-tagged plugins must have been added — neither extra plugins + // from elsewhere on the manifest path nor the {missing} call leaking anything. + expectLoadedPluginsEqual({"manifest-global-plugin", "manifest-scoped-plugin", "manifest-tagged-grids-plugin", + "manifest-tagged-visual-plugin"}, + "after tag-filtered loadPluginsFor calls"); +} + +CASE("first duplicate fully-qualified manifest wins") { + LibraryManager::autoLoadPlugins({"int.ecmwf.test.manifest-duplicate-plugin"}); + + const Plugin& plugin = LibraryManager::lookupPlugin("manifest-duplicate-plugin"); + EXPECT(hasTag(plugin, "first")); + EXPECT(!hasTag(plugin, "second")); + EXPECT(plugin.handle() != nullptr); + + expectLoadedPluginsEqual({"manifest-global-plugin", "manifest-scoped-plugin", "manifest-tagged-grids-plugin", + "manifest-tagged-visual-plugin", "manifest-duplicate-plugin"}, + "after autoLoad of manifest-duplicate"); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::test + +namespace { + +void writeManifest(const PathName& path, const std::string& body) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << body; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests1 = tmp / "manifests1"; + PathName manifests2 = tmp / "manifests2"; + manifests1.mkdir(); + manifests2.mkdir(); + + writeManifest(manifests1 / "global.yaml", R"YAML( +plugin: + name: manifest-global-plugin + namespace: int.ecmwf.test + library: manifest-global-plugin + version: 2.0.0 + tags: [global] +)YAML"); + + writeManifest(manifests1 / "scoped.yaml", R"YAML( +plugin: + name: manifest-scoped-plugin + namespace: int.ecmwf.test + library: manifest-scoped-plugin + for-library: eckit + min-version: 0.0.0 + version: 2.5.0 + tags: [scoped-load] +)YAML"); + + writeManifest(manifests1 / "tagged-grids.yaml", R"YAML( +plugin: + name: manifest-tagged-grids-plugin + namespace: int.ecmwf.test + library: manifest-tagged-grids-plugin + for-library: eckit + version: 3.0.1 + tags: [grids, io] +)YAML"); + + writeManifest(manifests1 / "tagged-visual.yaml", R"YAML( +plugin: + name: manifest-tagged-visual-plugin + namespace: int.ecmwf.test + library: manifest-tagged-visual-plugin + for-library: eckit + version: 3.4.0 + tags: [visualization] +)YAML"); + + writeManifest(manifests1 / "duplicate.yaml", R"YAML( +plugin: + name: manifest-duplicate-plugin + namespace: int.ecmwf.test + library: manifest-duplicate-plugin + version: 4.0.0 + tags: [first] +)YAML"); + + writeManifest(manifests2 / "duplicate.yaml", R"YAML( +plugin: + name: manifest-duplicate-plugin + namespace: int.ecmwf.test + library: manifest-duplicate-plugin + version: 4.0.0 + tags: [second] +)YAML"); + + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", std::string(manifests1) + ":" + std::string(manifests2)); + return run_tests(argc, argv); +} diff --git a/tests/system/test_system_plugin_version.cc b/tests/system/test_system_plugin_version.cc new file mode 100644 index 000000000..235091f99 --- /dev/null +++ b/tests/system/test_system_plugin_version.cc @@ -0,0 +1,168 @@ +/* + * (C) Copyright 1996- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include +#include +#include + +#include "eckit/exception/Exceptions.h" +#include "eckit/filesystem/PathName.h" +#include "eckit/filesystem/TmpDir.h" +#include "eckit/system/LibraryManager.h" +#include "eckit/system/Plugin.h" +#include "eckit/testing/Test.h" + +#include "plugin_test_helpers.h" + +using namespace eckit; +using namespace eckit::system; +using namespace eckit::testing; + +namespace eckit::test { + +//---------------------------------------------------------------------------------------------------------------------- + +CASE("compareVersions handles supported version forms") { + EXPECT(LibraryManager::compareVersions("1.2.3", "1.2.3") == 0); + EXPECT(LibraryManager::compareVersions("2.0.0", "1.9.9") > 0); + EXPECT(LibraryManager::compareVersions("1.9.9", "2.0.0") < 0); + EXPECT(LibraryManager::compareVersions("1.3.0", "1.2.9") > 0); + EXPECT(LibraryManager::compareVersions("1.2.4", "1.2.3") > 0); + EXPECT(LibraryManager::compareVersions("0.1.0", "0.2.0") < 0); + EXPECT(LibraryManager::compareVersions("1.2", "1.2.0") == 0); + EXPECT_THROWS_AS(LibraryManager::compareVersions("abc", "1.0.0"), BadValue); + EXPECT_THROWS_AS(LibraryManager::compareVersions("1", "1.0.0"), BadValue); +} + +CASE("loadPluginsFor accepts compatible min-version") { + auto loaded = LibraryManager::loadPluginsFor("eckit", {"version-ok"}); + + EXPECT(loaded.size() == 1); + EXPECT(loaded.front().get().name() == "version-ok-plugin"); + EXPECT(loaded.front().get().forLibrary() == "eckit"); + EXPECT(loaded.front().get().handle() != nullptr); + + expectLoadedPluginsEqual({"version-ok-plugin"}, "after loadPluginsFor version-ok"); +} + +CASE("loadPluginsFor rejects incompatible min-version") { + EXPECT_THROWS_AS(LibraryManager::loadPluginsFor("eckit", {"version-high"}), BadValue); + + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("version-high-plugin"), BadValue); + + // The rejected plugin must not have leaked into the registry; only the previously-loaded + // version-ok-plugin should still be present. + expectLoadedPluginsEqual({"version-ok-plugin"}, "after rejected version-high load"); +} + +CASE("autoLoadPlugins rejects explicitly requested scoped plugin when owner is missing") { + EXPECT_THROWS_AS(LibraryManager::autoLoadPlugins({"int.ecmwf.test.version-missing-owner-plugin"}), BadValue); + + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("version-missing-owner-plugin"), BadValue); + + expectLoadedPluginsEqual({"version-ok-plugin"}, "after rejected missing-owner autoLoad"); +} + +CASE("min-version is skipped when the owning library is not registered") { + auto loaded = LibraryManager::loadPluginsFor("missing-owner-library", {"missing-owner"}); + + EXPECT(loaded.size() == 1); + EXPECT(loaded.front().get().name() == "version-missing-owner-plugin"); + EXPECT(loaded.front().get().forLibrary() == "missing-owner-library"); + EXPECT(loaded.front().get().handle() != nullptr); + + expectLoadedPluginsEqual({"version-ok-plugin", "version-missing-owner-plugin"}, + "after loadPluginsFor missing-owner-library"); +} + +CASE("loadPluginsFor rejects manifest version mismatching plugin version") { + // version-mismatch-plugin is compiled as 1.0.0 but pinned to 9.9.9 in its manifest. + // The pinned-version check in LibraryRegistry::loadPluginFromManifest must reject + // the load AND roll back the dlopen it triggered, so the plugin must NOT remain + // registered with half-applied state. init() must not have run. + EXPECT_THROWS_AS(LibraryManager::loadPluginsFor("eckit", {"version-mismatch"}), BadValue); + + EXPECT_THROWS_AS(LibraryManager::lookupPlugin("version-mismatch-plugin"), BadValue); + EXPECT(::getenv("ECKIT_TEST_PLUGIN_INIT_version-mismatch-plugin") == nullptr); + + // Rollback must be complete: the registry size is unchanged from before this CASE. + expectLoadedPluginsEqual({"version-ok-plugin", "version-missing-owner-plugin"}, + "after rolled-back version-mismatch load"); +} + +//---------------------------------------------------------------------------------------------------------------------- + +} // namespace eckit::test + +namespace { + +void writeManifest(const PathName& path, const std::string& body) { + std::ofstream out(path.localPath()); + ASSERT(out.good()); + out << body; + ASSERT(out.good()); +} + +} // namespace + +int main(int argc, char** argv) { + TmpDir tmp; + PathName manifests = tmp / "manifests"; + manifests.mkdir(); + + writeManifest(manifests / "version-ok.yaml", R"YAML( +plugin: + name: version-ok-plugin + namespace: int.ecmwf.test + library: version-ok-plugin + for-library: eckit + min-version: 0.0.0 + version: 5.0.0 + tags: [version-ok] +)YAML"); + + writeManifest(manifests / "version-high.yaml", R"YAML( +plugin: + name: version-high-plugin + namespace: int.ecmwf.test + library: version-high-plugin + for-library: eckit + min-version: 999.0.0 + version: 5.1.0 + tags: [version-high] +)YAML"); + + writeManifest(manifests / "version-missing-owner.yaml", R"YAML( +plugin: + name: version-missing-owner-plugin + namespace: int.ecmwf.test + library: version-missing-owner-plugin + for-library: missing-owner-library + min-version: 999.0.0 + version: 6.0.0 + tags: [missing-owner] +)YAML"); + + // Mismatch manifest: pins a version that does NOT match the plugin's compiled + // self-reported version (1.0.0 — see tests/system/CMakeLists.txt). + // Used by the "rejects manifest version mismatching plugin version" CASE. + writeManifest(manifests / "version-mismatch.yaml", R"YAML( +plugin: + name: version-mismatch-plugin + namespace: int.ecmwf.test + library: version-mismatch-plugin + for-library: eckit + version: 9.9.9 + tags: [version-mismatch] +)YAML"); + + SetEnv manifestPath("PLUGINS_MANIFEST_PATH", manifests); + return run_tests(argc, argv); +} diff --git a/tests/utils/test_string_tools.cc b/tests/utils/test_string_tools.cc index 727a1aaa2..af4abefa5 100644 --- a/tests/utils/test_string_tools.cc +++ b/tests/utils/test_string_tools.cc @@ -13,6 +13,10 @@ #include "eckit/types/Types.h" #include "eckit/utils/StringTools.h" +#include +#include +#include + #include "eckit/testing/Test.h" using namespace std; @@ -166,6 +170,59 @@ CASE("back_trim") { EXPECT(StringTools::back_trim(t7, "0") == string("000001")); } +CASE("joinOstream streams a delimited sequence with no temporary string") { + std::vector v{"alpha", "beta", "gamma"}; + + std::ostringstream oss; + oss << StringTools::joinOstream(v, ", "); + EXPECT_EQUAL(oss.str(), "alpha, beta, gamma"); +} + +CASE("joinOstream handles edge cases (empty / single-element)") { + std::vector empty; + std::ostringstream oss; + oss << "[" << StringTools::joinOstream(empty, ", ") << "]"; + EXPECT_EQUAL(oss.str(), "[]"); + + std::vector one{"only"}; + oss.str(""); + oss << StringTools::joinOstream(one, ", "); + EXPECT_EQUAL(oss.str(), "only"); +} + +CASE("joinOstream is generic over container and element type") { + // std::list of strings + std::list l{"a", "b", "c"}; + std::ostringstream oss; + oss << StringTools::joinOstream(l, "|"); + EXPECT_EQUAL(oss.str(), "a|b|c"); + + // std::set: deterministic alphabetic order + std::set s{"gamma", "alpha", "beta"}; + oss.str(""); + oss << StringTools::joinOstream(s, "-"); + EXPECT_EQUAL(oss.str(), "alpha-beta-gamma"); + + // Numeric vector: relies on operator<< for the element type, not std::string conversion. + std::vector nums{1, 2, 3, 4}; + oss.str(""); + oss << StringTools::joinOstream(nums, ", "); + EXPECT_EQUAL(oss.str(), "1, 2, 3, 4"); +} + +CASE("joinOstream accepts string_view-compatible delimiters") { + std::vector v{"x", "y"}; + + std::ostringstream oss; + oss << StringTools::joinOstream(v, "::"); // const char* + EXPECT_EQUAL(oss.str(), "x::y"); + + const std::string sep = " -> "; + oss.str(""); + oss << StringTools::joinOstream(v, sep); // std::string -> string_view + EXPECT_EQUAL(oss.str(), "x -> y"); +} + //---------------------------------------------------------------------------------------------------------------------- } // namespace eckit::test From 55b4a8efa2bd5e3e4910d3943e98f4541fbb98d4 Mon Sep 17 00:00:00 2001 From: Simon Smart Date: Fri, 8 May 2026 21:58:05 +0200 Subject: [PATCH 2/4] EKIT-680: Agent related tooling should be considered separately in its own PR --- AGENTS.md | 103 ----------- CONTRIBUTING.md | 444 ------------------------------------------------ 2 files changed, 547 deletions(-) delete mode 100644 AGENTS.md delete mode 100644 CONTRIBUTING.md diff --git a/AGENTS.md b/AGENTS.md deleted file mode 100644 index 02fae41c4..000000000 --- a/AGENTS.md +++ /dev/null @@ -1,103 +0,0 @@ -# AGENTS.md - -Guidance for AI coding agents working in this repository. **Human contributors -should read [`CONTRIBUTING.md`](CONTRIBUTING.md) first** — that document is -the canonical reference for project structure, style, build/test workflow, -and the C++ source-file template. This file only adds the agent-specific -notes that sit on top of it. - ---- - -## Source of truth - -When the prose in any document disagrees with the build files, **the -executable CMake/workflow files win**. In order of authority: - -1. Root `CMakeLists.txt` and per-module `CMakeLists.txt` files — they define - what is built, gated, installed, and tested. -2. `tests//CMakeLists.txt` — authoritative list of tests and their - environment. -3. `.pre-commit-config.yaml` — authoritative list of lint/format checks. -4. `docs/Makefile`, `.github/workflows/*` — authoritative CI behaviour. -5. `CONTRIBUTING.md` — the human-readable summary; aim to keep it - consistent with the above when you touch related areas. -6. `AGENTS.md` (this file) and `.github/copilot-instructions.md` — agent - guidance; subordinate to everything above. - -If you spot a real divergence, fix the prose, not the build files (unless -the user has asked for a behaviour change). - ---- - -## Required reading before non-trivial changes - -* [`CONTRIBUTING.md`](CONTRIBUTING.md) — full project overview, file - templates, naming, formatting, testing harness, pre-commit checklist. -* The `CMakeLists.txt` of any module you are about to modify, plus its - test directory. - ---- - -## House-keeping invariants agents tend to miss - -These are the recurring traps. Re-check them before declaring a task done. - -* **Tool target ↔ installed binary.** `src/tools/CMakeLists.txt` maps - underscored CMake targets to hyphenated installed names: target - `eckit_version` → installed binary `eckit-version`. Search for both - spellings when looking up usage. -* **Feature gates have two halves.** A new optional feature needs *both* - an `ecbuild_add_option(FEATURE ...)` block in the root `CMakeLists.txt` - *and* an `if(eckit_HAVE_)` guard in each consuming - `CMakeLists.txt` *and* an `eckit_HAVE_` C++ macro in the - generated `eckit_config.h`. Touching one without the others produces - silently broken builds. -* **Generated headers live in the build tree.** `eckit_config.h`, - `eckit_version.h`, and similar are written under `build/src/...` not the - source tree. Compile commands and any tooling you spawn must include - both `src/` and `build/src/` on the include path. -* Avoid unnecessary `eckit::` qualifiers in the code. When inside a - nested namespace (such as `namespace eckit::test`) redundant - qualifiers are just noise. -* **`eckit::Mutex` is recursive.** Nested locking is safe, but do not - assume that of any other mutex type. Recursive calls should be avoided - wherever possible in any case. - ---- - -## Workflow rules for agents - -* **Add tests via `ecbuild_add_test`, never via ad-hoc scripts.** The - CTest integration is the only contract CI honours. -* **Never edit pre-existing style on lines you are not otherwise - modifying.** Cleanups creep, reviews balloon. Limit refactors to the - scope explicitly requested by the user, or open a separate task for - them. -* **Format with `clang-format` directly, not by hand.** Run - `./format-sources.sh` (which enforces version 19) on every C/C++ file - you touched. Do not run it on `CMakeLists.txt`. -* **Verify with the real build, not with reasoning.** After non-trivial - edits, run `cmake --build build -j` and at minimum the tests in the - modules you touched. The user's machine has limited cores; if they - have asked you to use a specific `-j`, respect it. If not specified - do not exceed -j6. -* **Do not commit unless the user explicitly asks.** The default - expectation is "show me the diff, I will commit". -* **Do not push to remote unless the user explicitly asks.** Same as - above. -* **Untracked artefacts** (e.g. `Testing/`, ad-hoc local scratch - directories) should be left alone unless the user asks to clean them - up. - ---- - -## Hand-off - -When pausing or finishing a piece of work, leave the workspace ready for -either the user or another agent to resume: - -* Build is clean (no half-applied edits that fail to compile). -* Tests in the touched module pass; mention any pre-existing failures you - did not address. -* Summarise what changed, why, and what is left, in a few bullets — not - a wall of text. The user will read it; brevity matters. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md deleted file mode 100644 index 876b648a6..000000000 --- a/CONTRIBUTING.md +++ /dev/null @@ -1,444 +0,0 @@ -# Contributing to eckit - -This document is the practical reference for how to add or modify code in this -repository, written so that both new contributors and AI coding agents can -produce changes that match the existing house style on the first try. - -For deeper architectural context, see [`AGENTS.md`](AGENTS.md) and -[`docs/`](docs/). - ---- - -## Project structure - -`eckit` is a C++17 toolkit built with [`ecbuild`](https://github.com/ecmwf/ecbuild) -on top of CMake. The repository is organised as follows: - -| Path | Purpose | -| --- | --- | -| `src/eckit/` | Public sources of the main `eckit` shared library. Each subdirectory is a logical module (`config`, `container`, `filesystem`, `io`, `log`, `runtime`, `system`, `thread`, `utils`, ...). | -| `src/eckit//` | Optional libraries gated by `ecbuild_add_option(FEATURE ...)` in the root `CMakeLists.txt`: `eckit_mpi`, `eckit_linalg`, `eckit_geometry`, `eckit_maths`, `eckit_web`, `eckit_cmd`, `eckit_codec`, `eckit_spec`, `eckit_geo`, `eckit_sql`. Each has its own `CMakeLists.txt`. | -| `src/eckit/testing/` | Header-only testing harness (`Test.h` — `CASE`, `EXPECT`, `EXPECT_EQUAL`, `EXPECT_THROWS_AS`, `SetEnv`, `run_tests`). | -| `src/tools/` | Source of installed CLI tools. CMake target `eckit_foo` produces installed binary `eckit-foo`. | -| `tests//` | One CMakeLists per module. New tests are declared there with `ecbuild_add_test(TARGET eckit_test__ ...)`. | -| `docs/` | Sphinx documentation. `make -j html` from `docs/`; warnings are errors. | -| `python/eckit/` | Cython wrapper. Builds against an already-built C++ eckit. | -| `bamboo/`, `tools/` | CI helpers. | -| `cmake/` | Project-specific CMake modules. | - -Generated headers like `eckit_config.h` and `eckit_version.h` are written into -the build tree. Compilation needs both `src/` and `build/src/` on the include -path; `ecbuild` handles this for in-tree targets. - -### Where to put new code - -* **New class in an existing module** — add `Foo.h` and `Foo.cc` next to its - siblings under `src/eckit//` and append to that module's - `CMakeLists.txt`. -* **New module** — create `src/eckit//` with its own `CMakeLists.txt`, - add an `ecbuild_add_option` entry in the root `CMakeLists.txt`, and - define an `eckit_HAVE_` config macro. -* **New test** — add the source under `tests//` and register it in - that directory's `CMakeLists.txt`. - ---- - -## Build, test, format - -`ecbuild` must be on `PATH` (or discoverable through `ecbuild_ROOT` / -`CMAKE_PREFIX_PATH` if you call CMake directly). - -```sh -mkdir -p build -(cd build && ecbuild --prefix= -- ..) -cmake --build build -j -(cd build && ctest -j --output-on-failure) -``` - -Run a single test by target name: - -```sh -(cd build && ctest -R '^eckit_test_types_uuid$' --output-on-failure) -``` - -For debugging, run the test binary directly from the build dir, e.g. -`./bin/eckit_test_types_uuid`. - -Test targets are declared in `tests//CMakeLists.txt` with -`ecbuild_add_test(TARGET eckit_test__ ...)`. Add new tests -there rather than via ad-hoc scripts. - -Format C/C++ sources with the project script (clang-format major version 19): - -```sh -./format-sources.sh path/to/file.cc path/to/file.h -``` - -`pre-commit` enforces clang-format 19 on C/C++, plus whitespace/LF, isort, -ruff for Python, yamllint, JSON formatting, and cython-lint. - -### Tool target naming - -`src/tools/CMakeLists.txt` maps internal CMake targets that use underscores -to installed binaries that use hyphens. For instance, target `eckit_version` -produces the installed tool `eckit-version`. Keep this in mind when looking -up which CMake target builds a particular CLI tool. - -### Generated headers - -Configured headers like `eckit_config.h` and `eckit_version.h` are written -into the build tree, so include paths must contain both `src/` and -`build/src/`. `ecbuild` wires this up for in-tree targets; external consumers -get it via the installed CMake package config. - ---- - -## Python wrapper - -The `python/eckit` wrapper is Cython-based and expects an already-built -eckit: - -```sh -ECKIT_SOURCE_DIR= ECKIT_BUILD_DIR= python3 setup.py build_ext -i -``` - -`python/eckit/setup.py` links against `eckit` and `eckit_geo`. Override -paths with `ECKIT_LIB_DIR` and `ECKIT_INCLUDE_DIRS` when the default build -layout does not match your setup. - -`python/eckit/build_chain.sh` assumes `uv`, `PYVERSION`, and the -wheelmaker / setup_utils environment; its pytest step is currently commented -out and only `twine check` runs. - ---- - -## File naming and ownership - -* **One class per file**, named after the class. `class FooBar` lives in - `FooBar.h` and `FooBar.cc`. Keep small helper structs and free functions - beside the class they support if they are an implementation detail. -* **Header extension is `.h`**, source extension is `.cc`. Do not use `.hpp`, - `.cpp`, `.cxx`. -* **Test files** are named `test__.cc` and produce a CTest - target `eckit_test__`. -* **Header-only test helpers** live next to the tests that use them, e.g. - `tests/system/plugin_test_helpers.h`. - ---- - -## C++ source file template - -### Common header (`.h` and `.cc`) - -Every source file starts with the ECMWF Apache 2.0 boilerplate, exactly: - -```cpp -/* - * (C) Copyright 1996- ECMWF. - * - * This software is licensed under the terms of the Apache Licence Version 2.0 - * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. - * In applying this licence, ECMWF does not waive the privileges and immunities - * granted to it by virtue of its status as an intergovernmental organisation nor - * does it submit to any jurisdiction. - */ -``` - -A blank line follows, then for **header files** an author/date block: - -```cpp -/// @author Simon Smart -/// @date May 2026 -``` - -The author/date block is conventional on headers and omitted from `.cc` files. -Authorship identifies individuals who are conceptually responsible for and -aware of the relevant code. -Use the month and year you create the file. Multiple `@author` lines are -allowed when several people are responsible. - -### Header file (`.h`) - -```cpp -/* - * (C) Copyright 1996- ECMWF. - * - * This software is licensed under the terms of the Apache Licence Version 2.0 - * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. - * In applying this licence, ECMWF does not waive the privileges and immunities - * granted to it by virtue of its status as an intergovernmental organisation nor - * does it submit to any jurisdiction. - */ - -/// @author Simon Smart -/// @date May 2026 - -#pragma once - -#include -#include - -#include "eckit/system/Library.h" - -namespace eckit::system { - -class LibraryManager; // forward declarations as needed - -//---------------------------------------------------------------------------------------------------------------------- - -/// @brief One-line summary of what this class is. -/// -/// Multi-line description if the contract is non-trivial: lifecycle, threading, -/// preconditions, ownership. Useful to spell out invariants that are not obvious -/// from the signatures. -class Foo { -public: - - /// @param [in] name what the parameter means - explicit Foo(const std::string& name); - - ~Foo(); - - /// @brief Short doxygen on each public method that is not self-evident. - const std::string& name() const { return name_; } - -private: - - friend class LibraryManager; // friend declarations at the top of private: - - std::string name_; -}; - -//---------------------------------------------------------------------------------------------------------------------- - -} // namespace eckit::system -``` - -Key points: - -* **Include guard**: prefer `#pragma once` for new files. Older files - use Older files use old-style include guards - when significant modifications - are made to those files switch them to use `#pragma once`, but otherwise - leave them alone. -* **Includes** are grouped: standard library first (`<...>`), then - third-party, then project (`"eckit/..."`). A blank line separates groups. - The corresponding `.h` of a `.cc` file comes first in that `.cc`. -* **Namespaces** start at column 0 with no indentation of their contents. - Use the C++17 nested form `namespace eckit::system {` rather than nested - blocks. Close with `} // namespace eckit::system` (two spaces before the - comment). -* **Separator lines** of exactly 116 dashes (`//----...`) bracket each major - section: one after the opening `namespace` declaration and one before the - closing brace, plus one between top-level declarations when grouping helps - readability. Match the existing files; clang-format does not generate them. -* **`using namespace` is forbidden in headers.** Use the fully qualified name - or a narrow `using eckit::Foo;` after the namespace open. -* **Doxygen** uses `///` (triple slash) and the `@brief`/`@param`/`@return`/ - `@throws` tags. Document non-obvious behaviour, especially threading, - ownership, and exception guarantees. -* **`friend` declarations** go at the top of the `private:` section. -* **Class layout order** is `public:` then `protected:` then `private:`, with - methods before data members in each. - -### Source file (`.cc`) - -```cpp -/* - * (C) Copyright 1996- ECMWF. - * - * This software is licensed under the terms of the Apache Licence Version 2.0 - * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. - * In applying this licence, ECMWF does not waive the privileges and immunities - * granted to it by virtue of its status as an intergovernmental organisation nor - * does it submit to any jurisdiction. - */ - -#include "eckit/system/Foo.h" - -#include -#include - -#include "eckit/exception/Exceptions.h" -#include "eckit/log/Log.h" - -namespace eckit::system { - -//---------------------------------------------------------------------------------------------------------------------- - -Foo::Foo(const std::string& name) : name_(name) {} - -Foo::~Foo() = default; - -//---------------------------------------------------------------------------------------------------------------------- - -} // namespace eckit::system -``` - -Key differences from the header: - -* **No author/date block** — the convention is to keep that on the header - only. -* **First include is the matching header**, on its own line, separated by a - blank line from the rest of the includes. This forces the header to be - self-contained. -* **No declaration body in the header is duplicated here.** Default and - trivial inline definitions stay in the header. -* **File-local helpers** (free functions, `static` variables, internal - classes) go inside an anonymous namespace at the top of the `.cc`: - - ```cpp - namespace { - - int helper() { return 42; } - - } // namespace - ``` - -### Header-vs-source rule of thumb - -| Put in `.h` | Put in `.cc` | -| --- | --- | -| Class declaration, public/protected API, friend declarations | Out-of-line method definitions | -| Inline trivial accessors (one-liners returning a member) | Anything that would otherwise pull a heavy include into the `.h` | -| Templates and constexpr definitions | Free functions and statics that are implementation-only | -| Doxygen describing the contract | Comments explaining *why* a non-obvious implementation choice was made | -| Forward declarations of types only used by reference / pointer | `#include` of those types' headers | - -If you find yourself adding a heavyweight `#include` in a header just to make -something compile, consider whether a forward declaration in the header plus -a full include in the `.cc` would do. - ---- - -## Coding conventions - -### Language level - -C++17. Prefer modern idioms (`auto`, range-for, structured bindings, -`std::optional`, `if constexpr`) where they aid clarity. Do not introduce -C++20 features. - -### Formatting - -clang-format 19 with the project `.clang-format`. Do **not** hand-format; -run `./format-sources.sh` after editing. Notable settings the formatter -enforces: - -* 4-space indent, no tabs. -* Pointer/reference attaches to type: `int* p`, not `int *p`. -* Braces on the same line for functions, classes, and control flow - (`if (x) {`). -* `class Foo : public Bar {` on one line. -* Member initialiser lists wrap with the colon on a new line and one - initialiser per line when long. - -### Naming - -* Types: `UpperCamelCase` (`PluginManifest`, `LibraryManager`). -* Functions and variables: `lowerCamelCase` (`loadedPlugins`, `forLibrary`). -* Private and protected data members: trailing underscore (`name_`, - `handle_`). -* Macros and preprocessor constants: `SCREAMING_SNAKE_CASE` - (`REGISTER_LIBRARY`, `eckit_HAVE_ECKIT_MPI`). -* File names match the primary type they declare. - -### Errors - -Throw `eckit` exceptions from `eckit/exception/Exceptions.h` -(`BadValue`, `SeriousBug`, `AssertionFailed`, ...) rather than standard -ones. Use the `Here()` helper to attach source location where to helps -diagnostics and wherever reasonable. `ASSERT(...)` for invariants, -`EXPECT(...)` only inside test `CASE` bodies. - -### Threading - -Use `std::mutex` and related `std` types where appropriate. There is -a plethora of existing use of `eckit::Mutex` / `eckit::AutoLock<...>` / -`eckit::StaticMutex` in the code base - it is not necessary to use those -for new code and old code is being gradually migrated. -Document threading expectations in doxygen. - -### Includes - -* Use `"eckit//.h"` form (with the `eckit/` prefix) for any - include from this project, even within the same module. -* Sort within each group; clang-format will do this for you. -* Never include `` or other libstdc++ internals. - -### Feature gates - -Feature-gated code uses both a CMake `if(eckit_HAVE_)` to compile -in/out the relevant translation units, and a generated `eckit_HAVE_` -macro from `eckit_config.h` for runtime conditional logic. New features get -both halves wired up. - ---- - -## Tests - -* New tests live in `tests//test__.cc` and are - registered with `ecbuild_add_test(TARGET eckit_test__ ...)`. -* Tests use the harness in `eckit/testing/Test.h`. A typical test file: - - ```cpp - #include "eckit/testing/Test.h" - - using namespace eckit::testing; - - namespace eckit::test { - - //---------------------------------------------------------------------------------------------------------------------- - - CASE("descriptive sentence") { - EXPECT(condition); - EXPECT_EQUAL(actual, expected); - EXPECT_THROWS_AS(badCall(), BadValue); - } - - //---------------------------------------------------------------------------------------------------------------------- - - } // namespace eckit::test - - int main(int argc, char** argv) { - return run_tests(argc, argv); - } - ``` - -* When a test must observe state both before and after a global - initialisation (e.g. before/after `Main` construction), put plain - conditional aborts in `main()` around the construction — `EXPECT` only - works reliably inside `CASE`. -* Do not silently rely on environment defaults. Make every relevant env var - explicit with `SetEnv` so the test is hermetic. - ---- - -## Documentation - -* User-facing docs live under `docs/content/` as reStructuredText files - consumed by Sphinx with the Doxygen-generated API reference. Add new - pages to the appropriate `toctree` in `docs/index.rst` or a - parent page. -* Doxygen comments in headers are extracted into the API docs; keep them - current when changing signatures or contracts. -* The Sphinx build runs with `-W` (warnings as errors). A broken cross- - reference will fail CI. -* Docs CI uses Python 3.13, installs `docs/requirements.txt`, prepares - Doxygen 1.14.0, and then runs `cd docs && make -j html`. Reproduce - locally with the same Python version when chasing CI-only failures. - ---- - -## Pre-commit checklist - -Before opening a PR: - -1. `./format-sources.sh` on every changed C/C++ file. -2. `cmake --build build -j` clean. -3. `ctest -j --output-on-failure` from the build dir, or at minimum the - tests in the modules you touched. -4. If you added Sphinx content, `cd docs && make -j html` clean. -5. Headers you created carry the licence header *and* `/// @author` / - `/// @date` lines. -6. No `using namespace` in headers; no stray `#include`s; no commented-out - code. From be4fdec72d20bce604e5ca478820b5e482fdd2ad Mon Sep 17 00:00:00 2001 From: Simon Smart Date: Fri, 8 May 2026 22:40:38 +0200 Subject: [PATCH 3/4] ECKIT-680: Tweaks from PR --- docs/content/plugins.rst | 2 +- src/eckit/system/LibraryManager.cc | 2 +- src/eckit/utils/StringTools.h | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/content/plugins.rst b/docs/content/plugins.rst index c43e3a532..71720e8fe 100644 --- a/docs/content/plugins.rst +++ b/docs/content/plugins.rst @@ -117,7 +117,7 @@ There are three orthogonal ways a plugin can be loaded. Scoped plugins are *not* loaded by this path. ***Note: This mechanism is deprecated and will be removed in the future.*** - Plugins should be explictily loaded by their owning library via the scoped + Plugins should be explicitly loaded by their owning library via the scoped mechanism described below. **Scoped load (library-driven)** diff --git a/src/eckit/system/LibraryManager.cc b/src/eckit/system/LibraryManager.cc index 5e12fa1ac..5ece56a2e 100644 --- a/src/eckit/system/LibraryManager.cc +++ b/src/eckit/system/LibraryManager.cc @@ -99,7 +99,7 @@ static bool parseSemver(const std::string& v, int& major, int& minor, int& patch major = minor = patch = 0; std::vector parts; - Tokenizer(".")(v, parts); + Tokenizer(".", true)(v, parts); if (parts.size() < 2 || parts.size() > 3) { return false; } diff --git a/src/eckit/utils/StringTools.h b/src/eckit/utils/StringTools.h index 89c3a5006..d4fd1d998 100644 --- a/src/eckit/utils/StringTools.h +++ b/src/eckit/utils/StringTools.h @@ -13,6 +13,7 @@ #pragma once +#include #include #include #include From 833514e750caf75269e66adb769bc8cd0fc4878a Mon Sep 17 00:00:00 2001 From: Simon Smart Date: Fri, 8 May 2026 22:47:53 +0200 Subject: [PATCH 4/4] ECKIT-680: clang-format --- tests/system/test_system_plugin_main_autoload_explicit.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/system/test_system_plugin_main_autoload_explicit.cc b/tests/system/test_system_plugin_main_autoload_explicit.cc index 94b151c41..57ebb7c55 100644 --- a/tests/system/test_system_plugin_main_autoload_explicit.cc +++ b/tests/system/test_system_plugin_main_autoload_explicit.cc @@ -80,8 +80,7 @@ int main(int argc, char** argv) { // AUTO_LOAD_PLUGINS=false MUST be honoured: nothing else may be loaded apart from the // single fqname listed in LOAD_PLUGINS. Without this assertion, the test would silently // pass even if global autoload were broken (the explicit plugin would be loaded too). - test::expectLoadedPluginsEqual({"main-autoload-explicit-plugin"}, - "after Main ctor (autoload explicit list)"); + test::expectLoadedPluginsEqual({"main-autoload-explicit-plugin"}, "after Main ctor (autoload explicit list)"); return run_tests(argc, argv, false); }