Skip to content

feat: Add 'ament_yamllint' package for YAML style validation#567

Open
frozenreboot wants to merge 6 commits into
ament:rollingfrom
frozenreboot:feat/add-ament-yamllint
Open

feat: Add 'ament_yamllint' package for YAML style validation#567
frozenreboot wants to merge 6 commits into
ament:rollingfrom
frozenreboot:feat/add-ament-yamllint

Conversation

@frozenreboot

Copy link
Copy Markdown

Description

This PR adds ament_yamllint to support YAML style validation.
It revive original PR

Origianl proposal PR

#351.

Changes from original proposal

  • Applied all previous review comments
  • Fixed CMake export: Added CONFIG_EXTRAS to ament_package() in ament_cmake_yamllint. Without this, the ament_yamllint() macro was not visible to downstream packages.

test

  1. Ran colcon test --packages-select ament_cmake_yamllint ament_yamllint (passed)
  2. Integration Test: Created a local dummy package to verify behavior:
    Valid YAML files -> Pass
    Invalid/Broken YAML files -> Fail (correctly reported syntax errors and lint issues)
    Directories with COLCON_IGNORE -> Skipped

Relate issue
#412.
ros-navigation/navigation2#5853, where the lack of a standard YAML linter in ROS 2 was highlighted.

@ahcorde ahcorde requested a review from cottsay January 12, 2026 09:16
@ahcorde

ahcorde commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

@cottsay do you mind to take a look ?

Comment thread ament_cmake_yamllint/cmake/ament_yamllint.cmake Outdated
Comment thread ament_cmake_yamllint/doc/index.rst Outdated
Comment thread ament_cmake_yamllint/doc/index.rst
Comment thread ament_cmake_yamllint/CMakeLists.txt Outdated
Comment thread ament_cmake_yamllint/package.xml Outdated
Comment thread ament_yamllint/setup.py Outdated
Comment thread ament_yamllint/setup.py Outdated
Comment thread ament_yamllint/setup.py Outdated
Comment thread ament_yamllint/setup.py Outdated
Comment thread ament_yamllint/setup.py Outdated
@frozenreboot frozenreboot force-pushed the feat/add-ament-yamllint branch from 88d9d29 to 8fd6c05 Compare January 20, 2026 13:31
@frozenreboot

Copy link
Copy Markdown
Author

@ahcorde Thanks for the detailed review!

I have applied all the requested changes:

  • Bumped version to 0.20.3 (matching Rolling)
  • Updated Copyright year to 2026
  • Added author and maintainer information correctly
  • Fixed setup.py metadata and dependencies as suggested

The CI checks passed successfully. It is ready for re-review.

@cottsay cottsay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments.

For the record, I pretty much just copied the original PR from another ament_lint tool and modified it for use with yamllint. I never pushed the PR forward because there wasn't really any outside interest in the tool at the time and most of Chris' feedback was applicable to the other ament_lint tool I had copied the code from.

Happy to help push this forward now that somebody's interested, though.

Comment thread ament_yamllint/setup.py Outdated
Comment thread ament_yamllint/CHANGELOG.rst
Comment thread ament_cmake_yamllint/CHANGELOG.rst
Comment thread ament_yamllint/ament_yamllint/main.py Outdated
@frozenreboot

Copy link
Copy Markdown
Author

I have addressed all the review comments. Ready for re-review.

@frozenreboot frozenreboot requested a review from cottsay January 22, 2026 14:30

@cottsay cottsay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple more changes, but this is looking good. I'll trigger some CI builds on the next round of changes.

The switch for Rolling to RHEL 10 is imminent, and I'm still working on yamllint support there, so we may have to hold off a bit until that work lands.

Thanks!

# @public
#
function(ament_yamllint)
cmake_parse_arguments(ARG "" "MAX_LINE_LENGTH;TESTNAME" "" ${ARGN})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this is a copy/paste error from another extension. The argument doesn't appear to be used and isn't documented.

Suggested change
cmake_parse_arguments(ARG "" "MAX_LINE_LENGTH;TESTNAME" "" ${ARGN})
cmake_parse_arguments(ARG "" "TESTNAME" "" ${ARGN})

set_tests_properties(
"${ARG_TESTNAME}"
PROPERTIES
LABELS "yamllint;linter"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's order these alphabetically.

Suggested change
LABELS "yamllint;linter"
LABELS "linter;yamllint"

@frozenreboot

Copy link
Copy Markdown
Author

Thanks for the review! I've addressed the feedback in the latest commit:

  • Removed the unused MAX_LINE_LENGTH argument.
  • Reordered test labels to be alphabetical (linter;yamllint).

I also noted your comment about the RHEL 10 transition. I'm ready to wait until the infrastructure work lands, so please take your time triggering the CI.

@frozenreboot frozenreboot requested a review from cottsay February 7, 2026 09:34
@cottsay

cottsay commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

Cross-link to ros2/ci#847

I'll trigger CI once that change merges.

@cottsay

cottsay commented Feb 17, 2026

Copy link
Copy Markdown
Contributor
  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde

ahcorde commented Feb 18, 2026

Copy link
Copy Markdown
Contributor

I can see some tests failing on windows:

test\test_yamllint.py:22: in test_yamllint
    rc = main(argv=[])
ament_yamllint\main.py:104: in main
    report = invoke_yamllint(files, config_file=temp_config.name)
ament_yamllint\main.py:244: in invoke_yamllint
    raise subprocess.CalledProcessError(
E   subprocess.CalledProcessError: Command '['C:\\pixi_ws\\.pixi\\envs\\default\\Scripts\\yamllint.EXE', '-s', '-f', 'parsable', '-c', 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\yamllint__s5pfsoe.yaml', 'ament_yamllint\\configuration\\yamllint.yaml']' returned non-zero exit status 1.

@frozenreboot

Copy link
Copy Markdown
Author

@ahcorde

Thanks for your review.

I noticed the Windows build failure.

The error from yamllint (returned non-zero exit status 1) suggests it might be an issue with the configuration or line endings (CRLF) on the Windows runner.

Would you be open to me adding a .gitattributes file to enforce LF for YAML files to see if that resolves it? Or is there a preferred way to handle this in this repository?

@cottsay cottsay self-assigned this Feb 19, 2026
@frozenreboot frozenreboot force-pushed the feat/add-ament-yamllint branch 2 times, most recently from 16f6415 to 0ebd40d Compare March 21, 2026 11:22
@frozenreboot

Copy link
Copy Markdown
Author

@cottsay

I have updated the PR to fix the Windows CI failure.

The root cause was a PermissionError specific to Windows'file locking mechanism. Previously, the NamedTemporaryFile was kept open within a with block while subprocess.run tried to read it using yamllint.exe. Windows denies access to the subprocess in this state.

For context, here is the exact trace from the Windows runner:

Click to expand error log
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\site-packages\yamllint\cli.py", line 197, in run
    conf = YamlLintConfig(file=args.config_file)
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\site-packages\yamllint\config.py", line 41, in __init__
    with open(file, mode='rb') as f:
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\yamllint_fp73nw0q.yaml'

I changed the logic to explicitly close() the temporary file before invoking the subprocess, and then manually clean it up using os.remove() in a finally block. I also verified this fix on my fork using a Windows GitHub Actions runner, and it passes perfectly.

Could you please re-trigger the CI?

@frozenreboot

Copy link
Copy Markdown
Author

Hi @cottsay,
Just a gentle ping on this PR.
The Windows PermissionError issue has been resolved in my latest commit.
Could you please re-trigger the CI when you have a chance? Thank you!

@frozenreboot frozenreboot requested a review from ahcorde May 23, 2026 07:25
frozenreboot and others added 6 commits June 14, 2026 14:42
This introduces ament_yamllint and ament_cmake_yamllint packages to support YAML linting in ROS 2.

This work revives and finalizes the original proposal in PR ament#351.

Co-authored-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: wj <frozenreboot@gmail.com>
…2026)

Signed-off-by: wj <frozenreboot@gmail.com>
- Update license to SPDX identifier in setup.py
- Add headers to CHANGELOG.rst files
- Remove COLCON_IGNORE from IGNORE_MARKERS in main.py

Signed-off-by: wj <frozenreboot@gmail.com>
Signed-off-by: wj <frozenreboot@gmail.com>
…mllint execution

Signed-off-by: wj <frozenreboot@gmail.com>
Signed-off-by: wj <frozenreboot@gmail.com>
@frozenreboot frozenreboot force-pushed the feat/add-ament-yamllint branch from 0ebd40d to 13931bd Compare June 14, 2026 06:02
@frozenreboot

Copy link
Copy Markdown
Author

Hi @cottsay, following up on this PR — a quick status summary:

  • The Windows CI failure has been root-caused and fixed in an earlier commit: it was a PermissionError from Windows file locking on NamedTemporaryFile while the subprocess read it. The file is now explicitly closed before invoking yamllint and cleaned up in a finally block (verified passing on a Windows GitHub Actions runner on my fork).
  • I've rebased onto the latest rolling (no conflicts) and bumped the version to 0.21.1 to match. colcon test passes locally for both packages.
  • All review comments from you and @ahcorde have been addressed.

Could you re-trigger the CI builds when you have a chance? Happy to make any further changes if needed. Thanks!

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