Skip to content

Conversation

@shenxianpeng
Copy link
Collaborator

@shenxianpeng shenxianpeng commented Nov 3, 2025

closes #364

Summary by CodeRabbit

  • Documentation

    • Expanded installation guidance with ordered, platform-specific fallbacks; added direct wheel URLs and removed the old static-binaries link; updated license wording to reference the MIT License.
  • Chores

    • Installer now uses build-platform detection, robustly validates versions/paths, emits warnings/errors for invalid or empty inputs, and performs per-tool, version-aware sequential installs with clearer logging and outputs.

@shenxianpeng shenxianpeng requested a review from a team as a code owner November 3, 2025 20:44
@shenxianpeng shenxianpeng requested review from 2bndy5 and removed request for a team November 3, 2025 20:44
@shenxianpeng shenxianpeng added the enhancement New feature or request label Nov 3, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

Walkthrough

Updated README to prefer clang wheels with ordered fallback sequences per OS and adjusted links/licensing wording. Reworked action.yml to validate/coerce version, build a per-tool install list from inputs, and perform tool-by-tool installation choosing between wheel and legacy installers.

Changes

Cohort / File(s) Change Summary
Documentation
README.md
Replaced references to clang-tools-static-binaries with explicit wheel URLs for clang-tidy and clang-format; removed static-binaries link; introduced ordered, OS-specific fallback install sequences (Linux, macOS, Windows); updated license wording to reference the MIT License; minor wording adjustments.
Action manifest / install logic
action.yml
Replaced Windows detection with build_os-based check; added robust version handling (empty -> platform-default exit; attempt integer coercion; handle invalid string vs valid path with distinct outcomes); construct dynamic per-tool list from inputs.tidy-checks and inputs.style; replace single installer invocation with per-tool loop selecting clang-tools or clang-tools-wheel based on tool/version rules and invoking installs per-tool with appropriate logging and exit paths.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review action.yml version coercion/error paths and how path-valued version is treated.
  • Verify per-tool install selection logic (when to use wheels vs legacy installer) and platform-specific branching.
  • Confirm logging, runner invocation, and exit status behavior preserved across the new loop.

Possibly related PRs

Suggested reviewers

  • shenxianpeng

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main objective: switching clang tools installation from static binaries to Python wheels, which aligns with the primary changes in both README.md and action.yml.
Linked Issues check ✅ Passed The PR successfully implements the core objective from issue #364: switching clang tools installation from static-binaries to Python wheels by updating documentation and modifying the installation workflow to use clang-tools-wheel.
Out of Scope Changes check ✅ Passed All changes in README.md and action.yml are directly scoped to implementing the wheels-based installation approach from issue #364; no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-364

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b72d11b and 454ad27.

📒 Files selected for processing (2)
  • README.md (2 hunks)
  • action.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: In the cpp-linter-action project, technical improvements can be deferred to separate PRs when they're not critical to the current changes, especially when the current implementation is known to work correctly.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:50:57.038Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:50:57.038Z
Learning: In Nu shell, `$in` is shorthand for a closure that takes 1 argument, so `(...) | complete | $in.exit_code == 0` is equivalent to `(...) | complete | {|in| $in.exit_code == 0}`.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:31:25.319Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:31:25.319Z
Learning: In Nu shell, the pattern `(...) | complete | $in.exit_code == 0` is valid syntax for checking command exit codes, where `complete` captures the command result and `$in` refers to the piped input.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T13:20:51.048Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: README.md:206-211
Timestamp: 2025-08-17T13:20:51.048Z
Learning: For GitHub Actions documentation references in cpp-linter-action, prefer stable workflow syntax documentation links over "canonical" specialized pages that may redirect, as the comprehensive workflow syntax page provides better context and examples for container configuration.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: The cpp-linter/cpp-linter-action repository manages the downstream parsing of command-line arguments through their own cpp-linter/cpp-linter project, so passing empty string values for flags like --extra-arg="" is acceptable and known to work in their implementation.

Applied to files:

  • action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: test (windows-latest, 17)
  • GitHub Check: test (windows-latest, 13)
  • GitHub Check: test (windows-latest, 12)
  • GitHub Check: test (macos-latest, 20)
  • GitHub Check: test (macos-latest, 21)
  • GitHub Check: test (ubuntu-latest, 21)
  • GitHub Check: test (macos-latest, 12)
  • GitHub Check: test (macos-latest, 11)
🔇 Additional comments (3)
action.yml (3)

351-351: Platform detection refactored correctly for consistency.

Line 351 now uses (version | get "build_os") | str starts-with "windows" for Windows detection, aligning with the suggestion from past reviews. This approach is more robust than previous alternatives and consistent with the platform check at line 420. Build_os returns values like "windows-x86_64", "macos-aarch64", and "linux-x86_64", making the string prefix check reliable across all GitHub Actions runners.


402-428: Per-tool installation with correct version-based fallback logic.

The implementation properly:

  • Builds a dynamic tool list (lines 402-410) based on actual usage: clang-tidy is included only if tidy-checks != "-*" (not disabled), and clang-format only if style is not empty
  • Iterates per-tool (lines 413-428) to install only what's needed, avoiding unnecessary installations
  • Applies version-aware fallback logic (lines 415-426):
    • Clang-tidy < v13: uses clang-tools (static binaries) for compatibility
    • Clang-format ≤ v9 on non-Linux: uses clang-tools (addresses issue Switch clang tools from static binaries to wheels? #364 regarding wheel platform tag availability)
    • All other cases: uses clang-tools-wheel for the modern wheels approach
  • Platform detection at line 420 correctly uses (version | get "build_os") | str starts-with "linux" for robust Linux identification

The fallback strategy directly addresses the PR objective to switch to wheels while maintaining backward compatibility for older clang-tidy versions and the wheel availability constraints documented in issue #364.


379-395: Based on my research, I found that the pattern let result = try { ... } catch { ... } was explicitly requested as a feature in Nushell issue #10452, but the implementation status remains unclear from the available documentation. While production code from March 2025 uses similar try-catch assignment patterns successfully, I cannot definitively verify the variable scoping behavior you requested since the Nushell interpreter is not available in the sandbox environment.

The code in action.yml (lines 379-395) follows a pattern where:

  • On successful try block execution, $version_str | into int result should assign to $result
  • On error, the catch block handles it

However, without direct execution capability or explicit Nushell version documentation confirming try-catch expression support, I cannot conclusively verify whether the variable version properly scopes after the try-catch block succeeds.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-format (v16.0.6) reports: 2 file(s) not formatted
  • docs/examples/demo/demo.hpp
  • docs/examples/demo/demo.cpp
clang-tidy (v16.0.6) reports: 7 concern(s)

Have any feedback or feature suggestions? Share it here.

Copy link
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.

I disagree with deprecated any version of clang-tidy before v13.

@2bndy5
Copy link
Collaborator

2bndy5 commented Nov 3, 2025

I think there's a bug in clang-tools-pip. It does not seem to match v9.0.0. Probably a logical error (>= vs >).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1a59d8 and 9de4cf2.

📒 Files selected for processing (1)
  • action.yml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: In the cpp-linter-action project, technical improvements can be deferred to separate PRs when they're not critical to the current changes, especially when the current implementation is known to work correctly.
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: README.md:206-211
Timestamp: 2025-08-17T13:20:51.048Z
Learning: For GitHub Actions documentation references in cpp-linter-action, prefer stable workflow syntax documentation links over "canonical" specialized pages that may redirect, as the comprehensive workflow syntax page provides better context and examples for container configuration.
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 306
File: .readthedocs.yaml:9-16
Timestamp: 2025-08-13T09:33:35.208Z
Learning: Read the Docs v2 config supports `tools.python: latest` as a valid value, not just explicit version strings like "3.12". This was confirmed by a successful build in cpp-linter-action project.
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: The cpp-linter/cpp-linter-action repository manages the downstream parsing of command-line arguments through their own cpp-linter/cpp-linter project, so passing empty string values for flags like --extra-arg="" is acceptable and known to work in their implementation.

Applied to files:

  • action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: test (macos-latest, 20)
  • GitHub Check: test (windows-latest, 21)
  • GitHub Check: test (windows-latest, 13)
  • GitHub Check: test (windows-latest, 10)
  • GitHub Check: test (macos-latest, 19)
  • GitHub Check: test (macos-latest, 15)
  • GitHub Check: test (macos-latest, 9)
  • GitHub Check: test (ubuntu-latest, 15)
🔇 Additional comments (2)
action.yml (2)

43-43: Documentation update is clear and helpful.

The added constraint note aligns with the fallback logic implemented in the installation step and sets proper user expectations.


388-396: Tool selection logic is clean and efficient.

Only clang-tidy and clang-format are installed when actually needed based on inputs, avoiding unnecessary installation overhead.

@2bndy5
Copy link
Collaborator

2bndy5 commented Nov 4, 2025

uv pip install clang-format==9
  × No solution found when resolving dependencies:
  ╰─▶ Because clang-format==9.0.0 has no wheels with a matching platform tag (e.g., `win_amd64`) and you require     
      clang-format==9, we can conclude that your requirements are unsatisfiable.

      hint: Wheels are available for `clang-format` (v9.0.0) on the following platform: `manylinux1_x86_64`

Apparently, there are only wheels for linux from clang-format v9 (or older) PyPI package:

JSON API response snippet

        "6.0.1": [
            {
                "comment_text": "",
                "digests": {
                    "blake2b_256": "e371db01cf5d188d0669af9cfd9f5cf8cfb47898df6ad5c86bc008ab3b73d327",
                    "md5": "fe41668bfa155409944fc6e7dc060b42",
                    "sha256": "1b71b62346e281f96aa9d45a8892427ce1eae7783c24ac0e1efbab1d5340748d"
                },
                "downloads": -1,
                "filename": "clang_format-6.0.1-py2-none-manylinux1_x86_64.whl",
                "has_sig": false,
                "md5_digest": "fe41668bfa155409944fc6e7dc060b42",
                "packagetype": "bdist_wheel",
                "python_version": "py2",
                "requires_python": null,
                "size": 1285495,
                "upload_time": "2019-11-04T14:55:48",
                "upload_time_iso_8601": "2019-11-04T14:55:48.150563Z",
                "url": "https://files.pythonhosted.org/packages/e3/71/db01cf5d188d0669af9cfd9f5cf8cfb47898df6ad5c86bc008ab3b73d327/clang_format-6.0.1-py2-none-manylinux1_x86_64.whl",
                "yanked": false,
                "yanked_reason": null
            },
            {
                "comment_text": "",
                "digests": {
                    "blake2b_256": "166065c642879f290c4233ac75d7f91038509750347271cd135a62cf0b86019c",
                    "md5": "cd777571cebf4190bc04ed4638d68bfd",
                    "sha256": "8028c358d0838e83ea85254bedba846f2106348406c9cdc4f2724779704eedca"
                },
                "downloads": -1,
                "filename": "clang_format-6.0.1-py2.py3-none-manylinux1_x86_64.whl",
                "has_sig": false,
                "md5_digest": "cd777571cebf4190bc04ed4638d68bfd",
                "packagetype": "bdist_wheel",
                "python_version": "py2.py3",
                "requires_python": null,
                "size": 1295165,
                "upload_time": "2019-11-09T15:05:57",
                "upload_time_iso_8601": "2019-11-09T15:05:57.500683Z",
                "url": "https://files.pythonhosted.org/packages/16/60/65c642879f290c4233ac75d7f91038509750347271cd135a62cf0b86019c/clang_format-6.0.1-py2.py3-none-manylinux1_x86_64.whl",
                "yanked": false,
                "yanked_reason": null
            }
        ],
        "7.1.0": [
            {
                "comment_text": "",
                "digests": {
                    "blake2b_256": "1728feeb87d11c17d4734763db4c0a1a6939448f0c11a1713fac142e8dd2f107",
                    "md5": "ca817583469776c6b76aee6e8d058e8d",
                    "sha256": "32629996956b3e34494a926a76835e3371f10c2823c3078f5fbef08bfebbaae7"
                },
                "downloads": -1,
                "filename": "clang_format-7.1.0-py2.py3-none-manylinux1_x86_64.whl",
                "has_sig": false,
                "md5_digest": "ca817583469776c6b76aee6e8d058e8d",
                "packagetype": "bdist_wheel",
                "python_version": "py2.py3",
                "requires_python": null,
                "size": 1330841,
                "upload_time": "2019-11-09T16:17:01",
                "upload_time_iso_8601": "2019-11-09T16:17:01.797915Z",
                "url": "https://files.pythonhosted.org/packages/17/28/feeb87d11c17d4734763db4c0a1a6939448f0c11a1713fac142e8dd2f107/clang_format-7.1.0-py2.py3-none-manylinux1_x86_64.whl",
                "yanked": false,
                "yanked_reason": null
            }
        ],
        "8.0.1": [
            {
                "comment_text": "",
                "digests": {
                    "blake2b_256": "376675e3c14f22c8a7a02c7e337547e29d42bed516ba8067fdef87d3d9346e52",
                    "md5": "2053ad2ea07c26dd5ad62cb7999722d7",
                    "sha256": "d3d0d62281ace883bc6e4188f949308b8f2e4f6de8fac463e5abd1ae02c03db6"
                },
                "downloads": -1,
                "filename": "clang_format-8.0.1-py2.py3-none-manylinux1_x86_64.whl",
                "has_sig": false,
                "md5_digest": "2053ad2ea07c26dd5ad62cb7999722d7",
                "packagetype": "bdist_wheel",
                "python_version": "py2.py3",
                "requires_python": null,
                "size": 1377569,
                "upload_time": "2019-11-20T18:37:41",
                "upload_time_iso_8601": "2019-11-20T18:37:41.956369Z",
                "url": "https://files.pythonhosted.org/packages/37/66/75e3c14f22c8a7a02c7e337547e29d42bed516ba8067fdef87d3d9346e52/clang_format-8.0.1-py2.py3-none-manylinux1_x86_64.whl",
                "yanked": false,
                "yanked_reason": null
            }
        ],
        "9.0.0": [
            {
                "comment_text": "",
                "digests": {
                    "blake2b_256": "25f2d989afaf8a91385f18fd06e6202644be0bc3a1d14548c18a8ece4911e005",
                    "md5": "7b0f1d7a2785a5c2c4e9d695dfdf4583",
                    "sha256": "085342f9e238c9b03a019e20ec23f242f8795a3cca9296ab2427b1dea45e7014"
                },
                "downloads": -1,
                "filename": "clang_format-9.0.0-py2.py3-none-manylinux1_x86_64.whl",
                "has_sig": false,
                "md5_digest": "7b0f1d7a2785a5c2c4e9d695dfdf4583",
                "packagetype": "bdist_wheel",
                "python_version": "py2.py3",
                "requires_python": null,
                "size": 1375189,
                "upload_time": "2019-11-20T19:37:14",
                "upload_time_iso_8601": "2019-11-20T19:37:14.542044Z",
                "url": "https://files.pythonhosted.org/packages/25/f2/d989afaf8a91385f18fd06e6202644be0bc3a1d14548c18a8ece4911e005/clang_format-9.0.0-py2.py3-none-manylinux1_x86_64.whl",
                "yanked": false,
                "yanked_reason": null
            }
        ]

@2bndy5 2bndy5 force-pushed the fix-364 branch 2 times, most recently from cc0b06f to 29a60ef Compare November 4, 2025 19:13
shenxianpeng and others added 2 commits November 4, 2025 11:16
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Brendan <2bndy5@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
action.yml (1)

402-417: Version-aware installation logic is correct; minor performance optimization opportunity.

The fallback logic correctly implements the version-aware installation strategy:

  • Clang-tidy < v13: uses static binaries (clang-tools CLI)
  • Clang-format ≤ v9 on non-Linux: uses static binaries (wheels unavailable for these versions on Windows/macOS)
  • Otherwise: uses wheels (clang-tools-wheel)

This aligns with prior feedback and PR objectives. One minor optimization: the platform detection at line 409 is called once per tool in the loop. Consider computing it once before the loop to avoid redundant system calls:

+        let is_linux = (sys host | get 'long_os_version') | str starts-with "Linux"
         for tool in $tools {
           print $"Installing ($tool) ($version)"
           let cmd = if (
             (($version < 13) and ($tool | str ends-with "tidy"))
             or (
               ($version <= 9)
               and ($tool | str ends-with "format")
-              and not ((sys host | get 'long_os_version') | str starts-with "Linux")
+              and (not $is_linux)
             )
           ) {
             [clang-tools --tool $tool --install $version]
           } else {
             [clang-tools-wheel --tool $tool --version $version]
           }
           ^$'($env.UV_INSTALL_DIR)/uv' ...$uv_args ...$cmd
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3e381b and a5e258b.

📒 Files selected for processing (2)
  • README.md (2 hunks)
  • action.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: In the cpp-linter-action project, technical improvements can be deferred to separate PRs when they're not critical to the current changes, especially when the current implementation is known to work correctly.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:50:57.038Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:50:57.038Z
Learning: In Nu shell, `$in` is shorthand for a closure that takes 1 argument, so `(...) | complete | $in.exit_code == 0` is equivalent to `(...) | complete | {|in| $in.exit_code == 0}`.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:31:25.319Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:31:25.319Z
Learning: In Nu shell, the pattern `(...) | complete | $in.exit_code == 0` is valid syntax for checking command exit codes, where `complete` captures the command result and `$in` refers to the piped input.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: The cpp-linter/cpp-linter-action repository manages the downstream parsing of command-line arguments through their own cpp-linter/cpp-linter project, so passing empty string values for flags like --extra-arg="" is acceptable and known to work in their implementation.

Applied to files:

  • action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: test (macos-latest, 21)
  • GitHub Check: test (macos-latest, 20)
  • GitHub Check: test (macos-latest, 14)
  • GitHub Check: test (macos-latest, 15)
  • GitHub Check: test (macos-latest, 13)
  • GitHub Check: test (ubuntu-latest, 20)
  • GitHub Check: test (ubuntu-latest, 21)
🔇 Additional comments (1)
action.yml (1)

391-401: Tool selection logic is sound; edge case with neither tool in use.

The code correctly identifies which tools to install (clang-tidy if enabled in tidy-checks, clang-format if style is not empty). However, there is an edge case: if a user disables both tools (e.g., tidy-checks: '-*' and style: ''), the tools array will be empty, and the print message will read awkwardly ("Ensuring 18 are present").

This is unlikely in practice and the empty loop is harmless, so no action required unless you want to add a guard to skip the message/loop entirely if no tools are selected.

Copy link
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.

I have pushed any changes that I requested.

This is now the least breaking implementation that I can muster.

Also, the README now states the origin of clang tools in order of install attempts.

Caution

This should be tested in test repo before merging.
We have made some significant changes to how version input is used before it is passed to cpp-linter.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 855de59 and b72d11b.

📒 Files selected for processing (1)
  • action.yml (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: In the cpp-linter-action project, technical improvements can be deferred to separate PRs when they're not critical to the current changes, especially when the current implementation is known to work correctly.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:50:57.038Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:50:57.038Z
Learning: In Nu shell, `$in` is shorthand for a closure that takes 1 argument, so `(...) | complete | $in.exit_code == 0` is equivalent to `(...) | complete | {|in| $in.exit_code == 0}`.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:31:25.319Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:273-321
Timestamp: 2025-08-17T12:31:25.319Z
Learning: In Nu shell, the pattern `(...) | complete | $in.exit_code == 0` is valid syntax for checking command exit codes, where `complete` captures the command result and `$in` refers to the piped input.

Applied to files:

  • action.yml
📚 Learning: 2025-08-17T12:34:25.463Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-action PR: 314
File: action.yml:394-416
Timestamp: 2025-08-17T12:34:25.463Z
Learning: The cpp-linter/cpp-linter-action repository manages the downstream parsing of command-line arguments through their own cpp-linter/cpp-linter project, so passing empty string values for flags like --extra-arg="" is acceptable and known to work in their implementation.

Applied to files:

  • action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: test (windows-latest, 15)
  • GitHub Check: test (windows-latest, 18)
  • GitHub Check: test (windows-latest, 19)
  • GitHub Check: test (windows-latest, 13)
  • GitHub Check: test (macos-latest, 14)
  • GitHub Check: test (macos-latest, 19)
  • GitHub Check: test (ubuntu-latest, 19)
  • GitHub Check: test (macos-latest, 18)
  • GitHub Check: test (macos-latest, 15)
  • GitHub Check: test (macos-latest, 9)
🔇 Additional comments (1)
action.yml (1)

379-395: Version input validation handles all documented cases correctly.

The try-catch block properly handles three cases: empty string (exits early), valid integer (proceeds to installation), and path input (validates existence and exits gracefully). This aligns well with the documented API where version can be empty, an integer, or a path.

Comment on lines 402 to 428
mut tools = []
if ("${{ inputs.tidy-checks }}" != "-*") {
# clang-tidy is being used
$tools = $tools | append "clang-tidy"
}
if ("${{ inputs.style }}" | is-not-empty) {
# clang-format is being used
$tools = $tools | append "clang-format"
}
print $"\n(ansi purple)Ensuring ($tools | str join " and ") ${{ inputs.version }} are present(ansi reset)"
for tool in $tools {
print $"Installing ($tool) ($version)"
let cmd = if (
(($version < 13) and ($tool | str ends-with "tidy"))
or (
($version <= 9)
and ($tool | str ends-with "format")
and not ((sys host | get 'long_os_version') | str starts-with "Linux")
)
) {
[clang-tools --tool $tool --install $version]
} else {
[clang-tools-wheel --tool $tool --version $version]
}
^$'($env.UV_INSTALL_DIR)/uv' ...$uv_args ...$cmd
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Per-tool conditional installation with platform-aware fallback logic is well-structured.

The logic correctly implements the PR objective to switch to wheels while maintaining backward compatibility:

  • Clang-tidy < v13 falls back to clang-tools (static binary) as required
  • Clang-format ≤ v9 on non-Linux platforms falls back to static binaries (addressing the wheel availability issue noted in issue Switch clang tools from static binaries to wheels? #364)
  • All other cases use clang-tools-wheel
  • Tools are only installed if they're actually being used (conditional tool list)

However, verify that the platform detection at line 420 is robust across all runners. The code uses sys host | get 'long_os_version' | str starts-with "Linux" to detect Linux, whereas other steps use GitHub Actions' runner.os variable. Please confirm this approach works consistently on Linux, macOS, and Windows runners:


🏁 Script executed:

#!/bin/bash
# Search for other platform detection patterns in action.yml
rg -n "runner\.os|sys host" action.yml

Length of output: 365


The web search didn't provide specific field details. Let me search more specifically for sys host output examples.Excellent! The GitHub issue shows that users can switch on (sys).host.name as a builtin OS detector that works on Windows, confirming that the name field exists and is used for platform detection. However, I still need to determine if using long_os_version with a string prefix check is equally robust. Let me search more specifically for the consistency of these fields.

The web search confirms that (sys).host.name is a builtin OS detector used for cross-platform detection. However, I need to verify whether the code at line 420 should use the same name field for consistency, or if long_os_version is intentionally different. Let me check the context within the action.yml file more directly to understand the code structure.

Let me search for any tests or documentation in the repository itself:

Actually, let me take a step back and look at the actual action.yml file around those lines to understand the logic better:


🏁 Script executed:

#!/bin/bash
# Get more context around line 351 and line 420 to understand their contexts
rg -n -B5 -A5 "sys host" action.yml | head -50

Length of output: 978


🏁 Script executed:

#!/bin/bash
# Check if there are any test files or discussions about this
rg -rn "long_os_version|sys.*host.*name" . --type yaml --type md | head -20

Length of output: 139


🏁 Script executed:

#!/bin/bash
# Extract the relevant sections to understand context
sed -n '345,360p' action.yml
sed -n '415,430p' action.yml

Length of output: 1258


Refactor platform detection to use consistent name field for robustness.

The verification found an inconsistency in platform detection approaches within the same file:

  • Line 351 uses (sys host | get 'name') == 'Windows' for direct OS comparison
  • Line 420 uses not ((sys host | get 'long_os_version') | str starts-with "Linux") with string prefix matching on version data

The long_os_version field approach at line 420 is fragile—if the version string format changes, the Linux detection breaks. For consistency and robustness across all runners, refactor line 420 to use the established pattern:

and not ((sys host | get 'name') == 'Linux')

This matches the direct field comparison approach already used in line 351 and is more reliable for cross-platform detection.

🤖 Prompt for AI Agents
In action.yml around lines 402 to 428, replace the fragile platform check that
uses long_os_version prefix matching with the consistent direct name check:
change the condition that reads not ((sys host | get 'long_os_version') | str
starts-with "Linux") to use and not ((sys host | get 'name') == 'Linux') so
platform detection matches the existing pattern (e.g., the check at line 351)
and is robust across runners; update only that boolean expression so the overall
if/else logic and command selection remain the same.

Copy link
Collaborator

@2bndy5 2bndy5 Nov 11, 2025

Choose a reason for hiding this comment

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

The long_os_version field approach at line 420 is fragile

This is invalid because name does not identify Linux distribution in a generic way. Rather, it returns the flavor of the Linux distribution, which makes it harder to identify Linux in particular.

> sys host | get name
Ubuntu

Perhaps we can use version | get build_os instead. This should return the compilation target of the nu binary.

> version | get build_os
linux-x86_64

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for nushell documentation or code examples showing what sys host name returns on different platforms
rg -n "sys.*host.*name|build_os" action.yml -B2 -A2

Length of output: 278


🌐 Web query:

nushell version build_os command

💡 Result:

Use the built-in version command and select the build_os field. Examples:

  • Show full version record:
    version
  • Show only build_os:
    version | get build_os

(You can also inspect other fields like build_target, rust_version, build_time.) [1][2]


🌐 Web query:

nushell version build_os output format examples

💡 Result:

Nushell’s version output is a record with keys like build_os (string), build_target, rust_version, build_time, etc. Example outputs showing common build_os values:

  • Windows (example):
    build_os | windows-x86_64. [1]

  • macOS (example):
    build_os | macos-aarch64. [2]

  • Linux / release artifacts (examples):
    build_os entries appear alongside platform tarballs such as x86_64-unknown-linux-gnu or aarch64-unknown-linux-gnu (packaging names; build_os strings in version records are e.g. linux-x86_64 / aarch64). [3]

Full example record (typical output from running version):
version | 0.105.1
major | 0
minor | 105
patch | 1
build_os | windows-x86_64
build_target | x86_64-pc-windows-msvc
rust_version | rustc 1.88.0 ...
build_time | 2025-07-07 09:19:56 +02:00. [1]

References:
[1] nushell issue showing version record (build_os: windows-x86_64).
[2] example showing build_os: macos-aarch64.
[3] release/package listings showing Linux build names.


@2bndy5 Thank you for the correction! You're absolutely right—I misunderstood how sys host | get 'name' behaves on Linux systems. Using version | get build_os is a much more robust approach.

Based on the nushell version command, build_os returns consistent values like "windows-x86_64", "macos-aarch64", and "linux-x86_64", which makes platform detection straightforward across all runners.

For consistency, here's the corrected implementation for line 420 (and optionally line 351 for uniformity):

Line 420 (Linux detection in the clang-tools fallback logic):

and not ((version | get build_os) | str starts-with "linux")

Line 351 (optional: update Windows detection for consistency):

let is_windows = (version | get build_os) | str starts-with "windows"

This approach ensures robust platform detection across all GitHub Actions runners and avoids fragile string matching on version strings. Would you like me to update both locations for consistency, or just line 420?

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

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch clang tools from static binaries to wheels?

3 participants