Skip to content

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

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
Closed
andrewkolos wants to merge 8 commits into from
Closed

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

andrewkolos wants to merge 8 commits into from

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Apr 4, 2023

Fixes #122609.

Related: #123643.
Tangentially related: #122376. (there are probably more issues than this--I need to look around)

CC: @christopherfujino @reidbaker

Context

flutter doctor -v prints the path of the java binary that is presumably used for all Android-related features (building apps, managing emulators, etc.). However, this logic is different from that used in other places tool outside of flutter doctor, which makes it possible for the JDK path that flutter doctor lists to be different from the one that flutter actually uses when building Android apps. See this comment on #122609 from Greg and this comment from me on #122609 for more info.

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

Change

This change normalizes the logic of JDK-location across the tool. Said another way, any feature that needs to find a JDK installation will use the same logic to do so. The code that implements this logic is also lightly refactored.

To-do

  • Consider restoring usage of /usr/libexec/java_home on macOS systems, which was consulted as a second-to-last resort when finding the JDK to use for everything except invoking gradle.
  • Add tests
  • Make the change description above more helpful

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 4, 2023
christopherfujino
christopherfujino reviewed Apr 4, 2023
View reviewed changes
packages/flutter_tools/lib/src/globals.dart Outdated

String? _javaPath;

String? get javaHome => _javaPath ??= findJavaHome(
Copy link
Contributor

@christopherfujino christopherfujino Apr 4, 2023

Choose a reason for hiding this comment

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

This is going to lead to a lot more implicit context accesses, does this need to be global?

Copy link
Contributor

@christopherfujino christopherfujino Apr 4, 2023

Choose a reason for hiding this comment

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

Or, can't we just have whatever needs to access this directly invoke findJavaHome?

@christopherfujino
Copy link
Contributor

christopherfujino commented Apr 5, 2023

Added a comment to the tracking issue: #122609 (comment)

@gnprice gnprice mentioned this pull request Apr 5, 2023
Merged
8 tasks
@andrewkolos andrewkolos marked this pull request as ready for review April 5, 2023 16:13
@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 5, 2023 •
edited
Loading

I marked this as ready-for-review since @christopherfujino has added reviewers--though tests are still needed.

Based on continued discussion on #122609, a simpler change that could be made instead of this one is simply to remove the macOS-specific logic in findJavaBinary. This change alone should solve this issue since 1) we already try to use the Android Studio-bundled JDK when trying to run anything dependent on Java1 and 2) Android SDK tools already try to use JAVA_HOME and which java. The only downside there is that the JDK-location logic is not visible at a flutter tool level--it's implicit behavior coming from the Android SDK.

Footnotes

  1. see references to javaPath from android_studio.dart ↩

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 5, 2023 •
edited
Loading

Based on continued discussion on #122609, a simpler change that could be made instead of this one is simply to remove the macOS-specific logic in findJavaBinary. This change alone should solve this issue since 1) we already try to use the Android Studio-bundled JDK when trying to run anything dependent on Java and 2) Android SDK tools already try to use JAVA_HOME and which java. The only downside there is that the JDK-location logic is not visible at a flutter tool level--it's implicit behavior coming from the Android SDK.

Per this and offline discussion with @christopherfujino, I will make a simpler PR that just removes the macOS-specific logic. This will make code review simpler and cherrypicking easier, which is ideal since we want this change sooner rather than later. Will post another comment with the new PR.

@andrewkolos andrewkolos closed this Apr 5, 2023
@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 5, 2023

Opened #124233

@christopherfujino
Copy link
Contributor

christopherfujino commented Apr 5, 2023

Based on continued discussion on #122609, a simpler change that could be made instead of this one is simply to remove the macOS-specific logic in findJavaBinary. This change alone should solve this issue since 1) we already try to use the Android Studio-bundled JDK when trying to run anything dependent on Java and 2) Android SDK tools already try to use JAVA_HOME and which java. The only downside there is that the JDK-location logic is not visible at a flutter tool level--it's implicit behavior coming from the Android SDK.

Per this and offline discussion with @christopherfujino, I will make a simpler PR that just removes the macOS-specific logic. This will make code review simpler and cherrypicking easier, which is ideal since we want this change sooner rather than later. Will post another comment with the new PR.

Thanks! I still think it's worth landing this refactor, but we can do that once the minimal "fix" has landed.

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Apr 5, 2023

I agree. I will set a reminder to open a tech-debt issue since JDK-finding is still fundamentally confusing at a flutter tool code level

@christopherfujino
Copy link
Contributor

christopherfujino commented Apr 5, 2023

since JDK-finding is still fundamentally confusing at a flutter tool code level

That's an understatement :)

@andrewkolos andrewkolos deleted the jdk-doctor branch April 21, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnprice gnprice Awaiting requested review from gnprice

@reidbaker reidbaker Awaiting requested review from reidbaker

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

Assignees

No one assigned

Labels

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

2 participants

@andrewkolos @christopherfujino