Switch to using prebuilt toolchain containers#33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the LessUI build system by switching from locally-built toolchain containers to pre-built containers hosted on GitHub Container Registry (ghcr.io/lessui-hq). This eliminates the need to clone toolchain repositories and build Docker images locally, significantly simplifying the setup process and reducing build times.
Key Changes
- Replaced git cloning and local Docker building with
docker pullfrom GitHub Container Registry - Added
force-pulltarget to easily update to the latest toolchain versions - Simplified the clean process to only remove local tags and marker files
- Updated documentation to reflect the new pre-built container workflow
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Default target: ensure toolchain is built, then launch interactive shell | ||
| all: $(INIT_IF_NECESSARY) | ||
| docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash |
There was a problem hiding this comment.
[nitpick] The all target at line 41 lacks the @ prefix for the docker run command. This causes the command to be echoed to the console before execution, which is inconsistent with the cleaner output style used in the $(INIT_IF_NECESSARY) target (lines 45-61) where informational messages use @echo. Consider adding @ to line 41 for consistency and cleaner output.
| docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash | |
| @docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash |
| # Build all workspace components in Docker (non-interactive) | ||
| build: $(INIT_IF_NECESSARY) | ||
| docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash -c '. ~/.bashrc && cd /root/workspace && make' No newline at end of file | ||
| docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash -c '. ~/.bashrc && cd /root/workspace && make' |
There was a problem hiding this comment.
[nitpick] The build target at line 70 lacks the @ prefix for the docker run command. This causes the command to be echoed to the console before execution, which is inconsistent with the cleaner output style used in the $(INIT_IF_NECESSARY) target (lines 45-61) where informational messages use @echo. Consider adding @ to line 70 for consistency and cleaner output.
| docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash -c '. ~/.bashrc && cd /root/workspace && make' | |
| @docker run --rm -e LOG_FLAGS="$(LOG_FLAGS)" -v $(HOST_WORKSPACE):$(GUEST_WORKSPACE) $(PLATFORM)-toolchain /bin/bash -c '. ~/.bashrc && cd /root/workspace && make' |
| # Clean Docker image | ||
| clean: | ||
| cd toolchains/$(PLATFORM)-toolchain && make clean | ||
| @docker rmi $(PLATFORM)-toolchain 2>/dev/null || true |
There was a problem hiding this comment.
[nitpick] The clean target only removes the local tag $(PLATFORM)-toolchain but doesn't remove the pulled container image $(CONTAINER_IMAGE) from Docker. This means docker pull won't fetch updates after a clean unless force-pull is used. Consider adding @docker rmi $(CONTAINER_IMAGE) 2>/dev/null || true to ensure a fresh pull on the next build, or document that force-pull should be used to get updates after cleaning.
| @docker rmi $(PLATFORM)-toolchain 2>/dev/null || true | |
| @docker rmi $(PLATFORM)-toolchain 2>/dev/null || true | |
| @docker rmi $(CONTAINER_IMAGE) 2>/dev/null || true |
| # make -f makefile.toolchain PLATFORM=miyoomini build # Build all | ||
| # | ||
| # Adding a new platform: | ||
| # 1. Fork union-<platform>-toolchain repo to lessui-hq org |
There was a problem hiding this comment.
Missing article 'the' before 'union--toolchain'. The sentence should read "Fork the union--toolchain repo to lessui-hq org" for better grammar.
| # 1. Fork union-<platform>-toolchain repo to lessui-hq org | |
| # 1. Fork the union-<platform>-toolchain repo to lessui-hq org |
| echo "❌ Container not found: $(CONTAINER_IMAGE)" && \ | ||
| echo "" && \ | ||
| echo "To add support for $(PLATFORM):" && \ | ||
| echo " 1. Fork union-$(PLATFORM)-toolchain to lessui-hq org" && \ |
There was a problem hiding this comment.
Missing article 'the' before 'union-$(PLATFORM)-toolchain'. The sentence should read "Fork the union-$(PLATFORM)-toolchain to lessui-hq org" for better grammar and consistency with the corrected instruction at line 14.
| echo " 1. Fork union-$(PLATFORM)-toolchain to lessui-hq org" && \ | |
| echo " 1. Fork the union-$(PLATFORM)-toolchain to lessui-hq org" && \ |
| # Force pull latest pre-built container (useful for updates) | ||
| force-pull: | ||
| @echo "Updating $(PLATFORM)-toolchain from $(CONTAINER_REGISTRY)/$(CONTAINER_ORG)..." | ||
| @docker pull $(CONTAINER_IMAGE) |
There was a problem hiding this comment.
The force-pull target doesn't handle docker pull failures. If the pull fails (e.g., network issue, authentication problem), the subsequent docker tag at line 76 will fail since the image won't exist, but the marker file will still be created at line 77, incorrectly indicating the toolchain is ready. Consider adding error handling similar to lines 48-57, or using set -e in the recipe to stop on first error.
| @docker pull $(CONTAINER_IMAGE) | |
| @docker pull $(CONTAINER_IMAGE) || \ | |
| (echo "" && \ | |
| echo "❌ Failed to pull container: $(CONTAINER_IMAGE)" && \ | |
| echo "" && \ | |
| echo "Check your network connection, authentication, or image existence." && \ | |
| echo "" && \ | |
| exit 1) |
No description provided.