[tools] allow explicitly specifying the JDK to use via a new config setting #128264

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 13 commits into flutter:master from andrewkolos:add-jdk-config-item

Jun 7, 2023

Contributor

andrewkolos commented Jun 5, 2023 •

edited

Loading

Closes #106416.

This PR adds a new flutter config setting named jdk-dir. When set, the tool will use the JDK found at this location for all Java-dependent tool operations such as building Android apps via gradle and running Android SDK tools.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.


          add jdk-dir config item

3296e56

andrewkolos added platform-android tool labels

flutter-dashboard bot added the a: text input label

andrewkolos added 4 commits

June 5, 2023 10:26


          reformat

43baf74


          remove tests redundant with java_test.dart

87bfbe9


          lint

f6c1d95


          fix test fake

1d3e528

andrewkolos marked this pull request as ready for review

June 5, 2023 19:52

andrewkolos commented

View reviewed changes

packages/flutter_tools/lib/src/commands/config.dart Outdated

Comment on lines 25 to 29

+                  argParser.addOption('jdk-dir', help: 'The Java Development Kit (JDK) install/home directory. '
+                    'If unset, flutter will search for one in the following order:\n'
+                    '    1) the JDK bundled with the latest installation of Android Studio,\n'
+                    '    2) the JDK found at the directory found in the JAVA_HOME environment variable, and\n'
+                    "    3) the directory containing the java binary found in the user's path.");

Contributor Author

andrewkolos Jun 5, 2023 •

edited

Loading

I'd like to get this ordered list to be indented, but I couldn't find a way to do this. Indenting with spaces or using \t doesn't seem to work as this seems to get removed in the output. (The spaces currently here are there to satisfy args_test.dart). Here is the output from flutter config:

    --android-sdk                               The Android SDK directory.
    --android-studio-dir                        The Android Studio install directory. If unset, flutter will search for valid installs at well-known locations.
    --jdk-dir                                   The Java Development Kit (JDK) install/home directory. If unset, flutter will search for one in the following order:
                                                1) the JDK bundled with the latest installation of Android Studio,
                                                2) the JDK found at the directory found in the JAVA_HOME environment variable, and
                                                3) the directory containing the java binary found in the user's path.
    --build-dir=<out/>                          The relative path to override a projects build directory.
    --[no-]enable-web                           Enable or disable Flutter for web.

andrewkolos commented

View reviewed changes

packages/flutter_tools/lib/src/commands/config.dart Outdated Show resolved Hide resolved

andrewkolos marked this pull request as draft

June 5, 2023 20:00

andrewkolos marked this pull request as ready for review

June 5, 2023 20:06

andrewkolos requested review from reidbaker and christopherfujino

June 5, 2023 20:06

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/lib/src/android/java.dart

                 required Logger logger,
                 required AndroidStudio? androidStudio,
                 required Platform platform,
               }) {
+                final Object? configured = config.getValue('jdk-dir');
+                if (configured != null) {
+                  return configured as String;

Contributor

christopherfujino Jun 5, 2023

nit, doesn't need to be in this PR, but should we move these casts to be methods on the Config class? That way it will be easier and more obvious how to refactor.

Contributor Author

andrewkolos Jun 5, 2023

Filed [tools] Consider adding getter methods to Config that cast values for convenience #128278

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/lib/src/commands/config.dart Outdated Show resolved Hide resolved


          remove extra new line

63ce53f

Co-authored-by: Christopher Fujino <fujino@google.com>

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/lib/src/commands/config.dart Outdated

@@ @@ -154,6 +160,10 @@ class ConfigCommand extends FlutterCommand { @@
                     _updateConfig('android-studio-dir', stringArg('android-studio-dir')!);
                   }
+                  if (argResults?.wasParsed('jdk-dir') ?? false) {

Contributor

christopherfujino Jun 5, 2023

I think here (and throughout this function) we should just cast argResults to a non-nullable value and get rid of all the nullable fallbacks.

Contributor Author

andrewkolos Jun 5, 2023

Done. I used non-null assertions. See commit within ConfigCommand.runCommand, assert argResults in non-null [sic.]


          simplify wording

36a4582

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/lib/src/commands/config.dart

@@ @@ -203,6 +213,10 @@ class ConfigCommand extends FlutterCommand { @@
                   if (results['android-sdk'] == null && androidSdk != null) {
                     results['android-sdk'] = androidSdk.directory.path;
                   }
+                  final Java? java = globals.java;
+                  if (results['jdk-dir'] == null && java != null) {
+                    results['jdk-dir'] = java.javaHome;

Contributor

christopherfujino Jun 5, 2023

this is actually surprising to me, but I suppose we shouldn't break this.

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/lib/src/android/android_workflow.dart Outdated Show resolved Hide resolved

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/test/commands.shard/hermetic/config_test.dart Outdated

@@ @@ -70,6 +70,9 @@ void main() { @@
                       expect(jsonObject.containsKey('android-sdk'), true);
                       expect(jsonObject['android-sdk'], isNotNull);
+                      expect(jsonObject.containsKey('jdk-dir'), true);
+                      expect(jsonObject['jdk-dir'], isNotNull);

Contributor

christopherfujino Jun 5, 2023

for these and the other isNotNull expectations in this test, can we expect on the actual value, to verify we're not letting the real implementations leak through?

Contributor Author

andrewkolos Jun 5, 2023

Done with commit make machine flag test more precise.

This made me realize the inconsistency with some of these fields. AndroidStudio.directory is a string, AndroidSdk.directory is a Directory, and Java.javaHome doesn't fit into the <class>.directory scheme.

andrewkolos added 2 commits

June 5, 2023 13:26


          remove extraneous call to Java.find

8300ebb


          Merge branch 'add-jdk-config-item' of https://github.com/andrewkolos/…

d24ac4e

…flutter into add-jdk-config-item

christopherfujino reviewed

View reviewed changes

packages/flutter_tools/test/general.shard/android/java_test.dart Outdated

+                        },
+                      );
+                      final Java? java = Java.find(

Contributor

christopherfujino Jun 5, 2023

Can you actually call Java.find() twice in this test, once before config.setValue('jdk-dir', ...) and once after, expecting the results are different?

Contributor Author

andrewkolos Jun 5, 2023

Isn't this already implicitly covered by the other tests?

Contributor

christopherfujino Jun 5, 2023 •

edited

Loading

We can only rely on other tests verifying the exact setup of this test if we know that now and forever this test and the dependent test are set up in exactly the same way. Also that that other test is never deleted.

In other words, I would say that this test is not completely verifying that Java.find() is finding java.javaHome via the Config (it could be now or in the future across refactors accidentally getting the right answer from the local configuration).

andrewkolos added 2 commits

June 5, 2023 13:33


          within ConfigCommand.runCommand, assert argResults in non-null

c995e80


          make machine flag test more precise

f45814e

andrewkolos mentioned this pull request

[tools] Consider adding getter methods to Config that cast values for better type-safety #128278

Open


          remove no longer necessary parameters to AndroidLicenseValidator

b01850e

andrewkolos requested a review from christopherfujino

June 5, 2023 21:37


          test Java.find after unsetting jdk-dir

530805f

christopherfujino approved these changes

View reviewed changes

Contributor

christopherfujino left a comment

LGTM

andrewkolos added the autosubmit label

auto-submit bot merged commit 96afa50 into flutter:master

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 0b7415356e07 to 6848110ff3a9 (44 revisions) flutter/packages#4156

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

0d45b17

…config setting (flutter/flutter#128264)

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 0b7415356e07 to 2830f0734d6c (45 revisions) flutter/packages#4158

Closed

andrewkolos mentioned this pull request

Add the ability to pass in java jdk as part of flutter build/run #125652

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

cc135d2

…config setting (flutter/flutter#128264)

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 0b7415356e07 to 8a5c22e282db (46 revisions) flutter/packages#4160

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

74ad301

…config setting (flutter/flutter#128264)

auto-submit bot pushed a commit to flutter/packages that referenced this pull request


          Roll Flutter from 0b74153 to 8a5c22e (46 revisions) (#4160)

e584280

Roll Flutter from 0b74153 to 8a5c22e (46 revisions)

flutter/flutter@0b74153...8a5c22e

2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny MediaQuery doc update (flutter/flutter#127904)
2023-06-07 jacksongardner@google.com Revert "Make inspector weakly referencing the inspected objects." (flutter/flutter#128436)
2023-06-07 leigha.jarett@gmail.com Update menu API docs to help developers migrate to m3 (flutter/flutter#128351)
2023-06-07 andrewrkolos@gmail.com [tools] allow explicitly specifying the JDK to use via a new config setting (flutter/flutter#128264)
2023-06-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6f9df0f988c1 to 59d5444cf06c (3 revisions) (flutter/flutter#128376)
2023-06-07 andrewrkolos@gmail.com Do not try to load main/default asset image if only higher-res variants exist (flutter/flutter#128143)
2023-06-07 92602467+99spark@users.noreply.github.com Addressed Ambiguity in transform.scale constructor docs (flutter/flutter#128182)
2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny fix of dead link (flutter/flutter#128160)
2023-06-07 goderbauer@google.com Refactor tests (flutter/flutter#128371)
2023-06-07 polinach@google.com Make inspector weakly referencing the inspected objects. (flutter/flutter#128095)
2023-06-07 goderbauer@google.com Add viewId to PointerEvents (flutter/flutter#128287)
2023-06-07 5236035+fzyzcjy@users.noreply.github.com Show error message in release mode when box is not laid out without losing performance (flutter/flutter#126302)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from ca499463ec2e to 6f9df0f988c1 (1 revision) (flutter/flutter#128363)
2023-06-06 khanhnwin@gmail.com Update Draggable YouTube video link (flutter/flutter#128078)
2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove more rounding hacks from TextPainter (flutter/flutter#127826)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4571695f9e76 to ca499463ec2e (1 revision) (flutter/flutter#128356)
2023-06-06 43054281+camsim99@users.noreply.github.com [Android] Update plugin and module templates to use Flutter constant for `compileSdkVersion` (flutter/flutter#128073)
2023-06-06 rmolivares@renzo-olivares.dev handleSelectWord in MultiSelectableSelectionContainerDelegate should handle rects inside of rects (flutter/flutter#127478)
2023-06-06 christopherfujino@gmail.com [flutter_tools] never tree shake 0x20 (space) font codepoints on web (flutter/flutter#128302)
2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove `textScaleFactor` dependent logic from `AppBar` (flutter/flutter#128112)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from b6d37f8f74ad to 4571695f9e76 (6 revisions) (flutter/flutter#128350)
2023-06-06 5236035+fzyzcjy@users.noreply.github.com Fix `Null check operator used on a null value` on TextField with contextMenuBuilder (flutter/flutter#128114)
2023-06-06 engine-flutter-autoroll@skia.org Roll Packages from db4e5c2 to da72219 (10 revisions) (flutter/flutter#128348)
2023-06-06 91688203+yusuf-goog@users.noreply.github.com Updating cirrus docker image to ubuntu focal. (flutter/flutter#128291)
2023-06-06 leigha.jarett@gmail.com Adding example for migrating to navigation drawer (flutter/flutter#128295)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 722aad83e5fe to b6d37f8f74ad (2 revisions) (flutter/flutter#128341)
2023-06-06 goderbauer@google.com Clean-up viewId casts in flutter_test (flutter/flutter#128256)
2023-06-06 kevinjchisholm@google.com Update cherry-pick issue template to more uniform labels. (flutter/flutter#128333)
2023-06-06 hans.muller@gmail.com Use Material3 in the 2D viewport tests (flutter/flutter#128155)
2023-06-06 nbosch@google.com Use a `show` over a `hide` for `test_api` exports (flutter/flutter#128298)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from aaa7574375a6 to 722aad83e5fe (1 revision) (flutter/flutter#128307)
2023-06-06 leigha.jarett@gmail.com Migration guide for moving from BottomNavigationBar to NavigationBar (flutter/flutter#128263)
2023-06-06 mdebbar@google.com [web] Use 'Uri' instead of 'dart:html' to extract pathname (flutter/flutter#127983)
2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2b353ae90731 to aaa7574375a6 (4 revisions) (flutter/flutter#128301)
2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 220ece4d9faa to 2b353ae90731 (1 revision) (flutter/flutter#128293)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.0.4 to 4.1.0 (flutter/flutter#128290)
2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7f12e3497428 to 220ece4d9faa (6 revisions) (flutter/flutter#128282)
2023-06-05 jonahwilliams@google.com [framework] attempt non-key solution (flutter/flutter#128273)
2023-06-05 chillers@google.com [labeler] Fix adding labels when name is directory (flutter/flutter#128243)
2023-06-05 katelovett@google.com Remove scrollbar deprecations isAlwaysShown and hoverThickness (flutter/flutter#127351)
2023-06-05 katelovett@google.com Fix update drag error that made NestedScrollView un-scrollable (flutter/flutter#127718)
2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9f72388a4da to 7f12e3497428 (4 revisions) (flutter/flutter#128271)
2023-06-05 goderbauer@google.com Migrate SemanticsBinding to onSemanticsActionEvent (flutter/flutter#128254)
2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from c838a1b05924 to f9f72388a4da (19 revisions) (flutter/flutter#128252)
2023-06-05 jonahwilliams@google.com [framework] force flexible space background to rebuild. (flutter/flutter#128138)
2023-06-05 engine-flutter-autoroll@skia.org Roll Packages from 75085ed to db4e5c2 (4 revisions) (flutter/flutter#128246)
...

andrewkolos deleted the add-jdk-config-item branch

June 14, 2023 22:27

andrewkolos mentioned this pull request

[tool] ☂️ Improve correctness, consistency, and maintainability of Java-finding logic across the tool #126126

Closed

3 tasks

camsim99 mentioned this pull request

Allow Gradle invocations to use Java SDK besides Android Studio bundle JRE #81974

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

9a6006c

…config setting (flutter/flutter#128264)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

fcaf3b9

…config setting (flutter/flutter#128264)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

2e3794f

…config setting (flutter/flutter#128264)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          96afa50 [tools] allow explicitly specifying the JDK to use via a new …

09ca6da

…config setting (flutter/flutter#128264)

w568w mentioned this pull request

feat: Implement Dynamic Color, Refactor Theming & Navigation DanXi-Dev/DanXi#516

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input autosubmit platform-android tool