Skip to content

Conversation

@nchapman
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings November 28, 2025 17:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a standardized hook-based build system to support adding the File Server pak and refactors the existing build infrastructure. The changes consolidate duplicated makefile logic into a shared common/build.mk template and implement a consistent hook interface (setup/build/install/clean) across all paks and utils.

Key changes:

  • Introduces workspace/all/common/build.mk providing shared build patterns for native code compilation
  • Refactors utility makefiles (jq, minui-keyboard, minui-list, minui-presenter) to use hook interface and build.mk
  • Refactors pak makefiles (Input, Clock, Bootlogo, Emus) to use hook interface
  • Adds PLATFORM_ARCH environment variable to launch.sh files for architecture-specific binary paths
  • Centralizes orchestration in workspace/all/utils/makefile and workspace/all/paks/makefile

Reviewed changes

Copilot reviewed 31 out of 45 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
workspace/makefile Simplifies all/utils invocation by calling top-level makefile instead of individual subdirectories
workspace/all/utils/makefile New orchestrator implementing hook interface for all utility builds
workspace/all/utils/*/makefile Refactored to use hook interface and common/build.mk (minui-keyboard, minui-list, minui-presenter) or implement hooks directly (jq)
workspace/all/paks/makefile Enhanced orchestrator with hook interface support for both root and src/ makefiles
workspace/all/paks/*/src/makefile Refactored to use hook interface and common/build.mk (Input, Clock) or implement hooks directly (Bootlogo)
workspace/all/paks/Emus/makefile New makefile implementing setup hook to download libretro cores for both architectures
workspace/all/common/build.mk New shared build template providing standard compiler configuration, source files, and hook targets
skeleton/SYSTEM/*/paks/MinUI.pak/launch.sh Added PLATFORM_ARCH variable and updated PATH to include architecture-specific bin directories
skeleton/SYSTEM/common/bin/*/.gitkeep Added placeholder files to track empty architecture-specific directories
workspace/all/utils/skeleton/SYSTEM/common/bin/*/.gitkeep Added placeholder files to track empty architecture-specific directories
makefile Refactored setup and system targets to use hook interface; removed cores-download target (now in Emus pak)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1 @@
placeholder
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

.gitkeep files should be empty. The content 'placeholder' is unnecessary and non-standard. Git tracks directories through the presence of files, so an empty .gitkeep file achieves the same purpose.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1 @@
placeholder
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

.gitkeep files should be empty. The content 'placeholder' is unnecessary and non-standard. Git tracks directories through the presence of files, so an empty .gitkeep file achieves the same purpose.

Copilot uses AI. Check for mistakes.
# standalone utilities without the common MinUI source files.

###########################################################
# Hook interface gate - setup and clean don't need PLATFORM
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Comment is incomplete and inconsistent with the implementation. The install hook (line 52) also doesn't need PLATFORM. Update comment to: 'Hook interface gate - setup, install, and clean don't need PLATFORM' to match the pattern documented in common/build.mk.

Suggested change
# Hook interface gate - setup and clean don't need PLATFORM
# Hook interface gate - setup, install, and clean don't need PLATFORM

Copilot uses AI. Check for mistakes.
###########################################################
# Hook interface gate - setup and clean don't need PLATFORM

ifeq ($(filter setup clean,$(MAKECMDGOALS)),)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The filter should include 'install' to match the comment and the pattern established in common/build.mk (line 30). While the install hook doesn't use PLATFORM so this works, it's inconsistent with the established pattern. Change to: ifeq ($(filter setup install clean,$(MAKECMDGOALS)),) for consistency.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +41
# DESTDIR is absolute path to build/SYSTEM/common/bin
# Cores go in build/.system/cores (3 levels up from SYSTEM, then down to .system)
setup:
@CORES_DIR="$(DESTDIR)/../../../.system/cores"; \
mkdir -p "$$CORES_DIR/a7" "$$CORES_DIR/a53"; \
if [ -f cores-override/linux-cortex-a7.zip ]; then \
echo "Using local a7 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a7.zip -d "$$CORES_DIR/a7"; \
else \
echo "Downloading a7 cores (ARM32) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a7.zip -o /tmp/lessui-cores-a7.zip; \
unzip -o -j -q /tmp/lessui-cores-a7.zip -d "$$CORES_DIR/a7"; \
rm /tmp/lessui-cores-a7.zip; \
fi; \
if [ -f cores-override/linux-cortex-a53.zip ]; then \
echo "Using local a53 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a53.zip -d "$$CORES_DIR/a53"; \
else \
echo "Downloading a53 cores (ARM64) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a53.zip -o /tmp/lessui-cores-a53.zip; \
unzip -o -j -q /tmp/lessui-cores-a53.zip -d "$$CORES_DIR/a53"; \
rm /tmp/lessui-cores-a53.zip; \
fi; \
echo "Cores deployed successfully:"; \
echo " a7: $$(ls "$$CORES_DIR/a7"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM32)"; \
echo " a53: $$(ls "$$CORES_DIR/a53"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM64)"
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The path manipulation using /../../../.system/cores is fragile and depends on DESTDIR having a specific structure. Consider documenting the expected DESTDIR format in a comment, or better yet, compute this path in the calling makefile and pass it as a separate variable like CORES_DIR. This would make the relationship explicit and less error-prone.

Suggested change
# DESTDIR is absolute path to build/SYSTEM/common/bin
# Cores go in build/.system/cores (3 levels up from SYSTEM, then down to .system)
setup:
@CORES_DIR="$(DESTDIR)/../../../.system/cores"; \
mkdir -p "$$CORES_DIR/a7" "$$CORES_DIR/a53"; \
if [ -f cores-override/linux-cortex-a7.zip ]; then \
echo "Using local a7 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a7.zip -d "$$CORES_DIR/a7"; \
else \
echo "Downloading a7 cores (ARM32) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a7.zip -o /tmp/lessui-cores-a7.zip; \
unzip -o -j -q /tmp/lessui-cores-a7.zip -d "$$CORES_DIR/a7"; \
rm /tmp/lessui-cores-a7.zip; \
fi; \
if [ -f cores-override/linux-cortex-a53.zip ]; then \
echo "Using local a53 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a53.zip -d "$$CORES_DIR/a53"; \
else \
echo "Downloading a53 cores (ARM64) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a53.zip -o /tmp/lessui-cores-a53.zip; \
unzip -o -j -q /tmp/lessui-cores-a53.zip -d "$$CORES_DIR/a53"; \
rm /tmp/lessui-cores-a53.zip; \
fi; \
echo "Cores deployed successfully:"; \
echo " a7: $$(ls "$$CORES_DIR/a7"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM32)"; \
echo " a53: $$(ls "$$CORES_DIR/a53"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM64)"
# CORES_DIR must be set by the calling makefile to the absolute path for cores (e.g. build/.system/cores)
# DESTDIR is absolute path to build/SYSTEM/common/bin (not used for path computation here)
setup:
mkdir -p "$(CORES_DIR)/a7" "$(CORES_DIR)/a53"; \
if [ -f cores-override/linux-cortex-a7.zip ]; then \
echo "Using local a7 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a7.zip -d "$(CORES_DIR)/a7"; \
else \
echo "Downloading a7 cores (ARM32) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a7.zip -o /tmp/lessui-cores-a7.zip; \
unzip -o -j -q /tmp/lessui-cores-a7.zip -d "$(CORES_DIR)/a7"; \
rm /tmp/lessui-cores-a7.zip; \
fi; \
if [ -f cores-override/linux-cortex-a53.zip ]; then \
echo "Using local a53 cores from cores-override/..."; \
unzip -o -j -q cores-override/linux-cortex-a53.zip -d "$(CORES_DIR)/a53"; \
else \
echo "Downloading a53 cores (ARM64) from minarch-cores $(MINARCH_CORES_VERSION)..."; \
curl -sL $(CORES_BASE)/linux-cortex-a53.zip -o /tmp/lessui-cores-a53.zip; \
unzip -o -j -q /tmp/lessui-cores-a53.zip -d "$(CORES_DIR)/a53"; \
rm /tmp/lessui-cores-a53.zip; \
fi; \
echo "Cores deployed successfully:"; \
echo " a7: $$(ls "$(CORES_DIR)/a7"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM32)"; \
echo " a53: $$(ls "$(CORES_DIR)/a53"/*.so 2>/dev/null | wc -l | tr -d ' ') cores (ARM64)"

Copilot uses AI. Check for mistakes.
cd ./all/utils/ && make
cd ./all/syncsettings/ && make
cd ./$(PLATFORM) && make

Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The trailing whitespace on line 146 should be removed. While functionally harmless, it's generally considered a best practice to avoid trailing whitespace in makefiles.

Copilot uses AI. Check for mistakes.
@nchapman nchapman merged commit adb1445 into develop Nov 28, 2025
4 checks passed
@nchapman nchapman deleted the feature/web-server branch November 28, 2025 18:35
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.

2 participants