-
-
Notifications
You must be signed in to change notification settings - Fork 3
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 to 8.5.2 and compileSdk to 35 #118
Conversation
WalkthroughThis update modifies the Android project’s build configuration. In the app module’s Gradle file, the target SDK version is increased from 32 to 35, the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Gradle as Gradle Build System
participant AGP as Android Gradle Plugin
participant Builder as Build Process
Dev->>Gradle: Initiate build
Gradle->>AGP: Load app/build.gradle configuration
AGP->>Gradle: Process compileSdk, namespace, buildConfig settings
Gradle->>Builder: Trigger BuildConfig generation
Builder->>Gradle: Build tasks execute
Gradle->>Dev: Complete build process
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: 0
🔭 Outside diff range comments (1)
app/build.gradle (1)
40-43
: 🛠️ Refactor suggestion
⚠️ Potential issueUpdate Java Compatibility:
ThecompileOptions
block still specifies JavaVersion.VERSION_11. Since AGP 8.1.4 requires Java 17, this discrepancy is likely causing build failures (as indicated by the pipeline error). Please update the Java compatibility to Java 17.Diff suggestion:
- sourceCompatibility JavaVersion.VERSION_11 - targetCompatibility JavaVersion.VERSION_11 + sourceCompatibility JavaVersion.VERSION_17 + targetCompatibility JavaVersion.VERSION_17
🧹 Nitpick comments (1)
app/build.gradle (1)
4-5
: SDK and Namespace Update:
The changes to update the SDK and package structure are clear:
compileSdk
is now set to 35, which aligns with the latest APIs.- A new
namespace "ai.elimu.vitabu"
has been introduced for better modularization and future-proofing.Note: Review the
targetSdkVersion
in thedefaultConfig
(currently at 32) to ensure it aligns with the updatedcompileSdk
if that is intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/build.gradle
(2 hunks)build.gradle
(1 hunks)gradle/wrapper/gradle-wrapper.properties
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- gradle/wrapper/gradle-wrapper.properties
🧰 Additional context used
🪛 GitHub Actions: Gradle Build
build.gradle
[error] 1-1: Android Gradle plugin requires Java 17 to run. You are currently using Java 11.
app/build.gradle
[error] 1-1: Android Gradle plugin requires Java 17 to run. You are currently using Java 11.
🔇 Additional comments (2)
build.gradle (1)
11-11
: AGP Version Upgrade:
The Android Gradle Plugin version has been correctly updated to 8.1.4. Please note that AGP 8.1.4 requires Java 17 to run. Ensure your build environment (and any related CI/CD configurations) is updated accordingly to avoid runtime issues.app/build.gradle (1)
44-47
: Build Features Update:
Enabling the generation of aBuildConfig
class by settingbuildConfig = true
is a good practice with the updated AGP. This change should streamline build configuration management.
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.
@tuancoltech The build checks are failing.
…radle Plugin 8.1.x
@jo-elimu I fixed the build error just now. |
2646aa3
to
e77c31c
Compare
e77c31c
to
3584490
Compare
@@ -1,6 +1,6 @@ | |||
#Thu Feb 24 11:21:45 PST 2022 | |||
distributionBase=GRADLE_USER_HOME | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-all.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-all.zip |
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.
If you use the Gradle Upgrade Manager, or the ./gradlew wrapper --gradle-version x.x.x
command, I think more files would be affected?
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.
@jo-elimu You're correct.
In fact, both ways are OK.
I usually just update the distributionUrl to avoid extra changes, but it seems the more recommended
way is to run gradlew wrapper
command.
Ref: https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:upgrading_wrapper
Maybe I'll take this chance to upgrade this wrapper via command as per you suggested.
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.
@jo-elimu Updated now using the command instead.
* Upgrade Gradle wrapper version to 8.12 * Add Java 21 to Action build script
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 (2)
gradlew (1)
144-161
: Limited utility for “maximum” file descriptors on POSIX.The
ulimit -H -n
approach is standard, but some distros may not allow raising the limit without explicit admin privileges. This won’t break the script, but it may produce warnings.gradlew.bat (1)
37-37
: Consider adjusting default memory settings.Defaulting to
-Xmx64m
and-Xms64m
might be low for larger Android Gradle builds on Java 17. Consider raising the heap size (e.g., 128m or 256m) to prevent potential build failures.
📜 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 (5)
.github/workflows/gradle-build.yml
(1 hunks)build.gradle
(1 hunks)gradle/wrapper/gradle-wrapper.properties
(1 hunks)gradlew
(2 hunks)gradlew.bat
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- build.gradle
- .github/workflows/gradle-build.yml
- gradle/wrapper/gradle-wrapper.properties
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
🔇 Additional comments (8)
gradlew (4)
1-16
: Ensure consistent shell availability.Switching the shebang to
#!/bin/sh
is standard for POSIX compliance, but on some systems/bin/sh
might be a minimal shell lacking certain features. It's worth verifying that all required POSIX features listed in the comments are indeed supported by/bin/sh
on your target distributions.
68-82
: Symlink resolution logic looks good.The updated approach to resolve symlinks via
app_path
and acase
statement is more robust and avoids deprecated constructs. No issues found.
119-141
: Validate Java detection and error messages.The script checks both
$JAVA_HOME
and the presence of thejava
command. This is solid logic and gracefully fails with a clear error message if Java is missing. Be sure to keep this consistent with the required Java 17 environment for the updated AGP version.
162-247
: Check availability ofxargs
on minimal systems.Requiring
xargs
is generally fine, but minimal environments might not have it installed by default. The script already handles this by failing fast and prompting the user. This approach is acceptable; just be aware of potential friction in very constrained build environments.gradlew.bat (4)
1-16
: License header inclusion is correct.Including the Apache License 2.0 header at the start of the batch script is good practice. No issues found.
28-35
: Path resolution improvement.Shortening
%APP_HOME%
via the loop helps avoid edge cases with relative paths. This makes the script more robust across Windows environments.
44-50
: Error handling alignment.Switching to
if %ERRORLEVEL% equ 0
and redirecting messages to1>&2
is a best practice for batch scripts, ensuring clear user feedback.
58-65
: Validation ofJAVA_HOME
directory.Verifying
%JAVA_EXE%
exists before proceeding avoids silent failures. This aligns well with your Java 17 requirement.
Issue Number
Purpose
Summary by CodeRabbit
Summary by CodeRabbit