Skip to content

Normalize Java SDK (JDK) location logic across the tool #124233

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 5 commits into from
Apr 5, 2023
Merged

Normalize Java SDK (JDK) location logic across the tool #124233

auto-submit merged 5 commits into from
Apr 5, 2023

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Apr 5, 2023 •
edited
Loading

Fixes #122609.

Related: #122376

Context

flutter doctor -v prints the path of the java binary that one might assume to be used for all Android-related features (building apps using Gradle, managing emulators, etc.). However, this java-locating logic is different than that used specifically when building Android apps using Gradle. See discussions on #122609 for more info.

To summarize:

For most activities, including flutter doctor validators and Android SDK-related features (creating emulators, checking license acceptance, etc..), we use the following logic:

  1. use the JDK bundled with Android Studio if we can find it; otherwise
  2. use JAVA_HOME (from env) if it is set; otherwise
  3. (on macOS only) use the path returned by /usr/libexec/java_home -v 1.8 if any; otherwise
  4. use which java (search for java on PATH)

However, specifically when building Android apps using Gradle, /usr/libexec/java_home (step 3) will never be consulted. Instead, we'll explicitly set JAVA_HOME to the Android Studio-bundled JDK if it exists; otherwise, gradlew will implicitly consult JAVA_HOME and then look for java on PATH.

This has the potential to be confusing to those debugging build issues in Android-enabled flutter projects.

Change

We'll stop using /usr/libexec/java_home -v 1.8 as a second-to-last fallback for non-Gradle build activities.

Pre-launch Checklist

Pre-launch Checklist
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 5, 2023
@andrewkolos andrewkolos mentioned this pull request Apr 5, 2023
Closed
8 tasks
@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 5, 2023 •
edited
Loading

This is (technically) a breaking change since the version of Java found by flutter doctor and used by flutter to perform Android SDK-related operations1 will change for macOS users for which 1) the flutter tool cannot find the Android Studio-bundled JDK, 2) the JAVA_HOME env variable is unset, and 3) usr/libexec/java_home -v 1.8 returns something different than which java. However, I have to imagine this situation is pretty unlikely.

Unrelated FYI for @reidbaker: this change shouldn't block (or risk merge conflicts with) any other work surrounding #122376. I'll save any refactoring for once the critical work is done.

Footnotes

  1. (validating Android SDK license acceptance, managing emulators, etc.) ↩

@reidbaker
Copy link
Contributor

reidbaker commented Apr 5, 2023

Solid this will get a review today as soon as I am out of meetings

gnprice
gnprice reviewed Apr 5, 2023
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I think a refactor like #124152 is important in order to prevent the two implementations of which JDK to use from diverging again in the future. But this fix makes sense as a way of reconciling the behavior for now while minimizing conflicts with related urgent changes.

In addition to filing that tech-debt issue, I'd suggest adding a couple of comments about the need to keep these separate implementations in sync.

packages/flutter_tools/lib/src/android/android_sdk.dart
if (javaHomeEnv != null) {
// Trust JAVA_HOME.
return fileSystem.path.join(javaHomeEnv, 'bin', 'java');
}
Copy link
Member

@gnprice gnprice Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to add a comment at the top of this function saying it needs to be kept in sync with how android/gradle.dart sets JAVA_HOME when running Gradle.

Also a comment at the relevant places in that file (the two places that https://github.com/flutter/flutter/pull/124152/files#diff-3c44d2e8279ea8d917b4fb942252d4649689154c36cf74227a1a2a879aaf1915 changes to use _javaSdkHome) saying they need to be kept in sync with what findJavaBinary does.

Copy link
Contributor

@reidbaker reidbaker Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewkolos I agree with @gnprice here.

@andrewkolos andrewkolos mentioned this pull request Apr 5, 2023
Closed
christopherfujino
christopherfujino reviewed Apr 5, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll leave final approval to @reidbaker

reidbaker
reidbaker approved these changes Apr 5, 2023
View reviewed changes
@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit auto-submit bot merged commit ec13058 into flutter:master Apr 5, 2023
@andrewkolos andrewkolos deleted the jdk-doctor-fix-simple branch April 5, 2023 22:59
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 6, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 6, 2023
@engine-flutter-autoroll
ec13058 Normalize Java SDK (JDK) location logic across the tool (flut…
2f5bdcf 
…ter/flutter#124233)
@andrewkolos andrewkolos mentioned this pull request Apr 7, 2023
Merged
8 tasks
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
@andrewkolos @exaby73
Normalize Java SDK (JDK) location logic across the tool (flutter#124233)
601431c 
Normalize Java SDK (JDK) location logic across the tool
@andrewkolos andrewkolos mentioned this pull request May 2, 2023
Closed
8 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
ec13058 Normalize Java SDK (JDK) location logic across the tool (flut…
d2e137c 
…ter/flutter#124233)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
ec13058 Normalize Java SDK (JDK) location logic across the tool (flut…
c1728d6 
…ter/flutter#124233)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnprice gnprice gnprice left review comments

@reidbaker reidbaker reidbaker approved these changes

+1 more reviewer

@christopherfujino christopherfujino christopherfujino left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JDK actually used for Gradle doesn't appear in flutter doctor -v nor in flutter run -v

4 participants

@andrewkolos @reidbaker @gnprice @christopherfujino