Conversation
Summary of ChangesHello @vojta-horanek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly updates and expands the Unix configuration for development environments. It focuses on modernizing the Android development stack by upgrading the Android SDK to API 35 and introducing a dedicated Docker image for Android emulators integrated with Maestro for UI automation. Additionally, it brings the Kotlin compiler and Gradle build tool to their latest stable versions, ensuring a robust and up-to-date foundation for building and testing applications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates various Unix configurations, primarily focusing on Docker images for Android development. It introduces a new Dockerfile for an Android emulator with Maestro, updates versions of Kotlin and Gradle, and adds newer Android SDK components. My review includes suggestions to remove temporary debugging commands, improve Dockerfile efficiency by consolidating commands and cleaning up build artifacts, fixing a critical bug in a shell script, and removing dead or redundant code for better maintainability.
| echo "${G_LOG_E} Failed to store x11vnc password." | ||
| exit 1 | ||
| fi | ||
| passwordArgument=-"-rfbauth ${passwordFilePath}" |
There was a problem hiding this comment.
There's a critical bug here. The extra hyphen in passwordArgument=-'-rfbauth ...' will cause the argument to be passed incorrectly to x11vnc, which will likely prevent the VNC server from starting correctly with password authentication.
| passwordArgument=-"-rfbauth ${passwordFilePath}" | |
| passwordArgument="-rfbauth ${passwordFilePath}" |
| RUN apt-get update && apt-get install -y \ | ||
| fonts-noto \ | ||
| fonts-noto-mono \ | ||
| fonts-noto-color-emoji && fc-cache -f -v |
There was a problem hiding this comment.
This RUN instruction has two issues:
- It runs
apt-get updateagain, which is inefficient. It's better to combine package installations into a singleRUNlayer with oneapt-get updatecall (like the one on line 18). - It's missing
rm -rf /var/lib/apt/lists/*afterapt-get install, which will unnecessarily increase the Docker image size.
I recommend moving the font installation to the RUN block at line 18 and removing this block.
|
|
||
| FROM ${JAVA_IMAGE} | ||
|
|
||
| RUN echo ${JAVA_IMAGE} |
There was a problem hiding this comment.
This echo command appears to be a leftover from debugging and should be removed from the final Dockerfile to keep it clean.
| RUN echo y | sdkmanager "tools" && \ | ||
| echo y | sdkmanager "platform-tools" && \ | ||
| echo y | sdkmanager "build-tools;35.0.0" && \ | ||
| echo y | sdkmanager "build-tools;34.0.0" && \ | ||
| echo y | sdkmanager "build-tools;33.0.2" && \ | ||
| echo y | sdkmanager "build-tools;33.0.1" && \ | ||
| echo y | sdkmanager "build-tools;32.1.0-rc1" && \ | ||
| echo y | sdkmanager "build-tools;30.0.3" |
There was a problem hiding this comment.
To improve efficiency and readability, these chained sdkmanager commands can be consolidated into a single call. The sdkmanager tool supports installing multiple packages at once, which reduces the number of processes spawned during the build.
RUN echo y | sdkmanager "tools" "platform-tools" "build-tools;35.0.0" "build-tools;34.0.0" "build-tools;33.0.2" "build-tools;33.0.1" "build-tools;32.1.0-rc1" "build-tools;30.0.3"
| RUN echo y | sdkmanager "platforms;android-35" && \ | ||
| echo y | sdkmanager "platforms;android-34" && \ | ||
| echo y | sdkmanager "platforms;android-33" && \ | ||
| echo y | sdkmanager "platforms;android-32" && \ | ||
| echo y | sdkmanager "platforms;android-31" |
There was a problem hiding this comment.
Similar to the previous comment, these sdkmanager calls can be combined into a single, more efficient command.
RUN echo y | sdkmanager "platforms;android-35" "platforms;android-34" "platforms;android-33" "platforms;android-32" "platforms;android-31"
| if [ $? -ne 0 ]; then | ||
| echo "Error launching emulator" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
This check if [ $? -ne 0 ] will not correctly determine if the emulator failed to launch. The nohup ... & command backgrounds the process and returns an exit code of 0 almost immediately. This check is misleading and provides a false sense of error handling.
| NC='\033[0m' # No Color | ||
|
|
||
|
|
||
| emulator_name=${EMULATOR_NAME} |
There was a problem hiding this comment.
This variable assignment is redundant. You can use the environment variable ${EMULATOR_NAME} directly in the script (e.g., on line 39) to simplify the code.
| function check_hardware_acceleration() { | ||
| if [[ "$HW_ACCEL_OVERRIDE" != "" ]]; then | ||
| hw_accel_flag="$HW_ACCEL_OVERRIDE" | ||
| else | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| # macOS-specific hardware acceleration check | ||
| HW_ACCEL_SUPPORT=$(sysctl -a | grep -E -c '(vmx|svm)') | ||
| else | ||
| # generic Linux hardware acceleration check | ||
| HW_ACCEL_SUPPORT=$(grep -E -c '(vmx|svm)' /proc/cpuinfo) | ||
| fi | ||
|
|
||
| if [[ $HW_ACCEL_SUPPORT == 0 ]]; then | ||
| hw_accel_flag="-accel off" | ||
| else | ||
| hw_accel_flag="-accel on" | ||
| fi | ||
| fi | ||
|
|
||
| echo "$hw_accel_flag" | ||
| } | ||
|
|
||
| # NOT IN USE ATM. | ||
| hw_accel_flag=$(check_hardware_acceleration) |
There was a problem hiding this comment.
This function check_hardware_acceleration and its call are marked as "NOT IN USE ATM.". This dead code should be removed to improve readability and maintainability.
| if [ $? -ne 0 ]; then | ||
| echo "Error launching emulator" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
This check if [ $? -ne 0 ] will not correctly determine if the emulator failed to launch. The nohup ... & command backgrounds the process and returns an exit code of 0 almost immediately. This check is misleading and provides a false sense of error handling.
| # Java 17 is default | ||
| ARG VERSION=17 | ||
|
|
||
| RUN echo ${VERSION} |
There was a problem hiding this comment.
This echo command appears to be a leftover from debugging and should be removed from the final Dockerfile to keep it clean.
2 participants
No description provided.