-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: configure gradle java.home #1795
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1795 +/- ##
==========================================
- Coverage 72.58% 72.11% -0.48%
==========================================
Files 23 23
Lines 1835 1847 +12
==========================================
Hits 1332 1332
- Misses 503 515 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2bfdbb5
to
501467d
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.
LGTM
Hi,
|
Can you provide the exact step you used to get that result? I don't get Android Studio's
If you are not building with Cordova and only opening with Android Studio, you wont have the right value and there is nothing we can do about that. |
Here is example of what I mean by builds wont work when
Build failed the requirement check and stopped. |
Hi, after adding the platform via
I opend it with Android Studio and get the result I described. I didn't build anything in Android Studio, just opened it. Edit: When I do |
I did it like you wrote, but the build succeded: % echo $JAVA_HOME
/Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home
% unset JAVA_HOME
% echo $JAVA_HOME
% cordova build android
Checking Java JDK and Android SDK versions
ANDROID_HOME=/Users/manuelbeck/Library/Android/sdk (recommended setting)
ANDROID_SDK_ROOT=undefined (DEPRECATED)
Using Android SDK: /Users/manuelbeck/Library/Android/sdk
Reading build config file: /Users/manuelbeck/Projekte/app_entwicklung/abfuhr_app/build.json
Reading the keystore from: /Users/manuelbeck/Projekte/app_entwicklung/abfuhr_app/Signierung/Android/netactive.keystore
BUILD SUCCESSFUL in 261ms |
If you opened with Android Studio and you did not run |
So I have to always first do |
This PR only focuses on resolving the issue with However, this PR does not address cases if users open Android Studio first. In that scenario, it's expected that you'll see a warning on the first load, and then Android Studio configures the Ideally, running Currently, Cordova doesn’t create the Gradle wrapper until the project is built using the Cordova command. As far as I remember, it has always worked this way. To support opening a project in Android Studio immediately after adding the platform, Cordova would need to move the Gradle wrapper build process earlier in the workflow—perhaps at the end of Hook scripts already allow users to run custom scripts during key phases, like |
Hi, thanks for the explanation. I unterstand that this is not in scope of this pr. I only mentioned it because I noticed it. |
Motivation, Context & Description
Fix an issue mentioned in a discussion thread.
config.properties
.java.home
is set correctly:CORDOVA_JAVA_HOME
environment variable.CORDOVA_JAVA_HOME
is missing, fall back toJAVA_HOME
.java.home
property (if previously set), allowing Android Studio to display a warning and configure its internal Java automatically.tools/.gradle
directory up one level.Note: Step 2.3 should not occur under normal conditions, but was included as a safeguard. When running
cordova build
, ifJAVA_HOME
is missing, its expected to not continue as its apart of the requirement checks.This change also ensures that Cordova-Android uses the correct Java version specified by the user via environment variables. According to the Cordova documentation, these variables must be configured for Cordova-Android setup. Defaulting to Android Studio's bundled Java could lead to unexpected results and not using versions that the App developer configured.
Additional Feedback
It appears that the change came from AGP 8.2. It created a new way to specify the JDK path was added and became default. This new macro is called
GRADLE_LOCAL_JAVA_HOME
From Android Developer - Java versions in Android builds:
Testing
Confirmed that
java.home
was set with configured environment variables.Re-run build with
CORDOVA_JAVA_HOME
set to another version of Java and confirmed thejava.home
was updated.Created a
before_build
hookscript which would run beforecordova build
.Confirm that the additional properties, defined in the hookscript, were injected in the
config.properties
file and thatjava.home
was set correctly by the build script.Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)