Extend validator to REP-149 format-3 conformance#21
Merged
Conversation
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Adds 'conflict' and 'replace' to ELEMENTS and NEW_LINE_BEFORE so the validator no longer rejects these standard format-3 anti-dependency tags. They flow through the schema-driven plumbing automatically: unknown-tag rejection, ordering, and occurrence checks all derive from ELEMENTS. retrieve_all_dependencies continues to skip them (filter is 'depend in tag'), so they are never sent to rosdep — correct, since they are anti-dependencies, not installable deps.
Validates the structural attributes REP-149 requires: <package format='3'> presence and value, <name> regex ^[a-z][a-z0-9_]*$, <version> regex ^\d+\.\d+\.\d+$, and non-empty email='…' on every <maintainer>. Read-only — none of these can be safely auto-filled. Skipped under --missing-deps-only and --ignore-formatting-errors. Slots before FormatterValidationStep so manifest-level errors surface before the formatter touches anything.
Threads condition evaluation through the rosdep, CMake-comparison, buildtool, and build_type steps so deps whose condition evaluates to False against os.environ are skipped — matching what colcon (via catkin_pkg.condition.evaluate_condition) does at build time. Adds catkin-pkg as a dependency; we delegate to the canonical evaluator rather than rolling our own parser. Adds --ignore-conditions CLI flag (default off) for users whose validation environment differs from build time. Tests cover the wrapper (None/empty/match/compound/malformed), the rosdep filtering (active/inactive/--ignore-conditions), the build_type filtering, and a deepcopy regression.
Adds DependencyExclusivityStep: a key declared in <depend> may not also appear in <build_depend>, <build_export_depend>, or <exec_depend> (REP-149). Reports the overlap in check-only mode; auto-fix mode collapses to canonical <depend> form but is suppressed when the granular tag carries a condition the <depend> does not — that's narrower, not redundant, so dropping it would change semantics.
The ROS 2 rolling 'Creating a package' tutorial generates an ament_python package.xml without <buildtool_depend>ament_python</…> — only <export><build_type>ament_python</build_type></export>. The package builds fine that way, so failing validation when the tag is missing or set to ament_cmake is stricter than the official template. Plain Python packages (PYTHON_PKG, not msg_pkg) now surface the finding via result.warnings without flipping result.valid. CMake packages and message packages keep strict hard-error behaviour (they genuinely won't build without ament_cmake/rosidl_default_generators). The --auto-fill-missing-deps path still inserts the tag on request.
… build_type, root-tag check, rosidl_default_runtime Five remaining REP-149 gaps: - LaunchDependencyStep now reads exec_deps/test_deps via the *_with_conditions helpers and filters inactive entries; an inactive manifest dep no longer satisfies a real launch reference. - BuildTypeExportStep applies REP-149's 'last wins when multiple are active' rule and emits a warning when >1 active <build_type> is detected. - ManifestSchemaStep rejects non-<package> root elements. - MemberOfGroupStep treats inactive <member_of_group> entries as absent. - New RosidlInterfaceRuntimeStep requires <exec_depend>rosidl_default_runtime</…> (or unified <depend>) for interface packages, with auto-fill and condition-awareness. Drops exec_deps/test_deps from LaunchDependencyStep's constructor; they are now computed at perform_check time alongside their conditions. Adds tests/test_rep149_followup.py (17 cases).
Sonar S5443 flags every hardcoded /tmp/... literal as a publicly- writable-directory risk, even when the path is never actually opened (our validation steps use xml_file only for log-message location). Replaces all 28 occurrences across five test files with per-test TemporaryDirectory() instances registered via addCleanup,
ColoredFormatter previously indented only ERROR-level records, so
warnings ('Auto-filling …', 'Corrected dependency order …') and info
notes ('Check element order corrected …') sat flush-left while errors
('Element order is incorrect') were tab-indented under the 'Processing
pkg_name...' header. Same message class, different alignment.
Default to indenting every log record. The 'Processing X...' header
itself, the four final summary lines, and two startup warnings opt
out via extra={'flush_left': True}.
README's Sample Output regenerated to match.
Joschi3
force-pushed
the
feature/extension
branch
from
76506ce to
098e63d
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<conflict>/<replace>— valid REP-149 anti-deps were rejected by the unknown-tag check.ManifestSchemaStep— validates root is<package>,format="3",<name>regex,<version>regex, non-empty<maintainer email>. Report-only.condition="…"awareness — threaded through rosdep, CMake, buildtool, build_type, member_of_group, launch-dep steps. Inactive entries are skipped (matches colcon). Delegates parsing tocatkin_pkg. New--ignore-conditionsflag.<depend>exclusivity — REP-149 forbids<depend>overlapping with<build_depend>/<build_export_depend>/<exec_depend>. Reports overlaps; auto-fill collapses redundant tags. Refuses to collapse when conditions differ.ament_pythonbuildtool rule — rolling tutorial omits the tag, so absence is now a warning, not an error. CMake/msg packages keep strict behaviour.<build_type>— REP-149 says the last active entry wins; we took the first. Also warns when >1 active.RosidlInterfaceRuntimeStep— requires<exec_depend>rosidl_default_runtime</…>for msg/srv/action packages. Auto-fillable.