Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an initial Android build pipeline for an SDL-based viewer MVP, integrating an Android Gradle project + native CMake build, plus Docker builders and a GitHub Actions workflow to produce a debug APK.
Changes:
- Add Android Gradle app scaffold using GameActivity and a native
dcma_sdl_viewershared library entrypoint. - Add Android cross-compilation/build tooling via Docker (build base + builder) and a CI workflow to assemble and upload the APK.
- Minor docstring spelling fix in an existing operation.
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Operations/CountObjects.cc | Fix typo in operation description string. |
| docker/builders/android/implementation_android.sh | Android dependency cross-compile + Gradle APK assembly script. |
| docker/builders/android/Dockerfile | Docker image wrapper to run the Android builder script. |
| docker/build_bases/android/implementation_android_base.sh | Debian-based base image provisioning for Android SDK/NDK. |
| docker/build_bases/android/Dockerfile | Docker base image definition for Android builds. |
| android/settings.gradle | Define Android project repositories and include :app. |
| android/gradlew | Minimal Gradle wrapper launcher script. |
| android/gradle/wrapper/gradle-wrapper.properties | Configure Gradle wrapper distribution (8.4). |
| android/gradle/wrapper/gradle-wrapper.jar | Add Gradle wrapper JAR. |
| android/gradle.properties | Set JVM args + enable AndroidX. |
| android/build.gradle | Configure Android Gradle plugin version. |
| android/app/build.gradle | Define Android app module, externalNativeBuild CMake integration, and GameActivity dependency. |
| android/app/src/main/AndroidManifest.xml | Declare app, activity, GLES requirement, and storage/media permissions. |
| android/app/src/main/java/ca/halclark/dicomautomaton/MainActivity.java | Java entrypoint extending GameActivity. |
| android/app/src/main/cpp/dcma_sdl_viewer_main.cc | Native SDL_main() entrypoint to launch SDL_Viewer. |
| android/app/src/main/cpp/android_shims/GL/glew.h | Provide Android GLES shim for GLEW include/entrypoints. |
| android/app/src/main/cpp/CMakeLists.txt | Native library build definition pulling in a subset of DICOMautomaton sources. |
| android/app/src/main/res/** | Minimal resources (strings + launcher icon). |
| android/app/proguard-rules.pro | Keep rules for GameActivity/SDL JNI classes. |
| .gitignore | Ignore Android build outputs and IDE metadata. |
| .github/workflows/android.yml | CI workflow to cross-compile deps, build APK, and upload artifacts. |
| --user-config=/tmp/boost_user_config_${abi}.jam \ | ||
| toolset="clang-android_${ndk_arch}" \ | ||
| target-os=android \ | ||
| address-model=64 \ | ||
| link=static \ | ||
| threading=multi \ | ||
| --prefix="${prefix}" \ | ||
| --with-filesystem \ | ||
| --with-iostreams \ | ||
| --with-serialization \ | ||
| --with-thread \ | ||
| --with-system \ | ||
| -j "$(nproc)" \ | ||
| install 2>&1 | tail -30 | ||
| done |
There was a problem hiding this comment.
./b2 ... install 2>&1 | tail -30 can mask Boost build failures: without set -o pipefail, the pipeline’s exit status is from tail, so a non-zero b2 exit code won’t fail the step. Either enable pipefail for this script block or avoid piping b2 output (or explicitly exit ${PIPESTATUS[0]} after the pipeline) so CI correctly fails when Boost doesn’t build.
|
|
||
| ./gradlew assembleDebug \ | ||
| --no-daemon \ | ||
| --stacktrace \ | ||
| 2>&1 | tee /tmp/gradle_build.log |
There was a problem hiding this comment.
The Gradle invocation is piped to tee, but the script only uses set -e (no pipefail). In bash this means a failing ./gradlew can be masked by a successful tee, causing the Docker build to appear successful while producing no/invalid APKs. Enable set -o pipefail (or capture and check ${PIPESTATUS[0]}) so Gradle failures fail the build.
| ./gradlew assembleDebug \ | |
| --no-daemon \ | |
| --stacktrace \ | |
| 2>&1 | tee /tmp/gradle_build.log | |
| set +e | |
| ./gradlew assembleDebug \ | |
| --no-daemon \ | |
| --stacktrace \ | |
| 2>&1 | tee /tmp/gradle_build.log | |
| gradle_status=${PIPESTATUS[0]} | |
| set -e | |
| if [ "${gradle_status}" -ne 0 ]; then | |
| echo "Gradle build failed with exit code ${gradle_status}" >&2 | |
| exit "${gradle_status}" | |
| fi |
| # Accept all SDK licenses non-interactively and warn if acceptance appears to fail. | ||
| yes | sdkmanager --licenses 2>&1 | grep -q "All SDK package licenses accepted" \ | ||
| || echo "Warning: Android SDK license acceptance may have failed" |
There was a problem hiding this comment.
The license acceptance pipeline (yes | sdkmanager ... | grep ... || echo ...) can hide sdkmanager failures because set -e without pipefail only checks the exit code of the last command in the pipeline. Consider adding set -o pipefail (or running sdkmanager --licenses separately and checking its exit status) so SDK install failures don’t slip through silently.
| "${DCMA_SRC_DIR}/File_Loader.cc" | ||
| "${DCMA_SRC_DIR}/Script_Loader.cc" | ||
| "${DCMA_SRC_DIR}/Standard_Scripts.cc" | ||
| "${DCMA_SRC_DIR}/Standard_Guides.cc" | ||
| "${DCMA_SRC_DIR}/Colour_Maps.cc" |
There was a problem hiding this comment.
Standard_Scripts.cc and Standard_Guides.cc are listed as sources, but the repository only contains Standard_Scripts.cc.in / Standard_Guides.cc.in templates (no generated .cc files). As-is, CMake configuration will fail because the source files don’t exist. Either generate these files in this Android build (e.g., via configure_file() with empty inlines for MVP) or remove/replace these dependencies for the Android target.
| set(SDL_VIEWER_SOURCES | ||
| "${DCMA_SRC_DIR}/Operations/SDL_Viewer.cc" |
There was a problem hiding this comment.
This Android build compiles the desktop src/Operations/SDL_Viewer.cc against OpenGL ES (via the GLEW shim), but SDL_Viewer.cc uses desktop-only OpenGL APIs such as glPolygonMode() (not available in GLES). This will cause compilation failures for the Android target unless the viewer code is GLES-ported/guarded. Either add #ifdef __ANDROID__ fallbacks in SDL_Viewer.cc (e.g., disable wireframe mode) or exclude the unsupported code paths for the Android build.
| set(SDL_VIEWER_SOURCES | |
| "${DCMA_SRC_DIR}/Operations/SDL_Viewer.cc" | |
| # Note: The desktop SDL_Viewer.cc uses desktop-only OpenGL APIs (e.g., glPolygonMode) | |
| # which are not available in the Android GLES build. To avoid compilation errors | |
| # when linking against GLES/EGL, we intentionally exclude SDL_Viewer.cc here. | |
| set(SDL_VIEWER_SOURCES |
| <!-- Storage permissions for loading DICOM files. | ||
| READ_EXTERNAL_STORAGE covers API ≤ 32 (Android 12 and below). | ||
| The granular READ_MEDIA_* permissions cover API 33+ (Android 13+). --> | ||
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" | ||
| android:maxSdkVersion="32" /> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" /> | ||
| <uses-permission android:name="android.permission.READ_MEDIA_AUDIO" /> |
There was a problem hiding this comment.
The comment says these permissions enable “loading DICOM files”, but on Android 13+ the READ_MEDIA_* permissions only grant access to media collections (images/video/audio), not arbitrary files like DICOM in Downloads or other directories. As a result, file-based DICOM loading will fail on API 33+ even when permissions are granted. For DICOM import, prefer the Storage Access Framework (ACTION_OPEN_DOCUMENT / document URIs) or another scoped-storage compatible approach rather than READ_MEDIA_*.
| <!-- Storage permissions for loading DICOM files. | |
| READ_EXTERNAL_STORAGE covers API ≤ 32 (Android 12 and below). | |
| The granular READ_MEDIA_* permissions cover API 33+ (Android 13+). --> | |
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" | |
| android:maxSdkVersion="32" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_AUDIO" /> | |
| <!-- Storage-related permissions. | |
| - READ_EXTERNAL_STORAGE covers API ≤ 32 (Android 12 and below) for legacy | |
| external storage access. | |
| - On API 33+ (Android 13+), DICOM and other non-media files must be | |
| accessed via the Storage Access Framework (e.g. ACTION_OPEN_DOCUMENT) | |
| or other scoped-storage compatible mechanisms; the granular | |
| READ_MEDIA_* permissions only grant access to media collections | |
| (images/video/audio), not arbitrary files such as DICOM in Downloads. | |
| - The READ_MEDIA_* permissions below are therefore for media content | |
| only (e.g. image/video previews), not DICOM file import. --> | |
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" | |
| android:maxSdkVersion="32" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> | |
| <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" /> |
| CMDLINE_TOOLS_VERSION="11076708" | ||
| CMDLINE_TOOLS_URL="https://dl.google.com/android/repository/commandlinetools-linux-${CMDLINE_TOOLS_VERSION}_latest.zip" | ||
|
|
||
| wget --quiet -O /tmp/cmdline-tools.zip "${CMDLINE_TOOLS_URL}" | ||
| unzip -q /tmp/cmdline-tools.zip -d /tmp/cmdline-tools-extract | ||
| mv /tmp/cmdline-tools-extract/cmdline-tools "${ANDROID_HOME}/cmdline-tools/latest" | ||
| rm -rf /tmp/cmdline-tools.zip /tmp/cmdline-tools-extract |
There was a problem hiding this comment.
The script downloads the Android command-line tools ZIP via wget from CMDLINE_TOOLS_URL and immediately unzips/installs it without any integrity verification. If an attacker can tamper with this download path (e.g., via a compromised CDN, DNS, or mirror), they can inject malicious binaries into the base image and all downstream Android builds. Add a pinned checksum or signature verification step for the ZIP (similar to the Boost SHA256 check) before unzipping and using the tools.
No description provided.