Extract compiler path construction logic to eliminate duplication in Android build script#89
Extract compiler path construction logic to eliminate duplication in Android build script#89
Conversation
…Android build script Co-authored-by: Gameaday <6642855+Gameaday@users.noreply.github.com>
…and error handling Co-authored-by: Gameaday <6642855+Gameaday@users.noreply.github.com>
…cross scripts Co-authored-by: Gameaday <6642855+Gameaday@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Android build script to eliminate code duplication by extracting compiler path construction logic into reusable functions, and introduces a common utilities module for shared functionality across all build scripts.
Key changes:
- Extracted target-to-compiler mapping logic into three helper functions in
build-android.sh - Created
scripts/common.shwith shared utilities (colors, error handling, validation functions) - Updated 5 build scripts to use common utilities, eliminating duplicated code
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/build-android.sh | Extracted compiler path logic to functions, replaced hardcoded exports with dynamic generation |
| scripts/common.sh | New shared utilities module with color definitions and common functions |
| scripts/validate-build.sh | Updated to use common utilities for colors and validation functions |
| scripts/test-mobile.sh | Updated to use common utilities, removed duplicated color definitions |
| scripts/build-mobile.sh | Updated to use common utilities for consistent messaging and validation |
| scripts/build-benchmark.sh | Updated to use common utilities, removed duplicated color definitions |
Comments suppressed due to low confidence (4)
scripts/build-android.sh:152
- Color variables RED, YELLOW, and NC are used but no longer defined after being moved to common.sh. These should use the common utility functions like error_exit or warning instead.
echo -e "${RED}Error: Compiler not found: $compiler${NC}"
echo -e "${YELLOW}Available compilers in NDK:${NC}"
scripts/test-mobile.sh:1
- The YELLOW and NC color variables are used but no longer defined in this script. This should use the warning() or info() function from common utilities instead.
#!/bin/bash
scripts/build-mobile.sh:1
- The YELLOW and NC color variables are used but no longer defined. This should use the warning() or info() function from common.sh instead.
#!/bin/bash
scripts/build-mobile.sh:1
- The GREEN and NC color variables are used but no longer defined. This should use the success() function from common.sh instead.
#!/bin/bash
| error_exit "ANDROID_NDK_HOME environment variable is not set | ||
| Please install Android NDK and set ANDROID_NDK_HOME | ||
| Example: export ANDROID_NDK_HOME=\$ANDROID_HOME/ndk/27.3.13750724" |
There was a problem hiding this comment.
Multi-line string passed to error_exit function should use proper bash syntax. Consider using a here-document or concatenating the message with newlines to ensure proper formatting.
| error_exit "ANDROID_NDK_HOME environment variable is not set | |
| Please install Android NDK and set ANDROID_NDK_HOME | |
| Example: export ANDROID_NDK_HOME=\$ANDROID_HOME/ndk/27.3.13750724" | |
| error_exit "$(cat <<EOF | |
| ANDROID_NDK_HOME environment variable is not set | |
| Please install Android NDK and set ANDROID_NDK_HOME | |
| Example: export ANDROID_NDK_HOME=\$ANDROID_HOME/ndk/27.3.13750724 | |
| EOF | |
| )" |
| echo "==========================" | ||
|
|
||
| # Function to print status | ||
| # Function to print status (using common utilities when possible) |
There was a problem hiding this comment.
The print_status function still uses local RED and NC color variables that are no longer defined after removing the color definitions. This will cause undefined variable errors.
| echo -e "${YELLOW}⚠️ $message${NC}" | ||
| warning "⚠️ $message" | ||
| else | ||
| echo -e "${RED}❌ $message${NC}" |
There was a problem hiding this comment.
The RED and NC variables are used but no longer defined in this script after the color definitions were removed. This will result in empty variable expansion.
The
scripts/build-android.shscript contained significant code duplication where compiler path construction logic was duplicated between the environment variable exports (lines 71-74) and the compiler verification loop (lines 94-107). This created maintenance burden and violated DRY principles.Changes Made
1. Extracted Compiler Path Logic
Created three helper functions to centralize target-to-compiler mappings:
get_compiler_prefix(): Maps target names (aarch64, armv7a, etc.) to NDK compiler prefixesget_rust_target(): Maps short target names to full Rust target namesget_android_abi(): Maps target names to Android ABI directory names2. Eliminated Environment Variable Duplication
Before: 16 hardcoded export statements
After: Single loop generating all environment variables
3. Simplified Verification Logic
Replaced the duplicated case statement in the compiler verification loop with a simple function call, ensuring the same logic is used for both environment variable generation and verification.
4. Additional DRY Improvements
Created
scripts/common.shwith shared utilities to eliminate color definition duplication across all shell scripts:error_exit(),success(),info(),warning()check_project_root(),command_exists(),check_rust_target()Updated 5 scripts to use the common utilities, eliminating ~25 lines of duplicated code.
Benefits
The refactored scripts maintain 100% backward compatibility while significantly reducing code duplication.
Fixes #86.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.