Skip to content

fix(cmake): key the GUI merge on the GUI target, not on TouchGFX - #283

Open
tobymurray wants to merge 1 commit into
UNAWatch:mainfrom
tobymurray:fix/customgui-merge-gate
Open

fix(cmake): key the GUI merge on the GUI target, not on TouchGFX#283
tobymurray wants to merge 1 commit into
UNAWatch:mainfrom
tobymurray:fix/customgui-merge-gate

Conversation

@tobymurray

@tobymurray tobymurray commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Rationale: moving forward with CustomGUI, which is not represented in this repo presently

una_app_build_app() lists what the merge step must wait for, and adds the GUI ELF only when TOUCHGFX_PATH is defined. That asks whether the app uses TouchGFX. What it needs to know is whether una_app_build_gui() built a GUI ELF.

The two matched while TouchGFX was the only way to draw one. Since cb1ae1f the SDK also ships the CustomGUI entry point, which needs only a Gui class taking the kernel and a run(), and leaves the app to own the message loop. An app using it builds a GUI ELF and sets no TOUCHGFX_PATH, so the merge target never depends on that ELF: the generated makefile lists Service.elf alone.

The ELF is still built, because add_executable puts it in all. What is missing is the ordering, so app_merging.py races the post-build step that packs Tmp/*.gui. "make App" fails outright with "Missing .gui file", which is required for every type but Glance, and a full parallel make succeeds or fails depending on scheduling order. Both outcomes were observed on one tree.

Glances are why that file is optional, and they are unaffected: a glance app has no GUI ELF by design. Its service asks the kernel for the glance geometry, sends a list of SDK::Glance controls with RequestGlanceUpdate, and the kernel draws them, so there is no app framebuffer and no GUI process to wait for.

if(TARGET) asks CMake the question directly. TOUCHGFX_PATH stays in the condition for apps that call una_app_build_app() before una_app_build_gui(), where the target does not exist yet and add_custom_target resolves DEPENDS at generate time.

Summary by CodeRabbit

  • Bug Fixes
    • Improved CustomGUI build support by ensuring the final application correctly includes the GUI component when available.
    • Preserved existing behavior for configurations set before the target is created.

una_app_build_app() lists what the merge step must wait for, and adds the GUI ELF
only when TOUCHGFX_PATH is defined. That asks whether the app uses TouchGFX. What
it needs to know is whether una_app_build_gui() built a GUI ELF.

The two matched while TouchGFX was the only way to draw one. Since cb1ae1f
(2025-09-29) the SDK also ships the CustomGUI entry point, which needs only a Gui
class taking the kernel and a run(), and leaves the app to own the message loop.
An app using it builds a GUI ELF and sets no TOUCHGFX_PATH, so the merge target
never depends on that ELF: the generated makefile lists Service.elf alone.

The ELF is still built, because add_executable puts it in all. What is missing is
the ordering, so app_merging.py races the post-build step that packs Tmp/*.gui.
"make <App>App" fails outright with "Missing .gui file", which is required for
every type but Glance, and a full parallel make succeeds or fails depending on
scheduling order. Both outcomes were observed on one tree.

Glances are why that file is optional, and they are unaffected: a glance app has
no GUI ELF by design. Its service asks the kernel for the glance geometry, sends
a list of SDK::Glance controls with RequestGlanceUpdate, and the kernel draws
them, so there is no app framebuffer and no GUI process to wait for.

if(TARGET) asks CMake the question directly. TOUCHGFX_PATH stays in the condition
for apps that call una_app_build_app() before una_app_build_gui(), where the
target does not exist yet and add_custom_target resolves DEPENDS at generate time.

Verified in the CI image with a CustomGUI app that names the SDK entry point by
path and sets none of TOUCHGFX_PATH, TOUCHGFX_LIBS or GUI_*: the merge target
gains GUI.elf as a prerequisite and "make <App>App" merges, where on upstream/main
the prerequisite is absent and the same command fails. Workout's .uapp is
byte-identical across the change (404484 bytes).
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d90f9f5-32e5-4bf1-8635-3bfbe860957a

📥 Commits

Reviewing files that changed from the base of the PR and between edf2fee and b5dd200.

📒 Files selected for processing (1)
  • cmake/una-app.cmake

📝 Walkthrough

Walkthrough

Final app merging now detects an existing GUI ELF target or a defined TOUCHGFX_PATH before adding the GUI dependency. This supports CustomGUI builds while preserving pre-target configuration behavior.

Changes

GUI dependency handling

Layer / File(s) Summary
App merge dependency detection
cmake/una-app.cmake
The merge logic adds the GUI ELF dependency when ${APP_NAME}GUI.elf already exists or TOUCHGFX_PATH is defined.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to b5dd2

This change corrects GUI build ordering for CustomGUI applications without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review.

Suggested reviewers: rryles, noamsto

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the CMake fix that makes GUI merging depend on the GUI target instead of only on TouchGFX.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant