Skip to content

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Aug 12, 2025

resolves #305
closes #234

This uses dependabot's newer option to combine multiple package ecosystems into 1 group. This means that updates to uv.lock, pyproject.toml, and **/requirements.txt should be submitted in 1 PR.

uv requires pyproject.toml to specify dependency groups. The specified [project] in pyproject.toml shall be considered "virtual" because no build-backend is specified.

Install all dependencies as before with uv:

uv sync --all-groups

Summary by CodeRabbit

  • New Features

    • Added a cache-enable action input (enabled by default) and a unified, cross-platform cpp-linter workflow with caching for faster, more consistent runs.
  • Documentation

    • Docs build upgraded to a newer OS/Python and now installs docs dependencies via a dedicated install job.
    • Documented the new cache-enable input.
  • Chores

    • Consolidated dependencies into project configuration and removed corresponding entries from requirements files.
    • Removed a pre-commit requirements fixer and adjusted dependency-update configuration.

@2bndy5 2bndy5 requested a review from a team as a code owner August 12, 2025 11:03
@2bndy5 2bndy5 requested review from shenxianpeng and removed request for a team August 12, 2025 11:03
@2bndy5 2bndy5 added dependencies Pull requests that update a dependency file python Pull requests that update Python code labels Aug 12, 2025
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 12, 2025

Technically requirements.txt is not used by uv (in favor of pyproject.toml). I have retained the requirements.txt files because:

  1. action.yml uses requirements.txt (not uv)
  2. our mkdocs reusable workflows expect docs/requirements.txt to exist (they don't use uv yet)

2bndy5 added 2 commits August 12, 2025 04:50
reusable workflow does not use uv (yet)
@shenxianpeng
Copy link
Collaborator

We might remove requirements.txt by installing action deps by pip install --group action or uv pip install --group action

@shenxianpeng
Copy link
Collaborator

- requirements: docs/requirements.txt

https://docs.readthedocs.com/platform/stable/build-customization.html#id15

Here also support pip install --group docs

@shenxianpeng
Copy link
Collaborator

If existing requirements.txt could be replaced by pyproject.toml file, that would be great!

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 12, 2025

We might remove requirements.txt by installing action deps by pip install --group action or uv pip install --group action

I had no idea this was supported. I need to investigate what version of pip introduced this option.

uv pip ... doesn't actually use pip. Rather it uses uv's own implementation of pip tools.

- requirements: docs/requirements.txt

https://docs.readthedocs.com/platform/stable/build-customization.html#id15

Here also support pip install --group docs

I often forget about updating RTD config. Nice catch!

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 12, 2025

pip v25.1 introduced the --group option (complying with PEP735). We may have to ensure action.yml updates pip before using the --group option.

Copy link
Collaborator

@shenxianpeng shenxianpeng left a comment

Choose a reason for hiding this comment

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

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 12, 2025

I think I found a way to prevent adding uv to the PATH env var:

env:
  UV_NO_MODIFY_PATH: 1

Using UV in action.yml can also make caching the venv (and action deps) rather easy. This would also resolve #234 . We also wouldn't require runners to have python installed because uv venv automatically downloads a standalone binary distribution of python (as built by a astral-sh sister project).


I'm a bit distracted right now. I'll pick this up later.

@shenxianpeng
Copy link
Collaborator

💯 No rush

Copy link
Contributor

github-actions bot commented Aug 13, 2025

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v12.0.0) reports: 3 concern(s)
  • build/CMakeFiles/3.31.6/CompilerIdC/CMakeCCompilerId.c:2:3: error: [clang-diagnostic-error]

    "A C++ compiler has been selected for C."

    # error "A C++ compiler has been selected for C."
      ^
  • build/CMakeFiles/3.31.6/CompilerIdC/CMakeCCompilerId.c:776:1: warning: [modernize-avoid-c-arrays]

    do not declare C-style arrays, use std::array<> instead

    char const info_version[] = {
    ^
  • build/CMakeFiles/3.31.6/CompilerIdC/CMakeCCompilerId.c:877:5: warning: [modernize-use-trailing-return-type]

    use a trailing return type for this function

    int main(int argc, char* argv[])
    ~~~ ^
    auto                             -> int

Have any feedback or feature suggestions? Share it here.

includes doc'd `minimum-version` for the new input option
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 13, 2025

I looked at the source for hustcer/setup-nu, and it does exactly what I would have to do manually:

  1. read the runner's architecture type and OS type
  2. match output from step 1 and the desired nushell version with nushell GitHub Release assets
  3. download the release asset to a known path (eg GTIHUB_ACTION_PATH)
  4. manually invoke the downloaded release asset wherever needed.

Fortunately, hustcer/setup-nu already does steps 1-3. It also adds the downloaded asset to somewhere in PATH, so step 4 is not really needed.

If the user's workflow already uses a version of nushell, then we may have a conflict on our hands. However, nu shell doesn't seem widely used because it is still in v0.x. I'll just have to keep updating the pinned version of nushell here:

uses: hustcer/setup-nu@985d59ec83ae3e3418f9d36471cda38b9d8b9879 # v3.20.0
with:
version: '0.106.1'

@2bndy5 2bndy5 changed the title feat: migrate to uv feat: migrate to uv and use nu shell Aug 13, 2025
@2bndy5 2bndy5 merged commit ac82521 into main Aug 13, 2025
83 checks passed
@2bndy5 2bndy5 deleted the switch-to-uv branch August 13, 2025 11:25
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 13, 2025

I forgot to review the README. A follow up PR is coming.

2bndy5 added a commit that referenced this pull request Aug 13, 2025
follow-up to #306
Python is no longer required to be installed on the runner anymore.
`uv sync` will automatically download the latest supported stable version of Python.
@2bndy5 2bndy5 mentioned this pull request Aug 13, 2025
2bndy5 added a commit that referenced this pull request Aug 13, 2025
follow-up to #306

Python is no longer required to be installed on the runner anymore.
`uv sync` will automatically download the latest supported stable version of Python.
@shenxianpeng shenxianpeng removed the dependencies Pull requests that update a dependency file label Aug 16, 2025
@coderabbitai coderabbitai bot mentioned this pull request Aug 17, 2025
@assignUser
Copy link

@2bndy5

This means we don't have to write 2 copies of the steps for unix and windows.

Why not set shell: bash on windows jobs instead of adding nu shell? That should also work and not require the additional dependency on another action? (which we have to track in our allowlist in addition to cpp-linter-action)

@shenxianpeng
Copy link
Collaborator

Why not set shell: bash on windows jobs instead of adding nu shell?

That’s a good point. Perhaps reverting this PR could be worth considering, since it looks like two issues (#321 and #323) have come up.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 28, 2025

Why not set shell: bash on windows jobs instead of adding nu shell?

Back when we did that, we got bug reports from people using a windows runner (probably self-hosted) that didn't have bash available (at least not in PATH). Only GithHub's windows runners have bash guaranteed to be available. That is what caused us to start writing duplicate steps in each native shell, which is not very sustainable.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 29, 2025

which we have to track in our allowlist in addition to cpp-linter-action

@assignUser

  1. How is this "allowlist" enforced?
  2. Does the enforcer also check for arbitrary bash scripts used in actions? We also use a bash script downloaded from LLVM to install certain versions of clang tools on Linux runners.

@assignUser
Copy link

@2bndy5

  1. The allow list regex is a commonly used feature to lock down use of arbitrary actions, see docs.
    In the ASF we have an additional layer on top of it due to being an org with 300+ independent projects and an infra staff of ~5, you can find it here. The nu shell sub-action is maintained by a single person which is usually a blocker for actions, given the security implications.
  2. No we don't, but we do try to review each new version at least superficially, before we merge the dependabot PRs. And yes of course there will always be away around things, but we can only do our best to prevent things like the tj-actions-incident from affecting our projects and their downstream dependents.

@assignUser
Copy link

Only GithHub's windows runners have bash guaranteed to be available.

I'd recommend using the official runner images as sys reqs for your action, everyone using self-hosted runners should be capable of adding git bash to the path properly. You are in the good situation to have such a nice baseline, so make use of it! (on the contrary in Arrow we have ~250 nightly jobs to ensure compatibility... 😓)

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 30, 2025

I'd recommend using the official runner images...

This is not up to us. We try to support whatever users throw at us. The real problem here is that the C/C++ language itself never adopted a package manager nor build system. So, running static analysis is a very hairy situation in this project's scope; people end up using very different workflows that are often specifically tailored to their project(s).

The nu shell sub-action is maintained by a single person which is usually a blocker for actions, given the security implications

I understand. We pinned the action using the tagged commit's SHA in an effort to avoid security concerns. Again, I personally reviewed the source of the setup-nu action, and it does everything I would have to do manually. See #306 (comment). I do plan on rigorously reviewing that action's source if we ever need to bump it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor A minor version bump python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate to uv
3 participants