Repository navigation
Add check-launch-tree, and scope launch patterns to their format - #24
Merged
Merged
Conversation
A pattern is a piece of syntax, and the same characters mean different
things in different languages. is a mapping key in YAML and a type
annotation in Python, so searching Python source for YAML keys reports
things that are not references and were never meant to be
The new code explained itself at about twice the length of the code, and several points were made three or four times over - "we did not look behind this" appeared in _edges_in, Walk.unresolved, scan_from and three test docstrings. Kept once, on Walk.unresolved. Dropped the workspace-specific narrative (hector_launch_manager's component count, athena's argument names) from a general-purpose library, the RST table comparing this entry point to the other one, and the docstrings that argued with an imagined critic rather than describing behaviour. Restored two short comments this branch had deleted - the ANY_FORMAT note and the reason TOML is only scanned under launch/ - and the trailing whitespace the second deletion left behind. _find_in_share no longer claims its basename fallback "can only widen the set of packages found, never invent one". It can: resolving to a same-named file elsewhere in the package reports what that file names. Fixing the behaviour is the next commit but one; the claim goes now.
The hook runs with pass_filenames: false and no args, so main() got ".", the repository root. check() joins its seed directories straight onto that, so a repository laid out as src/<pkg>/launch/ produced "read 0 launch file(s)" and exited 0 - the hook was a no-op everywhere except a single-package repository whose root is the package. main() now expands its arguments with find_package_xml_files, which the manifest validator already uses, so both entry points agree on what a package is and both skip COLCON_IGNORE and build-output trees. check() keeps taking one package directory and says so. The test fixture now writes a package.xml alongside the launch file, since that is what makes a directory a package.
Three ways the walk could be wrong about what it had read. A component's package and launch_file are now taken from the same YAML mapping rather than from a six-line window after a `package:` line. Given a node-only component followed by a launch one, the regex paired the first's package with the second's launch_file: a finding against a package that ships nothing of the sort, and the real include never followed. PyYAML is already a dependency and compose() keeps the line numbers. Calls are matched on their trailing name, so launch.actions.Include- LaunchDescription and ament_index_python.get_package_share_directory are seen. Matching ast.Name alone produced no edge at all for the qualified form - not even one flagged as unreadable - so a subtree could go unread with nothing saying so. _find_in_share only searches for a bare basename when a component asked for one. Applied to a written-out path it resolved a renamed or uninstalled launch file to a same-named file elsewhere in the package and went on to report what that file named, which both hid a real finding and invented packages the tree never loads.
…t is check() walked launch/ and each launch_manager_* directory separately, so anything reachable from more than one of them was re-read: a finding inside a shared launch file was printed once per directory that led to it, and counted that many times towards --error. scan_directories() takes the whole seed list and shares one visited set. An include whose package name still holds an unexpanded substitution was classified as a dead include and reported as "$(var robot)_sim does not ship launch/x.launch.yaml", which reads as a fault in a package of that name. It names no package at all, so it now goes to the unfollowable list where the rest of the unexpandable includes are. That list also says how many it did not print, rather than showing five and stopping. _report was executed by the tests but never asserted; its three shapes - a dead include, a reference from outside the package, a truncated list - now are, as is --arg reaching the walk through the CLI rather than only through check().
Ten lines: the hook in the quick-start snippet, a Features subsection, and a row in the architecture table. The workspace requirement is the part worth finding from outside the source - a check that skips silently in the wrong environment is otherwise indistinguishable from one that passed.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Several comments added with the launch-tree walk read as changelog entries: _component_edges described the regex that used to pair a node-only component's package with the next component's launch_file, _called_name described what matching only ast.Name did, and the pkg: pattern noted the package named `str` the unanchored version produced. In each case the rule and its reason survive; the account of the bug does not. The same for the test docstrings that reconstructed old behaviour in the past tense. COMPILED, TOML_REGEX_EXPR and TOML_COMPILED were kept behind a note explaining that they no longer decide anything. Nothing in the repository imports them, so the note and the names go together. REGEX_EXPR stays; scan_file's verbose logging indexes into it to name the pattern a match came from.
TestPatternsAreScopedToTheirFormat passed with the scoping removed entirely:
making _patterns_for return every pattern for every format left all six tests
green. Both false positives the fixture reproduced - `def resolve(pkg: str)`
and `{"launch_file": "pkg:robot..."}` - sit mid-line, so `(?m)^` rejects them
on its own and the formats never come into it.
The fixture now writes each form where only the format can rule it out: a
module-level `pkg: str = "hello"` and a `package = "sneaky_toml_form"`
assignment, both at column zero. Scanned with the scoping they yield nothing;
without it they yield `str` and `sneaky_toml_form`. The same mutation now fails
four tests.
test_the_toml_form_stays_out_of_yaml_and_python looped over the TOML-only
patterns and scanned the same file each time, never touching the loop variable.
It is replaced by one that checks every format-specific pattern against every
format, so what a pattern declares it is syntax for is what _patterns_for hands
it.
The hook printed nothing. pre-commit shows a hook's output only when it fails, declares verbose, or changes a file, so warning by default and exiting zero meant every finding was discarded: a scratch repo naming an uninstalled package reported "Passed" and no text at all. verbose: true on the hook entry is what makes the warnings reach anyone. --error also attached failure to the wrong thing. "Not installed" is what a CI runner sees for every package it has not built, and no commit can fix it. A reference the owning package.xml does not declare is a defect in the checkout, needs no workspace to see, and is exactly what should stop a commit. So the exit code now follows the finding: undeclared reference fatal dead include fatal not installed warning could not be followed warning Both fatal kinds only fail the run when the package owning the offending file is fixable: one in the checkout, or below a --fatal-under prefix, which is how CI marks the install space it populates itself. Against a binary-only install they drop to warnings - committing to athena_launch would otherwise fail on hector_detection_aggregator's manifest, which has no source in the workspace. Includes resolve through find-pkg-share, so most files the walk reads are installed copies. Where the same package is in the checkout, its manifest is the one graded; otherwise the check would grade the checkout against whatever was last built. A package in the checkout is never "not installed" either, only unbuilt. An empty AMENT_PREFIX_PATH no longer skips the run. Crossing package boundaries needs it, but the manifest check never did, and that is the case this has to work in. It walks the checkout's own launch files, suppresses the absence warnings that would all be noise, and says the tree was only partly followed. Also: --error gives way to --warn-only for introducing the hook to a repository that already has findings, --ignore matches the validator's global ignore list, a package with nothing to report prints nothing now that verbose shows every line, `components` joins the seed directories that LaunchDependencyStep already scans, and a component's dead include reports the filename the author wrote rather than one with `launch/` prepended. Over hector/src this is 74 errors across 15 of ~150 packages, all genuine missing exec_depends, mostly in launch_manager_components/ which format-package-xml does not scan - plus one real dead include, a component naming a launch file athena_driver_launch does not ship.
A finding was fatal only against a package in the checkout being scanned or below a --fatal-under prefix. ROS workspaces keep one repository per package, so under pre-commit "the checkout" is a single package and every sibling was someone else's problem - which is not what "packages in our src workspace" is supposed to mean. A defect one include away is normally in a sibling repository, so it was reported against the installed copy and downgraded to a warning. There is now a third tier between the two: the `src` of the surrounding workspace, found by walking up from the given paths with find_workspace_root. The tiers are indexed least editable first - a --fatal-under path, then the workspace, then the checkout - so where a package exists in several the most editable copy wins, and that is both the manifest a finding names and the one it is graded on. --fatal-under pointed at a source tree used to do nothing at all. It only prefix-matched the manifest a finding had already landed on, and a source tree outside the scanned paths never became the owner in the first place, so the match could not happen. Feeding those paths through the same index fixes it; install prefixes behave as before. workspace_sources resolves before calling find_workspace_root, which compares the path it is handed against an absolute <ws>/src. The hook passes ".", so without resolving it the workspace tier was silently absent in exactly the invocation that matters. Costs one rglob of the workspace, 1.3s cold on hector/src, and adds nothing to the failure count there: all 15 packages that fail still fail on their own manifest, and none newly fails because of a sibling.
|
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
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
Pattern scoping.
pkg:is a mapping key in YAML and a type annotation in Python, so scanning Python source for YAML keys reportedpkg: stras a package namedstr. Each pattern now declares the formats it applies to.New hook:
check-launch-tree. Follows every include out of a package's launch files, across package boundaries, and reports what does not resolve, e.g. a package that is not installed, or an include whose package is present but whose launch file is not.