Skip to content

fix(id): output into two files when structs present - #556

Open
jdacoello wants to merge 1 commit into
COVESA:masterfrom
jdacoello:fix/multiple-roots
Open

fix(id): output into two files when structs present#556
jdacoello wants to merge 1 commit into
COVESA:masterfrom
jdacoello:fix/multiple-roots

Conversation

@jdacoello

Copy link
Copy Markdown
Contributor

This PR addresses an issue introduced by #528

Problem

vspec export id writes static UIDs (staticUID) for every node in the tree into a single flat, FQN-keyed YAML dict, which is meant to be re-loaded as a normal vspec/overlay file downstream.

When --types was supplied, the struct/type tree's nodes were merged into that same dict as the main tree's nodes. Since both the main tree's root (e.g. Vehicle) and the types tree's root (e.g. Types) are separator-less top-level keys, the generated file ended up with two roots. Feeding that file back into any other vss-tools command (as --overlays) then failed with MultipleRootsException, since a single vspec file is only allowed to have one root.

Root cause

In id.py's cli(), both trees were exported into the same dict:

id_counter, _ = export_node(signals_yaml_dict, tree, id_counter, case_sensitive)
if datatype_tree:
    id_counter, _ = export_node(signals_yaml_dict, datatype_tree, id_counter, case_sensitive)

Other exporters (json, yaml, csv, binary) already had a --types-output option to write type/struct data to a separate file — id.py never wired it up, even though id.md already (incorrectly) documented it as if it existed.

Fix

  • Struct/type static UIDs are now written to a separate dict/file instead of being merged into the main output.
  • Added --types-output to vspec export id, consistent with other exporters.
  • --types-output is optional: if --types is used and --types-output is not given, the tool now derives a default name automatically: structs_<output-filename> written next to --output (e.g. --output out.vspecstructs_out.vspec).
  • Updated CHANGELOG.md and id.md (options table + new "Custom data types (structs)" section) to document the new behavior and the recommended chaining workflow (main output as -l/--overlays, types output as -t/--types — replacing, not supplementing, the original types file).
  • Updated/added tests in test_id_struct.py covering both explicit --types-output and the new default-naming fallback.

Usage change

# Before (broken): struct UIDs merged into the main output -> two roots, unusable downstream
vspec export id -s spec.vspec --types types.vspec -o ids_overlay.vspec

# After: struct UIDs written separately (explicit name)
vspec export id -s spec.vspec --types types.vspec \
  -o ids_overlay.vspec --types-output ids_types.vspec

# After: struct UIDs written separately (default name: structs_ids_overlay.vspec)
vspec export id -s spec.vspec --types types.vspec -o ids_overlay.vspec

# Chaining downstream:
vspec export <format> -s spec.vspec -l ids_overlay.vspec --types ids_types.vspec ...

Testing

  • test_id_struct.py: verifies struct/property nodes land in the types-output file (not the main one) both when --types-output is explicit and when it's omitted (default structs_ naming).
  • Full id/struct-related test suite passes (157 passed); 2 pre-existing, unrelated protobuf test failures confirmed to exist on main as well.

Signed-off-by: JD Alvarez <8550265+jdacoello@users.noreply.github.com>
Comment thread CHANGELOG.md
### Changed

- **Breaking**: Unit descriptions (`unit` field) must now be globally unique across all quantities. Previously accepted duplicate unit descriptions in `units.yaml` will now fail validation with a clear error message identifying the conflicting unit keys.
- **Breaking**: `vspec export id` now writes struct/type static UIDs to `--types-output` (or, if omitted, to a file named `structs_<output>` next to `--output`) instead of merging them into the main `--output` file. Previously, using `--types` produced a vspec with two top-level roots that other tools cannot load.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To be discussed at meeting - do we want to wait merging this one until we have released 6.1 (cannot be included in 6.1 due to breaking change, right)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants