The logic used to find a JDK in various tool operations is unclear and confusing #124252

Activity

1. andrewkolos

   added

   toolAffects the "flutter" command-line tool. See also t: labels.Affects the "flutter" command-line tool. See also t: labels.

   c: tech-debtTechnical debt, code quality, testing, etc.Technical debt, code quality, testing, etc.

   on Apr 5, 2023
2. andrewkolos

   self-assigned this

   on Apr 5, 2023
3. andrewkolos commented on Apr 5, 2023

   

   andrewkolos

   on Apr 5, 2023

   ContributorAuthor

   More actions

   Replaced placeholder text.
4. andrewkolos

   added

   t: flutter doctorProblem related to the "flutter doctor" toolProblem related to the "flutter doctor" tool

   platform-androidAndroid applications specificallyAndroid applications specifically

   P2Important issues not at the top of the work listImportant issues not at the top of the work list

   P3Issues that are less important to the Flutter projectIssues that are less important to the Flutter project

   and removed

   P2Important issues not at the top of the work listImportant issues not at the top of the work list

   P3Issues that are less important to the Flutter projectIssues that are less important to the Flutter project

   on Apr 5, 2023
5. gnprice commented on Apr 7, 2023

   

   gnprice

   on Apr 7, 2023

   Member

   More actions

   Thanks for filing this!

   I think one of the things that's made this a bit tricky, and led to the existing situation where we don't simply use the same logic everywhere, is that there are a couple of different kinds of ways that we invoke a JDK:

   • In some places (1a and 2 in your list), we run a java executable directly. These are a good fit for the existing findJavaBinary method, which provides the path of a java executable to run.
   • In other places (1b, 3, 4), we run a wrapper script that finds and runs java. (Either the Gradle wrapper gradlew, or the Android SDK command-line tools like avdmanager and sdkmanager.) We can't directly tell these what JDK to use in the form of a path to the java executable, so findJavaBinary doesn't directly help.

   We can and should tell those wrapper scripts how to find the exact same JDK we use elsewhere. But the way we tell them will just have to be a bit more subtle.

   The key is that all of those wrapper scripts have the same logic for how they find what JDK to use. They have verbatim the same code for it, in fact (#122609 (comment)) — but even if they didn't, the core logic has I think been standard in the Java world for a long time. That logic is:

   • Look for the environment variable JAVA_HOME. If there's an executable at "$JAVA_HOME"/bin/java, then use that as the java command to run.
   • Else fall back to looking for a java command on the PATH.

   So to get those wrapper scripts to use the JDK we want them to, we just have to set JAVA_HOME and/or PATH appropriately.

   ---

   There are several possible ways to do that, but I think the cleanest approach is:

   • Observe that our findJavaBinary method already has the same structure:
     • First it looks for a directory that could be used as a JAVA_HOME: a directory that should have a subfolder bin with a file java that is a suitable java command. (Its first choice for this is androidStudio?.javaPath, i.e. a JDK folder found within Android Studio.)
     • If that fails, it looks for a java command on the PATH.
   • So, refactor findJavaBinary to make that structure explicit: there's a findJavaHome that returns a directory that could be used as a JAVA_HOME. Then the implementation of findJavaBinary reduces to

     return findJavaHome(/* … */) ?? operatingSystemUtils.which(_javaExecutable)?.path;

     except it's a few lines longer than that in order to retain the printTrace calls.
   • Then, when running any Java-world wrapper script, including Gradle or the Android SDK command-line tools, do so with a call like:

     final String? javaHome = findJavaHome(/* … */);
     await _processUtils.run(
       args,
       environment: { 'JAVA_HOME': javaHome ?? '' }, // let PATH pass through from the tool's own environment
     );

     (Probably encapsulate that into a helper that lives next to findJavaHome and findJavaBinary, so that the call actually looks more like await _processUtils.run(args, environment: findJavaEnvironment(/* … */));.)
   • This will cause the wrapper script to use exactly the same java executable as findJavaBinary would return: if findJavaHome finds something, it will use that, and if not then it will find java on the PATH.
6. andrewkolos

   removed their assignment

   on Apr 7, 2023
7. abarke commented on Apr 25, 2023

   

   abarke

   on Apr 25, 2023

   Last edited by abarke

   More actions

   +1 I ran into this today... real pain #106416 (comment)

   As a flutter noob with first impressions, this doesn't feel good and doesn't provide me with much confidence.
8. auto-submit

   closed this as completedin #127354on Jun 1, 2023
9. auto-submit

   added a commit that references this issue on Jun 1, 2023

   [tools] use Java class for all java-searching behavior (#127354)

   ...

   This issue will close once commit 06e2b18 is merged into the 'master' branch.

   06e2b18
10. github-actions commented on Jun 15, 2023

    

    github-actionsbot

    on Jun 15, 2023

    More actions

    This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.
11. github-actions

    locked as resolved and limited conversation to collaborators on Jun 15, 2023