-
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Android Gradle Build Tools, Java, compileSdk #70, #72 #76
Conversation
WalkthroughThis pull request updates multiple build configuration files and wrapper scripts. The CI workflow now tests against both Java 17 and 21. The Android Gradle plugin version in the build file has been bumped up to 8.5.2, and the Gradle wrapper now uses version 8.12 with URL validation enabled. Additionally, both the Unix ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant G as gradlew Script
U->>G: Invoke gradlew
G->>G: Set APP_HOME (suppress cd output)
G->>G: Check for Java using "command -v java"
alt Java found
G->>G: Execute Java command with options
else Java not found
G->>U: Output error message
end
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
gradlew (1)
67-81
: Watch out for symlink parsing edge cases.The script uses
ls -ld
to retrieve symlink targets and extracts the link path from the output. This behavior can sometimes break if filenames contain unusual characters (e.g., spaces or newlines). Also, the format ofls -ld
output may differ across some platforms. Consider usingreadlink
(if available) instead of relying on parsingls -ld
output to reduce potential edge cases.gradlew.bat (1)
37-37
: Double-check memory bounds for large builds.Setting
DEFAULT_JVM_OPTS="-Xmx64m" "-Xms64m"
provides a tight memory limit. This might be insufficient for larger or more complex projects, potentially causing out-of-memory errors. Evaluate whether a higher -Xmx value is needed if your builds are more resource-intensive..github/workflows/gradle.yml (1)
17-18
: Action Versions Outdated:
Static analysis has flagged that bothactions/checkout@v3
andactions/setup-java@v3
might be outdated. Please consider updating these actions (for example, toactions/checkout@v4
and the latest stable release ofactions/setup-java
) to ensure improved performance, compatibility, and security.🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-18: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
app/build.gradle (1)
4-5
: SDK Version & Namespace Update:
Upgrading tocompileSdk 35
and adding thenamespace "ai.elimu.launcher"
property is a proper step toward modernizing your Android configuration. Consider reviewing whether thetargetSdkVersion
should also be updated to 35 to fully leverage the behavior changes introduced in the latest Android SDK.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jar
is excluded by!**/*.jar
📒 Files selected for processing (6)
.github/workflows/gradle.yml
(1 hunks)app/build.gradle
(2 hunks)build.gradle
(1 hunks)gradle/wrapper/gradle-wrapper.properties
(1 hunks)gradlew
(2 hunks)gradlew.bat
(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/gradle.yml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-18: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (7)
gradlew (2)
1-1
: Ensure full POSIX compliance across all shells.Changing the shebang to
#!/bin/sh
is a good step towards broader compatibility. However, different systems have varying implementations ofsh
. Please verify that all features (e.g., function definitions, parameter expansions, etc.) work reliably in default shells on the targeted systems (e.g., Alpine, Solaris).Would you like me to generate a script to invoke and test
gradlew
across multiple Linux environments (Alpine, Ubuntu, Debian, etc.) in a container-based approach?
144-159
: Verify portability of “ulimit” usage.Using
ulimit -H -n
within a POSIX shell can sometimes be undefined or behave differently on non-GNU systems (e.g., BSD, older UNIX). Although it appears to be standard in most modern environments, you may want to validate that it does not cause unexpected runtime errors if you plan to build on exotic platforms.gradlew.bat (1)
40-51
: Good error messaging for missing Java.The script clearly directs the user to set
JAVA_HOME
or install Java on the system ifjava.exe
is not found. This is a clean and user-friendly approach that should reduce confusion.gradle/wrapper/gradle-wrapper.properties (1)
3-5
: Verify that the distribution type aligns with your needs.Switching from an
all
distribution to abin
distribution can reduce download size, but it also omits certain Gradle source code and documentation. Make sure that thebin
distribution is sufficient for your use cases—particularly if you generate custom Gradle tasks or rely on certain built-in Gradle scripts not bundled in thebin
distribution.Also applies to: 7-7
.github/workflows/gradle.yml (1)
14-21
: Matrix Strategy Configuration Update:
The new matrix configuration withjava: [17, 21]
and the dynamic assignment ofjava-version: ${{ matrix.java }}
is a great enhancement for multi-version testing. This update will help ensure compatibility with both Java 17 and Java 21.🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-18: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
build.gradle (1)
8-8
: Android Gradle Plugin Upgrade:
Updating the dependency toclasspath 'com.android.tools.build:gradle:8.5.2'
is aligned with the recent build tool changes. Ensure that all sub-projects are compatible with this version, and verify against the official release notes for any migration notes or deprecations.app/build.gradle (1)
38-40
: BuildConfig Generation Enabled:
The addedbuildFeatures { buildConfig = true }
block is effective for generating the BuildConfig class, ensuring that build configuration constants are readily available in your app code. This is particularly useful in newer versions of the Android Gradle plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build checks are failing.
We'll need to fix #71 first. |
This PR depends on #77 |
@tuancoltech Is this PR still needed? |
@jo-elimu Yes, this PR will do 2 things: 1/ Upgrade Android Gradle build tools to 8.5.2 |
…o 35, gradle wrapper to 8.12
Restore jcenter() repository
b83a3ec
to
fde78c7
Compare
This PR depends on #79 |
bc2e038
to
e69a09f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/ai/elimu/launcher/MainActivity.java (1)
24-30
: LGTM! The annotation properly documents the intentional behavior.The
@SuppressLint("MissingSuperCall")
annotation correctly indicates that we're intentionally not callingsuper.onBackPressed()
. This is a common pattern in Android when you want to completely override the back navigation behavior.However, for better maintainability, consider adding a brief comment explaining why back navigation is disabled in this activity.
@SuppressLint("MissingSuperCall") @Override public void onBackPressed() { Timber.i("onBackPressed"); - // Do nothing + // Disable back navigation to prevent users from accidentally exiting the launcher }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/ai/elimu/launcher/MainActivity.java
(2 hunks)app/src/main/java/ai/elimu/launcher/ui/HomeScreensActivity.java
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/ai/elimu/launcher/ui/HomeScreensActivity.java
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
🔇 Additional comments (1)
app/src/main/java/ai/elimu/launcher/MainActivity.java (1)
3-3
: LGTM!Clean addition of the required import for the SuppressLint annotation.
Upgrade
Resolves #70 , #72
Summary by CodeRabbit
Summary by CodeRabbit
These updates strengthen the overall build system, ensuring more robust development and deployment experiences.