Skip to content

fix: enhance version input handling#422

Closed
shenxianpeng wants to merge 1 commit intofix-364from
review-fix-364
Closed

fix: enhance version input handling#422
shenxianpeng wants to merge 1 commit intofix-364from
review-fix-364

Conversation

@shenxianpeng
Copy link
Copy Markdown
Member

The following review suggestion comes from Codex


• The new installer flow introduces at least two functional regressions: it rejects valid
semantic version inputs and it can fail after a successful package-manager install
because fallback installers are invoked unconditionally. Those cases make the composite
action less reliable than the base branch.

Full review comments:

  • [P2] Preserve semantic version strings for tool installation — /Users/sxp/Repos/cpp-
    linter/cpp-linter-action/action.yml:384-390
    This now hard-fails any version like 20.1 or 20.1.7 unless it happens to be an existing
    path. That is a regression from the previous clang-tools -i ${{ inputs.version }}
    behavior: both the clang-tools and clang-tools-wheel CLIs accept semantic version
    specifications, and cpp-linter itself resolves dotted versions when selecting clang-
    format/clang-tidy. Workflows that pin an exact wheel/build version will now stop here
    with Invalid version input instead of installing the requested tool.
  • [P2] Stop after the first successful tool source — /Users/sxp/Repos/cpp-linter/cpp-
    linter-action/action.yml:412-427
    On Linux and macOS this loop runs even when the earlier apt/Homebrew step has already
    installed the requested clang tool. As a result, a later PyPI/static install failure
    will now fail the action even though clang-format-$version/clang-tidy-$version is
    already present, which breaks the documented “try sources in order” fallback behavior.
    The secondary installers should only run when the prior source did not yield a usable
    tool.

@shenxianpeng shenxianpeng requested a review from a team as a code owner April 9, 2026 16:23
@shenxianpeng shenxianpeng requested review from 2bndy5 and removed request for a team April 9, 2026 16:23
@github-actions github-actions bot added the bug Something isn't working label Apr 9, 2026
Copy link
Copy Markdown
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

The description here is typical AI slop. The unnecessary wordiness... The reasoning shown doesn't demonstrate an understanding of the code it is modifying.

Some of the changes here make a little sense. But, they are lazy changes that hardly achieve the stated goals.

print $"(ansi yellow)Using custom clang tools installation at path: ($version_str)(ansi reset)"
exit 0
try {
$version_str | split row '.' | first | into int
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.

This is ok, but it isn't complete semantic version support.

Because it only checks the major version is an int, users could pass in version: 12.x, and none of our install tools support this format.

@2bndy5 2bndy5 changed the title fix: enhance version input handling in action.yml to support semantic ersion strings fix: enhance version input handling Apr 10, 2026
@shenxianpeng
Copy link
Copy Markdown
Member Author

thanks for taking time to review 😃 so let's just close it.

@shenxianpeng shenxianpeng deleted the review-fix-364 branch April 10, 2026 11:27
@2bndy5
Copy link
Copy Markdown
Collaborator

2bndy5 commented Apr 10, 2026

There's nothing really wrong with #368. I've tested it in the test repo and verified it should be backward compatible. Maybe you want to play around with test-wheels branch on the test repo? Otherwise, I'm confident enough and ready to move #368 forward.

FYI, cpp-linter/cpp-linter-rs#279 aims to remove all this condition logic (and version parsing/handling) from action.yml. Instead, it would be all done in rust; no pip/Python or nushell really needed. At least that's the goal.

@shenxianpeng
Copy link
Copy Markdown
Member Author

Thank you for your testing. let's move forward. And looks forward to have cpp-linter/cpp-linter-rs#279 to remove all the condition logic in the future.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants