Skip to content

Add "includes" to Nodl schema - #93

Closed
alistair-english wants to merge 18 commits into
mainfrom
alistair/includes_key
Closed

Add "includes" to Nodl schema#93
alistair-english wants to merge 18 commits into
mainfrom
alistair/includes_key

Conversation

@alistair-english

@alistair-english alistair-english commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The first step in code generation is the includes field - so that we can represent "externally defined" things like node base types or interfaces we want to inherit.

My thinking about this is documented in docs/design/nodl-composition.md.

Includes field

Added a new optional includes field to the NoDL schema, enabling interface composition:

includes:
  - ref: nodl://tf2_ros/tf_listener
  • load_nodl() recursively resolves relative paths and nodl:// package URIs (via ament index), merges interfaces, and returns a single resolved document
  • Merge semantics: identical duplicates are deduplicated, conflicts raise errors, local declarations win over includes
  • Diamond and circular include graphs handled via canonical-path dedup

Cmake macros (see docs/design/ament-nodl-macros.md)

Replace the single ament_nodl_register_node macro with three layered macros:

  • ament_nodl_install - validates and installs .nodl.yaml files, registers them in the nodl_interfaces ament index
  • ament_nodl_register_executable - maps an executable to a NoDL spec (nodl_executables index)
  • ament_nodl_register_component - maps a component class to a NoDL spec (nodl_components index)

Testing / package merge

To support testing nodl_schema:

  • Migrated integration tests to use pytest-colcon-ws with embedded sub-workspaces
  • Merged ament_nodl into nodl_schema
    • nodl_schema had a test-time dependency on ament_nodl - if we want to exercise includes properly, we need schemas to be installed
    • they are also very tightly coupled functionality - we cant have an "include" feature in the schema, without the installation mechanism to support the execution of that feature

Key files to look at:

  • docs/design/*
    • I know i should probably add user facing documentation as well, not fully across how we have that set up?
    • this is to document my thinking about these changes in more detail
  • https://github.com/alistair-english/pytest-colcon-ws
  • nodl_schema/test/*
    • in particular nodl_schema/tests/test_schema.py
    • trying to define the tests at the "interface level", so that they document the expected behaviour directly

@read-the-docs-community

read-the-docs-community Bot commented Jul 29, 2026

Copy link
Copy Markdown

Documentation build overview

📚 nodl | 🛠️ Build #33825704 | 📁 Comparing 8500d29 against latest (427369d)

  🔍 Preview build  

7 files changed · ± 3 modified · - 4 deleted

± Modified

- Deleted

@alistair-english
alistair-english force-pushed the alistair/includes_key branch 2 times, most recently from 5b11b7a to f1d06a2 Compare July 29, 2026 23:30
emersonknapp
emersonknapp reviewed Jul 30, 2026
View reviewed changes

@emersonknapp emersonknapp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this approximately equivalent to #92 ? I was making progress on it, if slowly 😅

Is this actually a prerequisite for code module generation? I don't really think so... You just generate the part that is specified. Includes are necessary for testing and doc generation, from our discussion I don't think generators have to know about includes, at least not at first.

@alistair-english

alistair-english commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

I came to the conclusion that the best way to model the base class was via an includes pattern rather than a separate explict "base" key. Either way we need some sort of key to signify "use this node as the base node".

I think its better to do it as an includes because for the purpose of "description" (everything other than codegen) - the node base isnt special, its just another thing that gets included in the node interface.

Then for code generation we can have a separate way to determine what "type" of include it is - i was thinking a sidecar file with generation specific things in it. This way we arent tying any of our schema to a specific code generation constraint, and different code generators can ask for different things from their sidecars.

This is somewhat described here: https://github.com/ros-tooling/nodl/pull/93/changes#diff-3cc7cfeabd380765b91f376f5dd549227d856362b62ec5743eb50f838368c251

@alistair-english
Rewrite ament_nodl: three-macro layered architecture
85f2c74 
Replace the single ament_nodl_register_node macro with three layered
macros per the ament-nodl-macros design doc:

- ament_nodl_install: validates and installs NoDL files, registers them
  in the nodl_interfaces ament index resource. Idempotent via a CMake
  global property guard.
- ament_nodl_register_executable: maps an executable name to a NoDL
  spec in the nodl_executables index.
- ament_nodl_register_component: maps a component class to a NoDL spec
  in the nodl_components index.

Both higher-level macros call ament_nodl_install internally and use
ament_index_register_resource for index registration.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Add embedded test workspace for nodl_schema integration tests
e8739d1 
- Add test_ws/ with minimal ament_cmake packages (nodl_fragment_a,
  nodl_fragment_b) that install .nodl.yaml files via ament_nodl_install()
- Add conftest.py with session-scoped fixture that builds the workspace
  once, sources the install, and strips outer overlays
- Add smoke tests verifying packages appear on AMENT_PREFIX_PATH and
  NoDL files are installed
- Fix ament_nodl_install.cmake: accumulate filenames and register the
  ament index resource once instead of per-file (avoids CMake duplicate
  output file error when multiple FILES are passed)
- Add test README documenting the setup

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
docs: add NoDL composition & code generation design
6fae622 
Covers the includes mechanism, schema change, API layering,
ament index registration (all three resource types), and
codegen sidecars. All open questions are resolved inline.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
nodl_schema: migrate test fixtures to pytest-colcon-ws
077621f 
Replace the hand-rolled ~50-line session fixture with two small fixtures
consumed by the pytest-colcon-ws plugin:

- test_ws_path: points the plugin at the embedded test workspace
- test_ws_underlays: sources the outer workspace so ament_nodl is available

Benefits:
- Clean-shell isolation (no inherited env leakage)
- Better build failure reporting via pytest.fail()
- Less code to maintain (~15 lines vs ~50)

The test_ws_env fixture contract (dict[str, str]) is unchanged, so
test_integration.py requires no modifications.

Requires pytest>=8 and pytest-colcon-ws (workspace-level pixi.toml).

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Migrate test_ament_nodl to sub-workspace tests under ament_nodl
f3d5987 
Replace the standalone test_ament_nodl package with pytest-colcon-ws
based integration tests inside ament_nodl/test/. A jig package
(test_ament_nodl) in an embedded sub-workspace exercises all three
CMake macros: ament_nodl_install, ament_nodl_register_executable, and
ament_nodl_register_component.

Changes:
- Add ament_nodl/test/ with conftest, four test files, and a test_ws
  containing the test_ament_nodl jig package
- Add ament_cmake_pytest, python3-pytest, and pytest-colcon-ws as
  test dependencies in ament_nodl/package.xml
- Add ament_add_pytest_test calls in ament_nodl/CMakeLists.txt
- Update nodl_schema/test/conftest.py to use environment-based
  underlay discovery via AMENT_PREFIX_PATH instead of a hardcoded
  relative path
- Add pytest-colcon-ws test_depend to nodl_schema/package.xml
- Delete the standalone test_ament_nodl package

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Fix ament_nodl_install to support multiple calls per package
2342699 
ament_nodl_install claimed to be idempotent but calling it more than
once per package (e.g. directly and via ament_nodl_register_executable)
caused CMake to fail with 'Files to be generated by multiple different
commands' because each invocation independently called
ament_index_register_resource(nodl_interfaces ...).

Root cause: the ament_register_extension() call was inside
ament_nodl_install(), a function(). Since ament_register_extension is
a macro() that does list(APPEND ...), the append targeted a local
variable copy that was discarded when the function returned. The
ament_package() hook was never actually registered, and each
ament_nodl_install call fell through to its own
ament_index_register_resource.

Fix: defer the nodl_interfaces registration to ament_package() time
via a hook registered in ament_nodl-extras.cmake.in (which runs at
find_package scope). ament_nodl_install now only accumulates filenames
into a global property; the new ament_nodl_package_hook.cmake collects
them into a single ament_index_register_resource call.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Remove test_ament_nodl from CI package list
9a9a863 
Package was deleted as a top-level package and now only exists as a
nested test fixture at ament_nodl/test/test_ws/src/test_ament_nodl/,
causing all CI jobs to fail with 'Package test_ament_nodl specified
with --packages-up-to was not found'.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
update gh workflow to install pytest-colcon-ws
709605f 
I know its a bit of a hack, eventually ill get the package released

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
fix CI: pin pytest<9, work around Debian pluggy, add test_depend
3654a5f 
- Install pluggy with --ignore-installed to bypass the Debian-managed
  pluggy 1.4 on noble (jazzy/kilted) which pip cannot uninstall.
- Pin pytest>=8,<9 for launch_testing compatibility on Humble, Jazzy,
  and Kilted (pytest 9 removed the deprecated 'path' arg used by
  launch_testing's pytest_pycollect_makemodule hook).
- Add <test_depend>ament_nodl</test_depend> to nodl_schema so colcon
  sources ament_nodl's install prefix during tests, making it available
  on AMENT_PREFIX_PATH for the integration test conftest.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Merge ament_nodl into nodl_schema
d9a542b 
Eliminate the circular dependency between ament_nodl (exec_depend on
nodl_schema) and nodl_schema (test_depend on ament_nodl) by merging
them into a single ament_cmake_python package.

ament_nodl contained no Python code — just four CMake files whose
sole runtime dependency was nodl_schema. They are two halves of the
same thing, split prematurely.

The merged nodl_schema package:
- Uses ament_cmake_python to install both the Python package and
  the CMake macros
- Keeps all ament_nodl_*() macro names unchanged
- Ships nodl_schema-extras.cmake.in so find_package(nodl_schema)
  brings the macros into scope
- Consolidates both test suites and test workspaces

Downstream migration:
- find_package(ament_nodl) -> find_package(nodl_schema)
- <buildtool_depend>ament_nodl -> <buildtool_depend>nodl_schema
- Macro calls (ament_nodl_install, etc.) are unchanged
- Python API and CLI are unchanged

See docs/design/merge-nodl-schema-ament-nodl.md for rationale.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
ci: upgrade pip before installing pytest-colcon-ws
d34e691 
Jammy's pip 22 has a build-isolation bug that causes pyproject.toml
[project] metadata to be ignored, producing an empty UNKNOWN-0.0.0
wheel. Upgrading pip first avoids this. The upgrade is allowed to
fail (|| true) because Noble's Debian-installed pip has no RECORD
file.

Also consolidate the pip install into a single --ignore-installed
line so both pluggy and pytest (which also lack RECORD files on
Noble) are handled together.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
feat(nodl_schema): schema accepts includes field
7aba8ee 
Add 'includes' property to nodl.schema.yaml: a list of objects each
with a required 'ref' string constrained to either a nodl:// package
URI or a relative file path (./ or ../) ending in .nodl.yaml.

Add IncludeRef model and includes field to NodlDocument in models.py.

Phase 1 of the composition plan — structural validation only, no
resolution or merging.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Phase 2: load_nodl() resolves relative includes
c931fef 
- load_nodl() now takes a Path, reads the file, validates against the
  schema, recursively resolves relative includes, merges interfaces,
  and returns a NodlDocument with includes stripped.
- Merge semantics: local declarations win over includes, identical
  duplicates across includes are silently deduplicated, conflicting
  duplicates raise ValueError.
- Diamond and circular include graphs handled via canonical-path dedup.
- CLI entry points (python -m nodl_schema, ros2 nodl validate) use
  validate() consistently for schema-only checking.
- Removed str/bytes/IO overloads from load_nodl() — Path is the only
  supported input type.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
feat(nodl_schema): resolve nodl:// package URIs via ament index
c369d73 
Add _resolve_package_uri() to resolve nodl://package/name refs to
<prefix>/share/<package>/nodl/<name>.nodl.yaml using
ament_index_python.get_package_prefix().

Enrich test workspace fragment NoDL files with actual interface
content (publishers, parameters) so integration tests can assert
on merged results.

Add 4 integration tests exercising single-package resolution,
cross-package merging, and error paths for missing packages/files.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
test(nodl_schema): add CLI validation tests for includes (Phase 4)
aefa37d 
Confirm the end-to-end path: python -m nodl_schema accepts well-formed
includes entries and rejects malformed ones (bare strings instead of
{ref: ...} objects). No new implementation needed — the CLI already
calls validate() which uses the schema updated in Phase 1.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
fix: parameter merge across includes silently swallowed conflicts
724ab73 
_merge_parameters() was reused for the include-accumulation step in
_load_and_resolve(), passing the accumulator as the 'local' argument.
The 'local wins' branch silently skipped conflicting parameters from
subsequent includes instead of raising an error.

Add _accumulate_parameters() — a strict merge helper with no local-wins
semantics — and use it for the accumulation step. Identical duplicates
are still deduplicated; differing definitions now correctly raise
ValueError.

Add 16 tests covering merge-if-identical and merge-conflict for all
section types (subscriptions, service_servers, service_clients,
action_servers, action_clients, parameters), QoS-difference conflicts,
and local-wins for parameters.

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
clean up comment styles
0c839c6 
Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
Remove relative path includes, only allow nodl:// package URIs
8d7567e 
Relative file includes (./  ../) break when an installed nodl://
reference contains its own relative includes — the paths resolve
against the installed location, not the original source tree.

Remove relative path support entirely. Packages that need to
reference their own schemas can self-reference via nodl://.

Changes:
- Schema regex now only accepts nodl://package/name
- Collapse _resolve_package_uri into _resolve_ref (single code path)
- load_nodl() accepts Path | dict since source path is no longer
  needed for include resolution
- Drop base_dir threading and source path seeding of seen set
- Move merge-semantics tests from unit tests to integration tests
  using installed test workspace fragments
- Add 21 test fragment files across nodl_fragment_a and nodl_fragment_b
  to cover merge, dedup, conflict, diamond, nested, and circular cases

Signed-off-by: Alistair English <hello@alistairenglish.com>
@alistair-english
alistair-english deleted the alistair/includes_key branch August 11, 2026 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emersonknapp emersonknapp emersonknapp left review comments
@lsy3 lsy3 Awaiting requested review from lsy3

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alistair-english @emersonknapp