Add LVGL submodule to unix port and tidy up actions#1
Conversation
|
Code size report: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
.github/workflows/ports_unix.yml (1)
8-18: Include user_modules/ in PR path filters so LVGL submodule changes trigger unix CI**Given the new LVGL submodule under user_modules/, PRs updating that pointer won’t trigger this workflow. Add the path to ensure coverage.
pull_request: paths: - '.github/workflows/*.yml' - 'tools/**' - 'py/**' - 'extmod/**' - 'shared/**' - 'lib/**' + - 'user_modules/**' - 'examples/**' - 'mpy-cross/**' - 'ports/unix/**' - 'tests/**'.github/workflows/ruff.yml (1)
20-20: Formatting step won’t fail CI on unformatted files'ruff format --diff' prints a diff but exits zero; use '--check' to fail when changes are needed.
Apply this diff:
- - run: ruff format --diff . + - run: ruff format --check .
🧹 Nitpick comments (29)
.github/dependabot.yml (1)
7-7: Weekly cadence looks good; consider pinning day/time and grouping to reduce PR noiseWeekly is a sensible interval for GitHub Actions. For predictability and lower noise, consider setting a specific day/time/timezone and grouping all actions into a single weekly PR, plus adding labels and a cap on concurrent PRs.
- package-ecosystem: "github-actions" directory: "/" schedule: - interval: "weekly" + interval: "weekly" + day: "monday" + time: "04:00" + timezone: "UTC" + open-pull-requests-limit: 5 + labels: ["dependencies", "github-actions"] + groups: + actions: + patterns: + - "*".github/workflows/ports_webassembly.yml (1)
5-6: Add user_modules/ to PR path filters so LVGL submodule changes trigger builds.**
Right now, this workflow won’t run on PRs that only bump the LVGL submodule (user_modules/lv_binding_micropython) because that path isn’t included in pull_request.paths. Recommend adding it if you want WebAssembly CI to validate such changes.Example:
pull_request: paths: - '...' - 'user_modules/**'.github/workflows/ports_rp2.yml (1)
5-6: Ensure LVGL submodule changes can trigger this workflow when relevant.
If you expect RP2 CI to validate changes to user_modules/lv_binding_micropython (e.g., when updating bindings used in examples or shared code), include that path in pull_request.paths. Otherwise PRs that only adjust the submodule pointer won’t trigger this workflow.Example:
pull_request: paths: - '...' - 'user_modules/**'user_modules/lv_binding_micropython (2)
1-1: Optional: speed up CI by using shallow submodule checkoutFor large CI matrices, consider shallow submodule clones to reduce checkout time, unless you rely on submodule history/tags.
Add in your actions/checkout steps:
- uses: actions/checkout@v4 with: submodules: recursive fetch-depth: 0 # keep 0 if your build needs tags/describe; otherwise consider 1 # For shallow submodules (supported by Git >=2.9): # set in .gitmodules per submodule: # [submodule "user_modules/lv_binding_micropython"] # shallow = true
1-1: Optional: document update policy for the submoduleYou’re pinned to a commit, which is good for determinism. To ease maintenance, document how/when you’ll bump this pointer (e.g., align with MicroPython versioning or LVGL releases), and consider adding a lightweight check in CI that reminds when upstream tags advance.
Example maintenance note for CONTRIBUTING.md:
- We track lv_binding_micropython by pinned commit.
- To update: git submodule update --remote -- user_modules/lv_binding_micropython; validate build; then commit the new gitlink.
- We avoid tracking a moving branch to keep builds reproducible.
.github/workflows/docs.yml (1)
27-29: Pin the Python version for reproducible doc buildsUnpinned Python can introduce churn as ubuntu-latest advances. Consider pinning a stable 3.x.
Apply:
- - uses: actions/setup-python@v5 + - uses: actions/setup-python@v5 + with: + python-version: '3.11'.github/workflows/ports_unix.yml (1)
28-28: If/when building LVGL unix variants here, ensure submodules are fetchedactions/checkout doesn’t fetch submodules by default. If a future job in this workflow builds with user_modules content (e.g., LVGL), add submodules: recursive.
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + submodules: recursiveRepeat for any jobs that depend on user_modules.
.github/workflows/code_size.yml (1)
27-27: Runner uplift to ubuntu-latest: double-check toolchain compatibilitySome size builds pull older toolchains/libs; ubuntu-latest (currently 24.04) can surface incompatibilities (e.g., Python 3.12 defaults, libffi variants). If flakiness appears, pin to 22.04.
Optionally pin:
- runs-on: ubuntu-latest + runs-on: ubuntu-22.04.github/workflows/ports_stm32.yml (1)
5-6: Push restricted to master — confirm default branch and consider tag/manual triggersIf the repo’s default branch is not named “master”, this workflow won’t run on pushes. Also, consider running on tags for releases and allowing manual dispatch.
Proposed update:
on: push: branches: - master + tags: + - 'v*' + workflow_dispatch: pull_request: paths: - '.github/workflows/*.yml'.github/workflows/ports.yml (1)
5-6: Limit to master is fine; consider tags/manual runs for releases and hotfixesAs above, add tag and manual dispatch triggers if you want this to run for version tags or ad-hoc runs.
Proposed update:
on: push: branches: - master + tags: + - 'v*' + workflow_dispatch: pull_request: paths: - '.github/workflows/*.yml'ports/unix/variants/lvgl/mpconfigvariant.mk (1)
1-4: Tighten comment accuracy and make variable style consistentClarify the variant comment (this isn’t the default) and prefer $(TOP) for consistency with the rest of the build system.
Apply this diff:
-# This is the default variant when you `make` the Unix port. +# LVGL-specific variant configuration for the Unix port. FROZEN_MANIFEST ?= $(VARIANT_DIR)/manifest.py -USER_C_MODULES ?= ${TOP}/user_modules +USER_C_MODULES ?= $(TOP)/user_modules.github/workflows/codespell.yml (2)
3-7: Fix trailing spaces to satisfy yamllintYAMLlint flagged trailing spaces on Line 3 and Line 5.
-on: +on: push: - branches: + branches: - master
13-16: Ensure Python toolchain is available and avoid PATH issues with pip --userUsing pip with --user can place binaries outside PATH in GitHub runners. Also, pinning a Python version improves reproducibility. Recommend adding setup-python and installing without --user. While here, exclude the auto-generated lextab.py from codespell to reduce noise.
steps: - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.11' # codespell version should be kept in sync with .pre-commit-config.yml - - run: pip install --user codespell==2.4.1 tomli - - run: codespell + - run: python -m pip install --upgrade pip && python -m pip install codespell==2.4.1 tomli + - run: codespell -S "ports/unix/lextab.py"examples/lvgl.py (1)
22-23: Reduce sleep interval to keep the UI responsiveSleeping for 100 seconds can make the process sluggish to interrupt and less responsive. A shorter interval maintains liveness without busy-waiting.
-while True: - time.sleep(100) +while True: + time.sleep(1).github/workflows/code_formatting.yml (2)
3-7: Remove trailing space to appease yamllintYAMLlint flagged trailing spaces on Line 5.
on: push: - branches: + branches: - master
18-24: Pin Python version and ensure Bash shell for sourced scriptsPinning Python stabilises tooling, and explicitly using Bash avoids shells that don't support source.
- uses: actions/setup-python@v5 + with: + python-version: '3.11' - name: Install packages - run: source tools/ci.sh && ci_c_code_formatting_setup + shell: bash + run: source tools/ci.sh && ci_c_code_formatting_setup - name: Run code formatting - run: source tools/ci.sh && ci_c_code_formatting_run + shell: bash + run: source tools/ci.sh && ci_c_code_formatting_runports/unix/lextab.py (1)
1-3: Exclude auto-generated lextab.py from spell checks/formattersAuto-generated files are noisy for linters/formatters. Consider excluding ports/unix/lextab.py in codespell and any formatting tools to avoid churn.
If you agree, the earlier codespell.yml suggestion already excludes it:
- Change: codespell -> codespell -S "ports/unix/lextab.py"
.github/workflows/pr_hygene.yml (2)
1-11: File name typo: prefer pr_hygiene.yml for consistency and discoverabilityThe workflow name uses “Hygiene”, but the filename is “pr_hygene.yml”. Consider renaming the file to “.github/workflows/pr_hygiene.yml”.
9-11: Ensure the version check has the context it needsThe Makefile’s check runs a remote tags comparison. It should work as-is, but to be robust on forks and shallow clones, consider explicitly fetching tags or asserting the remote exists before the check.
Apply this minimal hardening:
- uses: actions/checkout@v4 - name: Check the version has been updated - run: make check-version-update + run: | + git remote -v + make check-version-update.dockerignore (2)
29-29: Ignoring user_modules blocks copying submodule content into build context (intentional?)Given the Dockerfile runs “git submodule update …” inside the build, excluding user_modules is fine. If you ever want offline builds or to use pre-populated submodules in context, remove this ignore or narrow it (e.g., ignore everything except lv_binding_micropython).
Possible selective allow-list:
user_modules/ +!user_modules/lv_binding_micropython
17-28: Nit: duplicate glob sections (docs/, examples/, tests/, scripts/)Both “dir/” and “**/dir/” are listed. Keeping just the recursive pattern is sufficient.
Apply this cleanup:
-docs/ -**/docs/ - -examples/ -**/examples/ - -tests/ -**/tests/ - -scripts/ -**/scripts/ +**/docs/ +**/examples/ +**/tests/ +**/scripts/Dockerfile (3)
1-1: Pin base image for reproducibilityUsing “ubuntu” pulls a moving target. Pin to a specific LTS (e.g., 22.04 or 24.04) to avoid surprise breakages.
Apply this diff:
-FROM ubuntu AS builder +FROM ubuntu:22.04 AS builder @@ -FROM ubuntu AS runner +FROM ubuntu:22.04 AS runner
7-10: Reduce image size and layers; use --no-install-recommends and clean apt cacheCombine apt commands, avoid recommended packages, and clean apt lists to shrink layers and improve cacheability.
Apply this diff:
-RUN apt-get update && apt-get install -y git libsdl2-dev parallel libfreetype-dev librlottie-dev libavformat-dev libavcodec-dev libswscale-dev libavutil-dev - -RUN apt-get install -y build-essential +RUN apt-get update \ + && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + git build-essential \ + libsdl2-dev libfreetype-dev librlottie-dev \ + libavformat-dev libavcodec-dev libswscale-dev libavutil-dev \ + && rm -rf /var/lib/apt/lists/*
23-27: Trim runtime deps; parallel isn’t needed at runtime; clean apt cache“parallel” is a build-time aid; it’s unnecessary in the runner. Also, clean apt lists.
Apply this diff:
-RUN apt-get update && apt-get install -y libsdl2-dev parallel libfreetype-dev librlottie-dev libavformat-dev libavcodec-dev libswscale-dev libavutil-dev +RUN apt-get update \ + && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + libsdl2-dev libfreetype-dev librlottie-dev \ + libavformat-dev libavcodec-dev libswscale-dev libavutil-dev \ + && rm -rf /var/lib/apt/lists/*.github/workflows/release_actions.yml (1)
45-53: Tagging step is brittle if tag exists; quote GITHUB_OUTPUT usage; ensure newline at EOF
- If the tag already exists, git tag will fail. Guard or force-update.
- Quote $GITHUB_OUTPUT path.
- Ensure file ends with a newline.
Apply this diff:
- run: echo "VERSION=$(make version)" >> $GITHUB_OUTPUT + run: echo "VERSION=$(make version)" >> "$GITHUB_OUTPUT" @@ - git tag ${{ steps.get_version.outputs.VERSION }} - git push origin ${{ steps.get_version.outputs.VERSION }} + VERSION="${{ steps.get_version.outputs.VERSION }}" + if git rev-parse -q --verify "refs/tags/${VERSION}" >/dev/null; then + echo "Tag ${VERSION} already exists, skipping." + else + git tag "${VERSION}" + git push origin "refs/tags/${VERSION}" + fiMakefile (4)
1-1: Declare all relevant phony targetsAdd all non-file-producing targets to .PHONY.
Apply this diff:
-.PHONY: run docker-build +.PHONY: run run-with-file docker-build version check-version-update
5-9: Version discovery may misorder “vX.Y.Z” tags; normalise or filterIf your tags are prefixed with “v”, sort -V may not compare as intended against bare numeric VERSION. Consider stripping a leading “v”.
Apply this diff:
LATEST_TAG = $(eval LATEST_TAG := $(shell \ git ls-remote --tags origin \ - | awk -F/ '{print $$3}' \ + | awk -F/ '{print $$3}' \ + | sed -E 's/^v//' \ | sort -V \ | tail -n 1))$(LATEST_TAG)
17-18: Simplify and harden version bump check; clearer intent and messagesCurrent condition is a bit dense. This form reads easier and avoids echo -e portability pitfalls.
Apply this diff:
check-version-update: - @if [ "$(VERSION)" = "$(LATEST_TAG)" ] || [ "$(VERSION)" != "$$(echo -e "$(LATEST_TAG)\n$(VERSION)" | sort -V | tail -n 1)" ]; then echo "Version $(VERSION) has not been updated"; exit 1; fi + @latest="$(LATEST_TAG)"; \ + if [ -z "$$latest" ]; then \ + echo "No existing tags found. Proceeding with VERSION $(VERSION)"; exit 0; \ + fi; \ + if [ "$(VERSION)" = "$$latest" ]; then \ + echo "Version $(VERSION) has not been updated (matches latest tag $$latest)"; exit 1; \ + fi; \ + top="$$(printf '%s\n%s\n' "$$latest" "$(VERSION)" | sort -V | tail -n 1)"; \ + if [ "$$top" != "$(VERSION)" ]; then \ + echo "Version $(VERSION) is not greater than latest tag $$latest"; exit 1; \ + fi
21-24: Document X11 requirements or provide a helper to set xhost permissionsRun commands assume X server access. A brief README note or a helper target to run xhost +si:localuser:root improves UX.
Would you like me to add a short README snippet and an optional make target that runs xhost safely for local testing?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
.dockerignore(1 hunks).github/FUNDING.yml(0 hunks).github/dependabot.yml(1 hunks).github/workflows/biome.yml(0 hunks).github/workflows/code_formatting.yml(1 hunks).github/workflows/code_size.yml(2 hunks).github/workflows/codespell.yml(1 hunks).github/workflows/commit_formatting.yml(0 hunks).github/workflows/docs.yml(1 hunks).github/workflows/examples.yml(0 hunks).github/workflows/mpremote.yml(0 hunks).github/workflows/mpy_format.yml(1 hunks).github/workflows/ports.yml(1 hunks).github/workflows/ports_alif.yml(0 hunks).github/workflows/ports_cc3200.yml(0 hunks).github/workflows/ports_esp32.yml(1 hunks).github/workflows/ports_esp8266.yml(0 hunks).github/workflows/ports_mimxrt.yml(0 hunks).github/workflows/ports_nrf.yml(0 hunks).github/workflows/ports_powerpc.yml(0 hunks).github/workflows/ports_qemu.yml(0 hunks).github/workflows/ports_renesas-ra.yml(0 hunks).github/workflows/ports_rp2.yml(1 hunks).github/workflows/ports_samd.yml(0 hunks).github/workflows/ports_stm32.yml(1 hunks).github/workflows/ports_unix.yml(1 hunks).github/workflows/ports_webassembly.yml(1 hunks).github/workflows/ports_windows.yml(0 hunks).github/workflows/ports_zephyr.yml(0 hunks).github/workflows/pr_hygene.yml(1 hunks).github/workflows/release_actions.yml(1 hunks).github/workflows/ruff.yml(1 hunks).gitmodules(1 hunks)Dockerfile(1 hunks)Makefile(1 hunks)examples/lvgl.py(1 hunks)ports/unix/lextab.py(1 hunks)ports/unix/variants/lvgl/manifest.py(1 hunks)ports/unix/variants/lvgl/mpconfigvariant.h(1 hunks)ports/unix/variants/lvgl/mpconfigvariant.mk(1 hunks)user_modules/lv_binding_micropython(1 hunks)
💤 Files with no reviewable changes (16)
- .github/workflows/mpremote.yml
- .github/workflows/ports_cc3200.yml
- .github/workflows/commit_formatting.yml
- .github/workflows/biome.yml
- .github/workflows/ports_renesas-ra.yml
- .github/workflows/examples.yml
- .github/FUNDING.yml
- .github/workflows/ports_samd.yml
- .github/workflows/ports_nrf.yml
- .github/workflows/ports_esp8266.yml
- .github/workflows/ports_powerpc.yml
- .github/workflows/ports_qemu.yml
- .github/workflows/ports_mimxrt.yml
- .github/workflows/ports_alif.yml
- .github/workflows/ports_windows.yml
- .github/workflows/ports_zephyr.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
ports/unix/variants/lvgl/manifest.py (1)
tools/manifestfile.py (1)
include(337-401)
🪛 actionlint (1.7.7)
.github/workflows/release_actions.yml
15-15: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
22-22: the runner of "docker/login-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
29-29: the runner of "docker/build-push-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/release_actions.yml
[error] 33-33: trailing spaces
(trailing-spaces)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/code_formatting.yml
[error] 5-5: trailing spaces
(trailing-spaces)
.github/workflows/codespell.yml
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 5-5: trailing spaces
(trailing-spaces)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 4-4: Target "VERSION" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (17)
.github/workflows/ports_webassembly.yml (2)
5-6: Gating push triggers to master reduces CI noise — looks good.
This aligns with the repo pattern and keeps CI focused on the default branch.
5-6: Confirm branch naming and release/hotfix needs.
If you ever switch default branch naming (e.g., to main) or want CI on release/hotfix branches, this filter will block those push-triggered runs. PRs are still covered..github/workflows/ports_rp2.yml (1)
5-6: Master-only push gating is fine here.
Keeps push-triggered runs constrained to the default branch; PR coverage remains unchanged..gitmodules (2)
77-79: Submodule addition LGTM.
URL and path look correct for lv_binding_micropython.
77-79: Double-check naming consistency across docs/scripts.
PR text mentions “lv_micropython_bindings” but the submodule is “lv_binding_micropython”. Ensure all build scripts, manifests, and docs reference the actual path used here.user_modules/lv_binding_micropython (1)
1-1: Submodule pin looks good for reproducible buildsPinning lv_binding_micropython to a specific commit ensures deterministic builds and aligns well with CI and release workflows.
.github/workflows/ports_esp32.yml (1)
5-6: Verify branch-name consistency across workflowsThe initial grep didn’t surface any branch filters—likely due to inline lists or alternate YAML syntax—so please manually audit all your workflows to confirm that push triggers use the same branch name. A mismatch (some using
master, othersmain) will cause unexpected CI gaps on pushes. In particular:
- .github/workflows/ports_esp32.yml specifies
branches: [master]- Review every
.github/workflows/*.ymlthat useson: push(including inline notation) and ensure they all reference the same branch..github/workflows/docs.yml (1)
5-10: Good gating and path scoping for doc buildsLimiting push runs to master and scoping to docs/py/tests paths is sensible and reduces unnecessary CI load.
.github/workflows/ports_unix.yml (1)
5-6: Restricting push runs to master looks fine; verify branch-name consistency across workflowsEnsure all workflows agree on the default branch (master vs main) to avoid missing push-triggered runs.
You can reuse the audit script provided in the esp32 workflow comment to check for inconsistencies.
.github/workflows/code_size.yml (1)
10-10: Nice: include user_modules/ to capture LVGL submodule changes in code-size reports**This ensures code-size diffs reflect updates to the LVGL binding submodule.
.github/workflows/mpy_format.yml (1)
5-6: Restricting push runs to master looks fine; verify branch-name consistency across workflowsAs with other workflows, confirm the repository’s default branch naming is uniform to avoid missed runs.
Use the branch-audit script from the esp32 workflow comment to standardise master/main usage across all workflows.
ports/unix/variants/lvgl/manifest.py (1)
1-3: LVGL unix variant manifest looks correctIncludes are ordered sensibly (base variant, asyncio, then LVGL bindings). This should pick up the submodule’s unix manifest as intended.
examples/lvgl.py (1)
17-20: Confirm SDL helper APIs in lv_binding_micropython submoduleI didn’t find any definitions for lv.sdl_window_create, lv.sdl_mouse_create, lv.sdl_keyboard_create or keyboard.set_group in the checked repository. Please verify that the pinned lv_binding_micropython submodule commit includes these SDL helper APIs.
• Check the submodule’s bindings/… directory for implementations of the SDL helper functions
• Ensure the Micropython LVGL bindings expose sdl_window_create, sdl_mouse_create, sdl_keyboard_create and support keyboard.set_group() at the chosen commitports/unix/variants/lvgl/mpconfigvariant.h (1)
27-36: LGTM: sensible feature toggles for the LVGL variantThe ROM level, scheduler enable, and builtin init settings look appropriate for LVGL on unix. Include path to mpconfigvariant_common.h also looks correct.
ports/unix/lextab.py (1)
1-3: Confirmed single lextab.py in repo; no duplicate or import references detected
The script only found one instance oflextab.py(ports/unix/lextab.py) and no staticimport lextaborfrom … import lextabusages in the codebase. This autogenerated file won’t be shadowed by another copy within this repository.Dockerfile (1)
11-15: Git submodule update relies on .git being present in build contextThis works since .dockerignore doesn’t exclude .git. Keep this in mind if you later exclude .git for smaller contexts; you’d then need to fetch the submodule via a full clone in the Dockerfile (git clone …) or pass submodules in context.
Would you like me to provide an alternative Dockerfile pattern that does a shallow clone with submodules to avoid sending .git in the context?
Makefile (1)
11-13: docker-build context relies on Dockerfile not being ignoredThis target will fail if the Dockerfile remains ignored in .dockerignore. Please remove Dockerfile from .dockerignore as suggested above.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/ports_unix.yml (1)
28-29: Enable recursive submodules checkout for LVGL bindingsThe CI script initialises a fixed set of submodules (via
$SUBMODULES) but does not includeuser_modules/lv_micropython_bindings. To ensure the LVGL submodule is fetched without further CI changes, enable recursive submodule checkout in your workflow:• File:
.github/workflows/ports_unix.yml
Lines: 28–29- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + submodules: recursiveThis ensures all submodules— including
user_modules/lv_micropython_bindings—are cloned beforetools/ci.shruns.
🧹 Nitpick comments (3)
.github/workflows/ports_unix.yml (3)
3-9: Broaden triggers: run on tags and allow manual dispatchWith automatic tagging introduced in this PR, it’s useful to run this workflow on tags as well. Adding a manual trigger helps when diagnosing CI or testing changes that don’t touch filtered paths.
Apply this diff:
on: push: branches: - master + tags: + - 'v*' + - 'release-*' + workflow_dispatch: pull_request:
8-18: Ensure PRs touching submodules or user modules trigger the workflowYou’ve added an LVGL submodule under user_modules. As currently written, PRs that only bump the submodule (or only touch .gitmodules/user_modules) won’t run this workflow due to the path filter. Consider adding entries for .gitmodules and user_modules to avoid silent skips.
Apply this diff:
pull_request: paths: - '.github/workflows/*.yml' + - '.gitmodules' - 'tools/**' - 'py/**' - 'extmod/**' - 'shared/**' - 'lib/**' + - 'user_modules/**' - 'examples/**' - 'mpy-cross/**' - 'ports/unix/**' - 'tests/**'If you prefer to limit scope further, use the specific submodule path:
- user_modules/lv_micropython_bindings/**
20-23: Set minimal GITHUB_TOKEN permissionsFor hardening, explicitly set minimal permissions. Most build-and-test workflows only need contents: read.
You can add:
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true + +permissions: + contents: read
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ports_unix.yml(1 hunks)
⏰ 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). (20)
- GitHub Check: qemu_riscv64
- GitHub Check: qemu_arm
- GitHub Check: macos
- GitHub Check: stackless_clang
- GitHub Check: standard_v2
- GitHub Check: settrace_stackless
- GitHub Check: coverage_32bit
- GitHub Check: gil_enabled
- GitHub Check: nanbox
- GitHub Check: longlong
- GitHub Check: build_stm32 (stm32_misc_build)
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: standard
- GitHub Check: build_stm32 (stm32_pyb_build)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build_idf (esp32_build_cmod_spiram_s2)
- GitHub Check: build_idf (esp32_build_s3_c3)
- GitHub Check: build_idf (esp32_build_c2_c6)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.gitignore(1 hunks)
⏰ 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). (20)
- GitHub Check: build_stm32 (stm32_pyb_build)
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: build
- GitHub Check: sanitize_address
- GitHub Check: sanitize_undefined
- GitHub Check: qemu_riscv64
- GitHub Check: stackless_clang
- GitHub Check: float_clang
- GitHub Check: settrace_stackless
- GitHub Check: coverage_32bit
- GitHub Check: gil_enabled
- GitHub Check: standard_v2
- GitHub Check: nanbox
- GitHub Check: longlong
- GitHub Check: standard
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build_idf (esp32_build_c2_c6)
- GitHub Check: build_idf (esp32_build_s3_c3)
- GitHub Check: build_idf (esp32_build_cmod_spiram_s2)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.github/workflows/release_actions.yml (8)
10-12: Remove trailing spaces in permissions block (YAML lint errors)These lines have trailing spaces flagged by yamllint. Clean them to avoid CI lint noise.
- permissions: - contents: read - packages: write + permissions: + contents: read + packages: write
41-42: Trailing spaces in second job’s permissions blockSame lint issue here; remove trailing spaces.
- permissions: - contents: write + permissions: + contents: write
58-58: Add a newline at end of fileYAML/style linters typically require a trailing newline.
- git push origin ${{ steps.get_version.outputs.VERSION }} + git push origin ${{ steps.get_version.outputs.VERSION }} +
20-22: Normalise version for Docker tag safety and robustnessIf make version returns characters invalid for Docker tags (e.g., + or uppercase), the push will fail. Emit both raw VERSION and a Docker-safe DOCKER_VERSION output.
- - name: Get version - id: get_version - run: echo "VERSION=$(make version)" >> $GITHUB_OUTPUT + - name: Get version + id: get_version + run: | + VERSION="$(make version)" + echo "VERSION=$VERSION" >> "$GITHUB_OUTPUT" + # Docker tag safe: lower-case and restrict to [a-z0-9_.-] + DOCKER_VERSION="$(printf '%s' "$VERSION" \ + | tr '[:upper:]' '[:lower:]' \ + | sed -E 's/[^a-z0-9_.-]+/-/g' \ + | sed -E 's/^-+|-+$//g')" + echo "DOCKER_VERSION=$DOCKER_VERSION" >> "$GITHUB_OUTPUT"
31-38: Use Docker-safe tag output and enable BuildKit cache to speed builds
- Use DOCKER_VERSION from the previous step to avoid invalid tags.
- Add GitHub Actions cache for faster subsequent builds.
- name: Build and push uses: docker/build-push-action@v5 with: context: . push: true + cache-from: type=gha + cache-to: type=gha,mode=max tags: ${{ env.IMAGE_NAME }}:latest - ${{ env.IMAGE_NAME }}:${{ steps.get_version.outputs.VERSION }} + ${{ env.IMAGE_NAME }}:${{ steps.get_version.outputs.DOCKER_VERSION }}
46-48: Fetch full history for safe tag operationsFetching with full depth ensures tags can be discovered/compared if needed by later steps.
- name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@v4 + with: + fetch-depth: 0
53-58: Make tag push idempotent and scope git config locally
- Avoid using --global; set repo-local config.
- Fetch tags and skip creation if tag already exists. Prevents failures on reruns.
- name: Create and push tag run: | - git config --global user.email "github-actions[bot]@users.noreply.github.com" - git config --global user.name "github-actions[bot]" - git tag ${{ steps.get_version.outputs.VERSION }} - git push origin ${{ steps.get_version.outputs.VERSION }} + git config user.email "github-actions[bot]@users.noreply.github.com" + git config user.name "github-actions[bot]" + git fetch --tags --force + if git rev-parse -q --verify "refs/tags/${{ steps.get_version.outputs.VERSION }}"; then + echo "Tag ${{ steps.get_version.outputs.VERSION }} already exists; skipping." + else + git tag ${{ steps.get_version.outputs.VERSION }} + git push origin ${{ steps.get_version.outputs.VERSION }} + fi
49-51: Mirror the Docker-safe version output here tooKeep the second job in sync with the first job’s outputs to avoid Docker tag issues if you later reuse the value.
- name: Get version id: get_version - run: echo "VERSION=$(make version)" >> $GITHUB_OUTPUT + run: | + VERSION="$(make version)" + echo "VERSION=$VERSION" >> "$GITHUB_OUTPUT" + DOCKER_VERSION="$(printf '%s' "$VERSION" \ + | tr '[:upper:]' '[:lower:]' \ + | sed -E 's/[^a-z0-9_.-]+/-/g' \ + | sed -E 's/^-+|-+$//g')" + echo "DOCKER_VERSION=$DOCKER_VERSION" >> "$GITHUB_OUTPUT"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.dockerignore(1 hunks).github/workflows/release_actions.yml(1 hunks).github/workflows/ruff.yml(1 hunks).gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ruff.yml
- .dockerignore
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/release_actions.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 58-58: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (20)
- GitHub Check: qemu_arm
- GitHub Check: macos
- GitHub Check: float_clang
- GitHub Check: qemu_riscv64
- GitHub Check: stackless_clang
- GitHub Check: qemu_mips
- GitHub Check: gil_enabled
- GitHub Check: coverage_32bit
- GitHub Check: longlong
- GitHub Check: standard_v2
- GitHub Check: standard
- GitHub Check: build_idf (esp32_build_cmod_spiram_s2)
- GitHub Check: build_idf (esp32_build_s3_c3)
- GitHub Check: build
- GitHub Check: build_idf (esp32_build_c2_c6)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: build_stm32 (stm32_pyb_build)
- GitHub Check: build_stm32 (stm32_misc_build)
🔇 Additional comments (1)
.github/workflows/release_actions.yml (1)
24-29: Solid GHCR login and build/publish setup; versions and interpolation look correct
- docker/login-action@v3 and docker/build-push-action@v5 are current.
- IMAGE_NAME uses proper Actions interpolation.
- Double-tagging latest and version is correct.
Also applies to: 31-38
|
@coderabbitai Please apply your nitpicks as full comments |
|
@minyiky I'll provide a comprehensive review with detailed, full comments on all the changes in this PR. Let me conduct a thorough analysis of the LVGL submodule addition and GitHub Actions cleanup. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.gitmodules (1)
77-79: LVGL submodule addition looks good; remember recursive checkout where it’s actually needed
- Declaration/path/URL are correct and consistent with the repo layout and usage in manifests/Docker builds.
- When building LVGL variants (e.g., unix LVGL or Docker builder stage), ensure CI uses recursive submodule checkout because lv_binding_micropython has nested submodules.
Given your preference to avoid CI overhead unless required, only update the specific workflows that build/test the LVGL variant. When you do, add:
- uses: actions/checkout@v4 with: submodules: recursiveIf you want, I can scan the workflows you intend to use for LVGL and open a small PR to add this only there.
user_modules/lv_binding_micropython (1)
1-1: Ensure recursive init where LVGL is built (lv_binding_micropython has nested submodules)lv_binding_micropython itself uses nested submodules (e.g., lvgl, SDL backends). Builds that compile the Unix LVGL variant must run a recursive submodule init, either via actions/checkout with submodules: recursive or an explicit git submodule update --init --recursive user_modules/lv_binding_micropython. Noting your preference to avoid blanket recursive submodule fetches, limit this to the workflows/paths that actually build the LVGL Unix variant and release images; Docker already appears to handle this.
Run this script to verify checkout steps for submodules in workflows that likely build ports/unix or images, and confirm Docker/Make paths explicitly init recursively:
#!/bin/bash set -euo pipefail echo "1) Identify workflow files:" fd -H -t f -p '.github/workflows' -e yml -e yaml || true echo echo "2) Checkout steps and submodule flags in those workflows:" rg -n -C2 -g '.github/workflows/*.{yml,yaml}' 'uses:\s*actions/checkout@' || echo "No checkout steps found" rg -n -C2 -g '.github/workflows/*.{yml,yaml}' 'submodules:\s*(true|recursive)' || echo "No workflows enable submodules" echo echo "3) Find workflows that build unix/variants/lvgl or produce images (heuristic):" rg -n -C2 -g '.github/workflows/*.{yml,yaml}' -e 'ports.*unix' -e 'lvgl' -e 'docker build|buildx' || true echo echo "4) Search Dockerfiles/Makefiles for recursive submodule init:" fd -H -t f -e Dockerfile -e dockerfile -e mk -e makefile | while read -r f; do echo "--- $f" rg -n -C1 -e 'git submodule update --init --recursive' -e '--recurse-submodules' "$f" || true done echo echo "5) Confirm .gitmodules entry exists and is correct:" if [[ -f .gitmodules ]]; then git config -f .gitmodules --get-regexp 'submodule\..*lv_binding_micropython\..*' || true else echo ".gitmodules not found" fi
🧹 Nitpick comments (29)
.github/dependabot.yml (1)
7-7: Optional: pin scheduling details and consider grouping + Docker updatesTo make updates more predictable and reduce PR noise further:
- Add day/time/timezone so updates land in a defined off-hours window.
- Use groups to bundle Actions updates into a single PR.
- Set an open PR limit to avoid backlog.
- Since this repo now has a Dockerfile, consider adding a Docker ecosystem entry so base images are kept current.
Example configuration (for reference):
version: 2 updates: - package-ecosystem: "github-actions" directory: "/" schedule: interval: "weekly" day: "sunday" time: "02:00" timezone: "UTC" groups: gha-deps: patterns: - "*" open-pull-requests-limit: 5 labels: - dependencies - github-actions - package-ecosystem: "docker" directory: "/" schedule: interval: "weekly" day: "sunday" time: "02:00" timezone: "UTC" groups: base-image: patterns: - "*" labels: - dependencies - docker.github/workflows/ports_webassembly.yml (1)
5-6: Master-only push gating: OK; consider running on tags as wellRestricting pushes to master is reasonable. If you plan to run CI on release tags (you mentioned adding automatic tagging), consider including tags to avoid missing release builds/tests.
Proposed change:
on: push: branches: - master + tags: + - 'v*'Would you like the same tag trigger added to the other gated workflows for consistency?
.github/workflows/mpy_format.yml (1)
5-6: Gating push runs to master is fine; optionally add tag trigger for releasesThis keeps CI quiet on branch pushes, relying on PR runs. If tools should be verified on release tags, include a tag trigger.
Suggested tweak:
on: push: branches: - master + tags: + - 'v*'Confirm your default branch is “master”; if it ever changes to “main”, this gate will suppress push runs unexpectedly. Want me to add a brief note in CONTRIBUTING or migrate these to a shared workflow with an input-driven branch?
.github/workflows/ports_rp2.yml (1)
5-6: Push gated to master: consistent; add tags if release builds are desiredLooks consistent with the broader CI tidy-up. If rp2 artefacts or tests should run on release tags, include a tag filter.
Proposed addition:
on: push: branches: - master + tags: + - 'v*'Also, please confirm the repository’s default branch is indeed “master”; otherwise pushes won’t trigger this workflow.
user_modules/lv_binding_micropython (2)
1-1: Optional: consider shallow submodule to speed CI clonesIf CI clone time becomes a concern, you can mark this submodule as shallow to avoid fetching full history:
Add to .gitmodules (adjust name if different):
[submodule "user_modules/lv_binding_micropython"] path = user_modules/lv_binding_micropython url = https://github.com/lvgl/lv_binding_micropython.git shallow = trueThen run:
- git submodule sync --recursive
- git submodule update --init --recursive --depth=1
1-1: Document/update strategy for the submoduleDecide whether you want to:
- Pin to specific commits and bump manually (current approach, safest), or
- Track a stabilised branch/tag via .gitmodules (e.g., branch = release/vX) and periodically update.
Whichever you choose, a short note in CONTRIBUTING.md or the Unix LVGL variant README avoids confusion for future bump PRs.
.github/workflows/docs.yml (1)
5-10: Include tools changes in push path filter to catch ci.sh edits used by this jobThis workflow calls
tools/ci.sh(step “Build unix port”). Changes totools/**won’t currently trigger the docs build on push. Addtools/**to the push path filter.Apply this diff within the push.paths block:
push: branches: - master paths: - docs/** - py/** - tests/cpydiff/** + - tools/**Additionally, consider mirroring this in the pull_request paths for parity (YAML snippet for reference):
pull_request: paths: - docs/** - py/** - tests/cpydiff/** - tools/**.gitignore (1)
21-23: Keep .gitignore minimal as per header; consider moving IDE excludes to local excludesThe file header recommends avoiding repo-specific IDE rules. Either move this to your global gitignore or .git/info/exclude, or keep it if you prefer the convenience for collaborators.
Apply this diff only if you want to strictly follow the stated policy:
-# IDE settings -.vscode/ -.github/workflows/code_size.yml (1)
29-32: Checkout submodules for future-proofing when paths include user_modules/If the build ever reads code from submodules (e.g. user_modules), ensure they’re available in the workspace.
- uses: actions/checkout@v4 with: - fetch-depth: 100 + fetch-depth: 100 + submodules: recursive.github/workflows/codespell.yml (1)
3-7: Clean up trailing spaces flagged by yamllintHarmless at runtime but noisy for linters.
-on: +on: push: - branches: + branches: - master pull_request:.github/workflows/code_formatting.yml (3)
5-5: Remove trailing whitespace to satisfy YAML lintersThere’s a trailing space after "branches:" which YAMLlint flagged. It’s harmless to execution but noisy in CI.
- branches: + branches:
18-18: Pin the Python version to avoid CI driftactions/setup-python without an explicit version can lead to non-repeatable builds when ubuntu-latest updates. Pin a version (adjust as needed for the repo).
- - uses: actions/setup-python@v5 + - uses: actions/setup-python@v5 + with: + python-version: '3.11'
20-24: Enable bash strict mode for clearer failuresAdd set -euo pipefail so the job fails fast on any error instead of continuing with partial state.
- - name: Install packages - run: source tools/ci.sh && ci_c_code_formatting_setup + - name: Install packages + run: | + set -euo pipefail + source tools/ci.sh + ci_c_code_formatting_setup - - name: Run code formatting - run: source tools/ci.sh && ci_c_code_formatting_run + - name: Run code formatting + run: | + set -euo pipefail + source tools/ci.sh + ci_c_code_formatting_runMakefile (4)
1-1: Declare all recipe-only targets as PHONYAvoid accidental clashes with files/directories named like the targets.
-.PHONY: run docker-build +.PHONY: run run-with-file docker-build version check-version-update
5-9: Simplify and stabilise LATEST_TAG evaluationThe current $(eval …) pattern re-evaluates on each expansion and may pick annotated tag dereferences (^{}). Use a simple, one-time assignment, filter out derefs, and normalise a leading v.
-LATEST_TAG = $(eval LATEST_TAG := $(shell \ - git ls-remote --tags origin \ - | awk -F/ '{print $$3}' \ - | sort -V \ - | tail -n 1))$(LATEST_TAG) +LATEST_TAG := $(shell \ + git ls-remote --tags --refs origin \ + | awk -F/ '{print $$3}' \ + | sed -E 's/\^\{\}//; s/^v//' \ + | sort -V \ + | tail -n 1)
17-18: Harden version bump check (normalise tags, avoid echo -e, handle no tags)
- Normalise leading v on both sides to prevent false negatives.
- Prefer printf to avoid echo -e portability pitfalls.
- Treat no tags as 0.0.0 so initial releases pass.
-check-version-update: - @if [ "$(VERSION)" = "$(LATEST_TAG)" ] || [ "$(VERSION)" != "$$(echo -e "$(LATEST_TAG)\n$(VERSION)" | sort -V | tail -n 1)" ]; then echo "Version $(VERSION) has not been updated"; exit 1; fi +check-version-update: + @latest="$(LATEST_TAG)"; latest="$${latest:-0.0.0}"; \ + version="$(VERSION)"; \ + latest_stripped="$${latest#v}"; version_stripped="$${version#v}"; \ + if [ "$$version_stripped" = "$$latest_stripped" ] || \ + [ "$$version_stripped" != "$$(printf '%s\n' "$$latest_stripped" "$$version_stripped" | sort -V | tail -n 1)" ]; then \ + echo "Version $(VERSION) has not been updated"; exit 1; \ + fi
23-24: Quote paths in run-with-file to handle spaces and special charsThis prevents breakage when $(FILE) or the working directory contains spaces.
run-with-file: - @docker run -it --rm -e DISPLAY=$(DISPLAY) -v /tmp/.X11-unix:/tmp/.X11-unix:ro -v $$(pwd)/$(FILE):/$(FILE) micropython /$(FILE) + @docker run -it --rm -e DISPLAY=$(DISPLAY) \ + -v /tmp/.X11-unix:/tmp/.X11-unix:ro \ + -v "$$(pwd)/$(FILE):/$(FILE)" \ + micropython "/$(FILE)".github/workflows/pr_hygene.yml (2)
1-1: Rename file to pr_hygiene.yml (spelling fix)The workflow name is “PR Hygiene”, but the filename is misspelt as pr_hygene.yml. Renaming improves clarity and avoids future confusion.
2-12: Add minimal permissions and concurrency to avoid redundant/over-privileged runsTighten the token permissions and cancel superseded runs on the same PR.
name: PR Hygiene on: pull_request: +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: check-version-update: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - name: Check the version has been updated run: make check-version-updateports/unix/variants/lvgl/mpconfigvariant.mk (2)
1-1: Fix misleading comment about default variantThis file defines the LVGL variant, not the default Unix variant. Suggest updating the comment to avoid confusion.
-# This is the default variant when you `make` the Unix port. +# LVGL variant configuration for the Unix port.
4-4: Use $(TOP) for consistency with other Make variablesMinor consistency improvement; both forms work, but the rest of the file uses $(…).
-USER_C_MODULES ?= ${TOP}/user_modules +USER_C_MODULES ?= $(TOP)/user_modulesDockerfile (4)
7-10: Reduce layers, pin base image, and clean up apt caches for smaller, more reproducible imagesCombine apt installs, use --no-install-recommends, and clean apt lists in the same layer. Also consider pinning the Ubuntu base for reproducibility.
Apply this diff:
-FROM ubuntu AS builder +FROM ubuntu:24.04 AS builder @@ -RUN apt-get update && apt-get install -y git libsdl2-dev parallel libfreetype-dev librlottie-dev libavformat-dev libavcodec-dev libswscale-dev libavutil-dev - -RUN apt-get install -y build-essential +RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + git build-essential pkg-config ca-certificates \ + libsdl2-dev libfreetype-dev librlottie-dev \ + libavformat-dev libavcodec-dev libswscale-dev libavutil-dev \ + && rm -rf /var/lib/apt/lists/*Optional (verify necessity before adding): the unix port often needs python3 and libffi-dev at build-time. If your build occasionally fails or picks up fewer features, consider adding:
- python3, python3-venv, python3-setuptools
- libffi-dev
I can generate a variant including these if you want.
11-14: Docker build depends on .git presence; ensure .dockerignore doesn’t exclude it, or avoid relying on repo metadataThis step will fail if the Docker build context excludes the .git directory (common in .dockerignore), or when building from a tarball. It also makes the build sensitive to the host checkout configuration.
Two robust alternatives:
- Ensure actions/checkout includes submodules and do not exclude .git in .dockerignore (works only in CI).
- Or, avoid repository metadata in the image and fetch the submodule inside the container by URL (works everywhere). For example:
-COPY . /src - -RUN git submodule update --init --recursive user_modules/lv_binding_micropython +COPY . /src +# Ensure lv_binding_micropython is present even if .git is not available +RUN test -d user_modules/lv_binding_micropython/.git || { \ + rm -rf user_modules/lv_binding_micropython && \ + git clone --recurse-submodules https://github.com/lvgl/lv_binding_micropython.git user_modules/lv_binding_micropython; \ + }If you prefer to rely on CI checkout, add this to the checkout step in your workflow:
- submodules: recursive
- fetch-depth: 0
Would you like me to adjust the workflow accordingly?
19-19: Parameterise DEBUG and parallelism for flexibilityHardcoding DEBUG=1 forces a debug build in all images. Consider ARGs so callers can choose.
- RUN make -B -j $(nproc) -C ports/unix DEBUG=1 VARIANT=lvgl + ARG DEBUG=0 + ARG JOBS + RUN make -B ${JOBS:+-j ${JOBS}} -C ports/unix DEBUG=${DEBUG} VARIANT=lvglCallers can pass --build-arg DEBUG=1 and optionally JOBS to override.
21-27: Use runtime libraries in the final image (avoid -dev packages) and clean apt listsThe runner installs -dev packages, inflating the image. Prefer runtime libs (e.g., libsdl2-2.0-0, libfreetype6, librlottie0, libav* runtime libs) and clean apt lists.
Example (verify package names for your chosen Ubuntu release):
-FROM ubuntu AS runner +FROM ubuntu:24.04 AS runner @@ -RUN apt-get update && apt-get install -y libsdl2-dev parallel libfreetype-dev librlottie-dev libavformat-dev libavcodec-dev libswscale-dev libavutil-dev +RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ + libsdl2-2.0-0 libfreetype6 librlottie0 \ + libavformat60 libavcodec60 libswscale7 libavutil58 \ + && rm -rf /var/lib/apt/lists/*If parallel isn’t required at runtime, omit it.
examples/lvgl.py (1)
12-22: Minor ordering and exit handling: create drivers before event loop; trap KeyboardInterruptCreating the display/inputs before the event loop is a common pattern, and trapping Ctrl+C avoids docker/terminal hangs.
-el = event_loop() - -WIDTH = 480 -HEIGHT = 320 - -disp_drv = lv.sdl_window_create(WIDTH, HEIGHT) -mouse = lv.sdl_mouse_create() -keyboard = lv.sdl_keyboard_create() -keyboard.set_group(lv.group_create()) +WIDTH = 480 +HEIGHT = 320 + +disp_drv = lv.sdl_window_create(WIDTH, HEIGHT) +mouse = lv.sdl_mouse_create() +keyboard = lv.sdl_keyboard_create() +keyboard.set_group(lv.group_create()) + +el = event_loop() @@ -while True: - time.sleep(100) +try: + while True: + time.sleep(1) +except KeyboardInterrupt: + pass.github/workflows/release_actions.yml (3)
10-12: Remove trailing spaces and add newline at EOFYAMLlint flags trailing spaces on Lines 10–12 and 41–42, and a missing newline at EOF.
Apply this diff:
- permissions: - contents: read - packages: write + permissions: + contents: read + packages: write @@ - permissions: - contents: write + permissions: + contents: write @@ - git push origin ${{ steps.get_version.outputs.VERSION }} + git push origin ${{ steps.get_version.outputs.VERSION }} +Also applies to: 41-42, 58-58
53-58: Make tagging idempotent to avoid failures if the tag already existsIf the version already exists as a tag, the job will fail. Guard against this.
- - name: Create and push tag - run: | - git config --global user.email "github-actions[bot]@users.noreply.github.com" - git config --global user.name "github-actions[bot]" - git tag ${{ steps.get_version.outputs.VERSION }} - git push origin ${{ steps.get_version.outputs.VERSION }} + - name: Create and push tag + run: | + set -euo pipefail + git config --global user.email "github-actions[bot]@users.noreply.github.com" + git config --global user.name "github-actions[bot]" + VERSION="${{ steps.get_version.outputs.VERSION }}" + if git rev-parse -q --verify "refs/tags/${VERSION}" >/dev/null; then + echo "Tag ${VERSION} already exists; skipping." + exit 0 + fi + git tag "${VERSION}" + git push origin "${VERSION}"
31-38: Optional: speed up builds with Docker layer cache (GHA cache backend)Caching significantly reduces build times for frequent pushes.
- name: Build and push uses: docker/build-push-action@v5 with: context: . push: true + cache-from: type=gha + cache-to: type=gha,mode=max tags: | ${{ env.IMAGE_NAME }}:latest ${{ env.IMAGE_NAME }}:${{ steps.get_version.outputs.VERSION }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
.dockerignore(1 hunks).github/FUNDING.yml(0 hunks).github/dependabot.yml(1 hunks).github/workflows/biome.yml(0 hunks).github/workflows/code_formatting.yml(1 hunks).github/workflows/code_size.yml(2 hunks).github/workflows/codespell.yml(1 hunks).github/workflows/commit_formatting.yml(0 hunks).github/workflows/docs.yml(1 hunks).github/workflows/examples.yml(0 hunks).github/workflows/mpremote.yml(0 hunks).github/workflows/mpy_format.yml(1 hunks).github/workflows/ports.yml(1 hunks).github/workflows/ports_alif.yml(0 hunks).github/workflows/ports_cc3200.yml(0 hunks).github/workflows/ports_esp32.yml(1 hunks).github/workflows/ports_esp8266.yml(0 hunks).github/workflows/ports_mimxrt.yml(0 hunks).github/workflows/ports_nrf.yml(0 hunks).github/workflows/ports_powerpc.yml(0 hunks).github/workflows/ports_qemu.yml(0 hunks).github/workflows/ports_renesas-ra.yml(0 hunks).github/workflows/ports_rp2.yml(1 hunks).github/workflows/ports_samd.yml(0 hunks).github/workflows/ports_stm32.yml(1 hunks).github/workflows/ports_unix.yml(1 hunks).github/workflows/ports_webassembly.yml(1 hunks).github/workflows/ports_windows.yml(0 hunks).github/workflows/ports_zephyr.yml(0 hunks).github/workflows/pr_hygene.yml(1 hunks).github/workflows/release_actions.yml(1 hunks).github/workflows/ruff.yml(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)Dockerfile(1 hunks)Makefile(1 hunks)examples/lvgl.py(1 hunks)ports/unix/variants/lvgl/manifest.py(1 hunks)ports/unix/variants/lvgl/mpconfigvariant.h(1 hunks)ports/unix/variants/lvgl/mpconfigvariant.mk(1 hunks)user_modules/lv_binding_micropython(1 hunks)
💤 Files with no reviewable changes (16)
- .github/workflows/commit_formatting.yml
- .github/workflows/ports_samd.yml
- .github/workflows/biome.yml
- .github/workflows/mpremote.yml
- .github/workflows/ports_qemu.yml
- .github/workflows/ports_windows.yml
- .github/workflows/ports_renesas-ra.yml
- .github/workflows/ports_nrf.yml
- .github/workflows/examples.yml
- .github/workflows/ports_powerpc.yml
- .github/FUNDING.yml
- .github/workflows/ports_alif.yml
- .github/workflows/ports_cc3200.yml
- .github/workflows/ports_zephyr.yml
- .github/workflows/ports_esp8266.yml
- .github/workflows/ports_mimxrt.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-17T19:58:45.970Z
Learnt from: minyiky
PR: minyiky/pydive-micropython#1
File: .gitmodules:77-79
Timestamp: 2025-08-17T19:58:45.970Z
Learning: User minyiky prefers not to implement recursive submodule fetching in GitHub Actions workflows until actually needed, to avoid unnecessary CI overhead for workflows that don't require the submodules.
Applied to files:
user_modules/lv_binding_micropython.gitmodules
🧬 Code Graph Analysis (1)
ports/unix/variants/lvgl/manifest.py (1)
tools/manifestfile.py (1)
include(337-401)
🪛 YAMLlint (1.37.1)
.github/workflows/codespell.yml
[error] 3-3: trailing spaces
(trailing-spaces)
[error] 5-5: trailing spaces
(trailing-spaces)
.github/workflows/release_actions.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 58-58: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/code_formatting.yml
[error] 5-5: trailing spaces
(trailing-spaces)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 4-4: Target "VERSION" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (10)
.github/dependabot.yml (1)
7-7: LGTM: reduce Dependabot noise to weekly for ActionsWeekly checks for GitHub Actions are reasonable and will cut PR churn while still keeping workflows reasonably fresh.
user_modules/lv_binding_micropython (1)
1-1: Pinned submodule commit is good for reproducibilityLocking the submodule to a specific commit ensures deterministic builds. No issues with the pointer itself.
.github/workflows/ports_esp32.yml (1)
5-6: No changes required: workflow branches filter matches default branchThe repository’s default branch is
master, so the push-trigger filter is correctly configured..github/workflows/ports_stm32.yml (1)
5-6: Branches filter matches default branch
Verified that the repository’s default branch ismaster, so the workflow’s branch filter aligns and no changes are needed..github/workflows/ports.yml (1)
5-6: Master-only push gating validated
Confirmed the repository’s default branch ismaster, so thebranches: - masterfilter in.github/workflows/ports.ymlcorrectly captures push events. If restricting metadata builds to master is the intended policy, no further changes are needed..github/workflows/ruff.yml (1)
3-10: LGTM: Explicit triggers and correct recursive Python path filterGood move to explicit
onmapping, master-only push gating, and**/*.pyfor PRs. This resolves the earlier path-filter limitation for nested Python files..github/workflows/code_size.yml (1)
27-27: Switching to ubuntu-latest may change toolchains and skew size deltasCode-size can vary with compiler/toolchain versions. If you rely on stable size baselines, consider pinning the runner or verifying size variance after this change.
.github/workflows/ports_unix.yml (1)
5-6: Restricting push triggers to master looks goodMatches common practice and reduces unnecessary CI on feature branches.
.github/workflows/code_formatting.yml (1)
19-22: Required CI functions verified in tools/ci.sh
The functionsci_c_code_formatting_setup(line 39) andci_c_code_formatting_run(line 45) are defined intools/ci.sh, so the workflow steps will execute as expected.ports/unix/variants/lvgl/mpconfigvariant.h (1)
27-36: LGTM: sensible defaults for the LVGL variantROM feature level, inclusion of the common unix variant header, and enabling the scheduler and builtin module init look correct for LVGL on unix. Disabling sys.settrace is standard.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/code_formatting.yml (3)
3-12: Optionally mirror the paths filter for PRs to cut CI noise.Unless you want formatting to run for every PR change, add the same
pathsfilter underpull_request.on: push: branches: - master paths: - "**/*.c" - "**/*.cpp" - "**/*.h" - "**/*.hpp" - pull_request: + pull_request: + paths: + - "**/*.c" + - "**/*.cpp" + - "**/*.h" + - "**/*.hpp"
20-29: Make shell explicit and harden step execution.Explicitly set the shell to bash and enable strict flags so
sourcefailures or partial pipeline errors don’t slip through.jobs: code-formatting: runs-on: ubuntu-latest + defaults: + run: + shell: bash -euo pipefail steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 - name: Install packages run: source tools/ci.sh && ci_c_code_formatting_setup - name: Run code formatting run: source tools/ci.sh && ci_c_code_formatting_run - name: Check code formatting run: git diff --exit-code
18-21: Consider pinning the runner or validating toolchain on ubuntu-latest.If your formatter toolchain is sensitive to OS image changes, pin to a specific image (e.g.,
ubuntu-22.04) or add a quick version probe inci.sh.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/code_formatting.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/code_formatting.yml
8-8: could not parse as YAML: yaml: line 8: did not find expected alphabetic or numeric character
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/code_formatting.yml
[error] 8-8: syntax error: expected alphabetic or numeric character, but found '*'
(syntax)
⏰ 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). (20)
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: qemu_riscv64
- GitHub Check: float_clang
- GitHub Check: longlong
- GitHub Check: qemu_mips
- GitHub Check: nanbox
- GitHub Check: sanitize_address
- GitHub Check: qemu_arm
- GitHub Check: macos
- GitHub Check: settrace_stackless
- GitHub Check: gil_enabled
- GitHub Check: coverage_32bit
- GitHub Check: standard
- GitHub Check: standard_v2
- GitHub Check: build_idf (esp32_build_s3_c3)
- GitHub Check: build_idf (esp32_build_c2_c6)
- GitHub Check: build_idf (esp32_build_cmod_spiram_s2)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: minyiky <71966069+minyiky@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/code_formatting.yml (1)
10-14: Globs correctly quoted — YAML alias pitfall avoided.The quoted patterns resolve the earlier parse error from unquoted *. Looks good.
🧹 Nitpick comments (1)
.github/workflows/code_formatting.yml (1)
23-23: runs-on: ubuntu-latest is fine; consider pinning for reproducibility.Pin to ubuntu-24.04 (or a SHA-pinned runner image) to avoid surprise breakages when ubuntu-latest moves.
- runs-on: ubuntu-latest + runs-on: ubuntu-24.04
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/code_formatting.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/code_formatting.yml
3-3: could not parse as YAML: yaml: line 3: did not find expected key
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/code_formatting.yml
[error] 7-7: syntax error: expected , but found ''
(syntax)
⏰ 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). (19)
- GitHub Check: build_idf (esp32_build_c2_c6)
- GitHub Check: qemu_arm
- GitHub Check: qemu_mips
- GitHub Check: macos
- GitHub Check: sanitize_undefined
- GitHub Check: sanitize_address
- GitHub Check: gil_enabled
- GitHub Check: settrace_stackless
- GitHub Check: stackless_clang
- GitHub Check: longlong
- GitHub Check: nanbox
- GitHub Check: coverage_32bit
- GitHub Check: qemu_riscv64
- GitHub Check: standard
- GitHub Check: standard_v2
- GitHub Check: build_stm32 (stm32_misc_build)
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: build_stm32 (stm32_pyb_build)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/code_formatting.yml (1)
24-36: Normalise indentation under jobs (yamllint warnings).Indent list items under
steps:and keep 2-space nesting throughout to avoid lint noise.jobs: code-formatting: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 - - name: Install packages - run: source tools/ci.sh && ci_c_code_formatting_setup - - name: Run code formatting - run: source tools/ci.sh && ci_c_code_formatting_run - - name: Check code formatting - run: git diff --exit-code + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + - name: Install packages + run: source tools/ci.sh && ci_c_code_formatting_setup + - name: Run code formatting + run: source tools/ci.sh && ci_c_code_formatting_run + - name: Check code formatting + run: git diff --exit-code
♻️ Duplicate comments (1)
.github/workflows/code_formatting.yml (1)
8-11: Globs correctly quoted — resolves YAML alias trap.Good fix; the patterns are now strings instead of YAML aliases.
Also applies to: 14-17
🧹 Nitpick comments (2)
.github/workflows/code_formatting.yml (2)
20-23: Align concurrency block indentation.Minor but keeps YAML consistent and silences linters.
-concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
26-26: Runner choice is fine; consider pinning or documenting.
ubuntu-latestis OK. Optionally pin to a specific LTS (e.g.,ubuntu-24.04) to avoid surprise image changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/code_formatting.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/code_formatting.yml
11-11: could not parse as YAML: yaml: line 11: did not find expected key
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/code_formatting.yml
[warning] 5-5: wrong indentation: expected 6 but found 5
(indentation)
[warning] 6-6: wrong indentation: expected 8 but found 7
(indentation)
[warning] 8-8: wrong indentation: expected 8 but found 7
(indentation)
[error] 12-12: syntax error: expected , but found ''
(syntax)
[warning] 13-13: wrong indentation: expected 5 but found 6
(indentation)
[warning] 14-14: wrong indentation: expected 9 but found 7
(indentation)
[warning] 21-21: wrong indentation: expected 3 but found 2
(indentation)
[warning] 25-25: wrong indentation: expected 3 but found 2
(indentation)
[warning] 26-26: wrong indentation: expected 5 but found 4
(indentation)
⏰ 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). (20)
- GitHub Check: build
- GitHub Check: sanitize_undefined
- GitHub Check: qemu_arm
- GitHub Check: qemu_riscv64
- GitHub Check: stackless_clang
- GitHub Check: longlong
- GitHub Check: gil_enabled
- GitHub Check: standard_v2
- GitHub Check: standard
- GitHub Check: macos
- GitHub Check: settrace_stackless
- GitHub Check: coverage_32bit
- GitHub Check: qemu_mips
- GitHub Check: nanbox
- GitHub Check: float_clang
- GitHub Check: build_stm32 (stm32_misc_build)
- GitHub Check: build_stm32 (stm32_nucleo_build)
- GitHub Check: build_stm32 (stm32_pyb_build)
- GitHub Check: build
- GitHub Check: build
I have added in the `lv_micropython_bindings` as a new submodule under `user_modules` to allow firmware built this way to be used in the unix simulator. Additionally I have updated some actions with the following changes - Removed unneeded actions - Added PR Hygiene actions - Added a container build action - Added auto tagging of the repo (may change later to follow mp versioning)
1 participant
Summary
I have added in the
lv_micropython_bindingsas a new submodule underuser_modulesto allow firmware built this way to be used in the unix simulator.Additionally I have updated some actions with the following changes