Skip to content
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

[tool] Move Java functions to their own file #126086

Merged
merged 25 commits into from
May 11, 2023
Merged

[tool] Move Java functions to their own file #126086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ea4bc2a
introduce Java class and have AndroidSdk use it for java related things
andrewkolos May 4, 2023
6654a91
fix tests
andrewkolos May 4, 2023
b592b88
fix test
andrewkolos May 5, 2023
fc34192
make java global
andrewkolos May 5, 2023
42c5992
oops
andrewkolos May 5, 2023
f6320c9
lint
andrewkolos May 5, 2023
0d75603
add todo docs
andrewkolos May 5, 2023
6576e71
analyzer lint
andrewkolos May 5, 2023
0c2b7fc
migrate existing androidSdk.java* callers
andrewkolos May 9, 2023
681c97e
move java construction to context_runner.dart
andrewkolos May 9, 2023
053cdea
use absolute paths in FakeJava
andrewkolos May 9, 2023
47481a5
add dart doc for JavaVersion members; rename shortText->number
andrewkolos May 9, 2023
fcb51c8
analyze lints
andrewkolos May 9, 2023
04ed942
fix some indentation
andrewkolos May 9, 2023
fad8da7
Update proxied_devices_test.dart
andrewkolos May 9, 2023
63b0077
remove try-catch in _findJavaBinary
andrewkolos May 9, 2023
f0da7c7
tweak docstring
andrewkolos May 9, 2023
42ceaf0
tweak docstring
andrewkolos May 9, 2023
fcaddc2
experiment: use lazily-initialization for version property
andrewkolos May 9, 2023
f9f0d57
Update android_project_migration_test.dart
andrewkolos May 9, 2023
0b5d0fc
remove non-warning traces
andrewkolos May 9, 2023
f9e97a0
Update packages/flutter_tools/lib/src/globals.dart
andrewkolos May 9, 2023
b289b44
Merge branch 'master' into move-java-functions-simple
andrewkolos May 10, 2023
3884d3b
Merge branch 'master' into move-java-functions-simple
andrewkolos May 10, 2023
71f0099
Merge branch 'master' into move-java-functions-simple
andrewkolos May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
172 changes: 7 additions & 165 deletions packages/flutter_tools/lib/src/android/android_sdk.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,13 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:meta/meta.dart';

import '../base/common.dart';
import '../base/file_system.dart';
import '../base/os.dart';
import '../base/platform.dart';
import '../base/process.dart';
import '../base/version.dart';
import '../convert.dart';
import '../globals.dart' as globals;
import 'android_studio.dart';
import 'java.dart';

// ANDROID_HOME is deprecated.
// See https://developer.android.com/studio/command-line/variables.html#envar
Expand All @@ -36,16 +32,18 @@ final RegExp _sdkVersionRe = RegExp(r'^ro.build.version.sdk=([0-9]+)$');
// $ANDROID_SDK_ROOT/platforms/android-23/android.jar
// $ANDROID_SDK_ROOT/platforms/android-N/android.jar
class AndroidSdk {
AndroidSdk(this.directory) {
AndroidSdk(this.directory, {
Java? java,
}): _java = java {
reinitialize();
}

static const String javaHomeEnvironmentVariable = 'JAVA_HOME';
static const String _javaExecutable = 'java';

/// The Android SDK root directory.
final Directory directory;

final Java? _java;

List<AndroidSdkVersion> _sdkVersions = <AndroidSdkVersion>[];
AndroidSdkVersion? _latestVersion;

Expand Down Expand Up @@ -411,162 +409,6 @@ class AndroidSdk {
return null;
}

/// Returns the version of java in the format \d(.\d)+(.\d)+
/// Returns null if version not found.
String? getJavaVersion({
required AndroidStudio? androidStudio,
required FileSystem fileSystem,
required OperatingSystemUtils operatingSystemUtils,
required Platform platform,
required ProcessUtils processUtils,
}) {
final String? javaBinary = findJavaBinary(
androidStudio: androidStudio,
fileSystem: fileSystem,
operatingSystemUtils: operatingSystemUtils,
platform: platform,
);
if (javaBinary == null) {
globals.printTrace('Could not find java binary to get version.');
return null;
}
final RunResult result = processUtils.runSync(
<String>[javaBinary, '--version'],
environment: sdkManagerEnv,
);
if (result.exitCode != 0) {
globals.printTrace(
'java --version failed: exitCode: ${result.exitCode} stdout: ${result.stdout} stderr: ${result.stderr}');
return null;
}
return parseJavaVersion(result.stdout);
}

/// Extracts JDK version from the output of java --version.
@visibleForTesting
static String? parseJavaVersion(String rawVersionOutput) {
// The contents that matter come in the format '11.0.18' or '1.8.0_202'.
final RegExp jdkVersionRegex = RegExp(r'\d+\.\d+(\.\d+(?:_\d+)?)?');
final Iterable<RegExpMatch> matches =
jdkVersionRegex.allMatches(rawVersionOutput);
if (matches.isEmpty) {
globals.logger.printWarning(_formatJavaVersionWarning(rawVersionOutput));
return null;
}
final String? versionString = matches.first.group(0);
if (versionString == null || versionString.split('_').isEmpty) {
globals.logger.printWarning(_formatJavaVersionWarning(rawVersionOutput));
return null;
}
// Trim away _d+ from versions 1.8 and below.
return versionString.split('_').first;
}

/// A value that would be appropriate to use as JAVA_HOME.
///
/// This method considers jdk in the following order:
/// * the JDK bundled with Android Studio, if one is found;
/// * the JAVA_HOME in the ambient environment, if set;
String? get javaHome {
return findJavaHome(
androidStudio: globals.androidStudio,
fileSystem: globals.fs,
operatingSystemUtils: globals.os,
platform: globals.platform,
);
}


static String? findJavaHome({
required AndroidStudio? androidStudio,
required FileSystem fileSystem,
required OperatingSystemUtils operatingSystemUtils,
required Platform platform,
}) {
if (androidStudio?.javaPath != null) {
globals.printTrace("Using Android Studio's java.");
return androidStudio!.javaPath!;
}

final String? javaHomeEnv = platform.environment[javaHomeEnvironmentVariable];
if (javaHomeEnv != null) {
globals.printTrace('Using JAVA_HOME from environment valuables.');
return javaHomeEnv;
}
return null;
}

/// Finds the java binary that is used for all operations across the tool.
///
/// This comes from [findJavaHome] if that method returns non-null;
/// otherwise, it gets from searching PATH.
// TODO(andrewkolos): To prevent confusion when debugging Android-related
// issues (see https://github.com/flutter/flutter/issues/122609 for an example),
// this logic should be consistently followed by any Java-dependent operation
// across the the tool (building Android apps, interacting with the Android SDK, etc.).
// Currently, this consistency is fragile since the logic used for building
// Android apps exists independently of this method.
// See https://github.com/flutter/flutter/issues/124252.
static String? findJavaBinary({
required AndroidStudio? androidStudio,
required FileSystem fileSystem,
required OperatingSystemUtils operatingSystemUtils,
required Platform platform,
}) {
final String? javaHome = findJavaHome(
androidStudio: androidStudio,
fileSystem: fileSystem,
operatingSystemUtils: operatingSystemUtils,
platform: platform,
);

if (javaHome != null) {
return fileSystem.path.join(javaHome, 'bin', 'java');
}

// Fallback to PATH based lookup.
final String? pathJava = operatingSystemUtils.which(_javaExecutable)?.path;
if (pathJava != null) {
globals.printTrace('Using java from PATH.');
} else {
globals.printTrace('Could not find java path.');
}
return pathJava;
}

// Returns a user visible String that says the tool failed to parse
// the version of java along with the output.
static String _formatJavaVersionWarning(String javaVersionRaw) {
return 'Could not parse java version from: \n'
'$javaVersionRaw \n'
'If there is a version please look for an existing bug '
'https://github.com/flutter/flutter/issues/'
' and if one does not exist file a new issue.';
}

Map<String, String>? _sdkManagerEnv;

/// Returns an environment with the Java folder added to PATH for use in calling
/// Java-based Android SDK commands such as sdkmanager and avdmanager.
Map<String, String> get sdkManagerEnv {
if (_sdkManagerEnv == null) {
// If we can locate Java, then add it to the path used to run the Android SDK manager.
_sdkManagerEnv = <String, String>{};
final String? javaBinary = findJavaBinary(
androidStudio: globals.androidStudio,
fileSystem: globals.fs,
operatingSystemUtils: globals.os,
platform: globals.platform,
);
if (javaBinary != null && globals.platform.environment['PATH'] != null) {
_sdkManagerEnv!['PATH'] = globals.fs.path.dirname(javaBinary) +
globals.os.pathVarSeparator +
globals.platform.environment['PATH']!;
}
}
return _sdkManagerEnv!;
}

/// Returns the version of the Android SDK manager tool or null if not found.
String? get sdkManagerVersion {
if (sdkManagerPath == null || !globals.processManager.canRun(sdkManagerPath)) {
Expand All @@ -577,7 +419,7 @@ class AndroidSdk {
}
final RunResult result = globals.processUtils.runSync(
<String>[sdkManagerPath!, '--version'],
environment: sdkManagerEnv,
environment: _java?.environment,
);
if (result.exitCode != 0) {
globals.printTrace('sdkmanager --version failed: exitCode: ${result.exitCode} stdout: ${result.stdout} stderr: ${result.stderr}');
Expand Down
37 changes: 17 additions & 20 deletions packages/flutter_tools/lib/src/android/android_workflow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import '../base/context.dart';
import '../base/file_system.dart';
import '../base/io.dart';
import '../base/logger.dart';
import '../base/os.dart';
import '../base/platform.dart';
import '../base/user_messages.dart' hide userMessages;
import '../base/version.dart';
Expand All @@ -20,6 +19,7 @@ import '../doctor_validator.dart';
import '../features.dart';
import 'android_sdk.dart';
import 'android_studio.dart';
import 'java.dart';

const int kAndroidSdkMinVersion = 29;
final Version kAndroidJavaMinVersion = Version(1, 8, 0);
Expand Down Expand Up @@ -86,12 +86,6 @@ class AndroidValidator extends DoctorValidator {
_androidStudio = androidStudio,
_fileSystem = fileSystem,
_logger = logger,
_operatingSystemUtils = OperatingSystemUtils(
fileSystem: fileSystem,
logger: logger,
platform: platform,
processManager: processManager,
),
_platform = platform,
_processManager = processManager,
_userMessages = userMessages,
Expand All @@ -101,7 +95,6 @@ class AndroidValidator extends DoctorValidator {
final AndroidStudio? _androidStudio;
final FileSystem _fileSystem;
final Logger _logger;
final OperatingSystemUtils _operatingSystemUtils;
final Platform _platform;
final ProcessManager _processManager;
final UserMessages _userMessages;
Expand Down Expand Up @@ -138,6 +131,8 @@ class AndroidValidator extends DoctorValidator {
}
String? javaVersionText;
try {
// TODO(andrewkolos): Use Java class to find version instead of using duplicate
// code. See https://github.com/flutter/flutter/issues/124252.
_logger.printTrace('java -version');
final ProcessResult result = await _processManager.run(<String>[javaBinary, '-version']);
if (result.exitCode == 0) {
Expand Down Expand Up @@ -240,12 +235,13 @@ class AndroidValidator extends DoctorValidator {

_task = 'Finding Java binary';
// Now check for the JDK.
final String? javaBinary = AndroidSdk.findJavaBinary(
final String? javaBinary = Java.find(
logger: _logger,
androidStudio: _androidStudio,
fileSystem: _fileSystem,
operatingSystemUtils: _operatingSystemUtils,
platform: _platform,
);
processManager: _processManager,
)?.binaryPath;
if (javaBinary == null) {
messages.add(ValidationMessage.error(_userMessages.androidMissingJdk));
return ValidationResult(ValidationType.partial, messages, statusInfo: sdkVersionText);
Expand All @@ -266,18 +262,18 @@ class AndroidValidator extends DoctorValidator {
/// SDK have been accepted.
class AndroidLicenseValidator extends DoctorValidator {
AndroidLicenseValidator({
required Java? java,
required AndroidSdk? androidSdk,
required Platform platform,
required OperatingSystemUtils operatingSystemUtils,
required FileSystem fileSystem,
required ProcessManager processManager,
required Logger logger,
required AndroidStudio? androidStudio,
required Stdio stdio,
required UserMessages userMessages,
}) : _androidSdk = androidSdk,
}) : _java = java,
_androidSdk = androidSdk,
_platform = platform,
_operatingSystemUtils = operatingSystemUtils,
_fileSystem = fileSystem,
_processManager = processManager,
_logger = logger,
Expand All @@ -286,10 +282,10 @@ class AndroidLicenseValidator extends DoctorValidator {
_userMessages = userMessages,
super('Android license subvalidator');

final Java? _java;
final AndroidSdk? _androidSdk;
final AndroidStudio? _androidStudio;
final Stdio _stdio;
final OperatingSystemUtils _operatingSystemUtils;
final Platform _platform;
final FileSystem _fileSystem;
final ProcessManager _processManager;
Expand Down Expand Up @@ -330,12 +326,13 @@ class AndroidLicenseValidator extends DoctorValidator {
}

Future<bool> _checkJavaVersionNoOutput() async {
final String? javaBinary = AndroidSdk.findJavaBinary(
final String? javaBinary = Java.find(
logger: _logger,
androidStudio: _androidStudio,
fileSystem: _fileSystem,
operatingSystemUtils: _operatingSystemUtils,
platform: _platform,
);
processManager: _processManager,
)?.binaryPath;
if (javaBinary == null) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

For the canRun check we do on the next line, should we not do that during Java.find() and if you can't run it, keep falling back?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, or should we tool exit because maybe the user should fix their JDK setup? I'm not sure, legitimately asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the canRun check we do on the next line, should we not do that during Java.find() and if you can't run it, keep falling back?

Not sure what you mean here, though I probably should further migrate this code by using the canRun and version methods/properties on Java.

Hmm, or should we tool exit because maybe the user should fix their JDK setup? I'm not sure, legitimately asking.

Hmm...I haven't looked at this code critically before. If we can't find the java version (or a well-formed Android SDK), we returned a ValidationResult with a ValidationType of missing, so I wonder what the doctor output would look like here. I'm just thinking out loud.

I can play around with this once I get home, where I have some a VM set up to test this kind of stuff.

Copy link
Member

Choose a reason for hiding this comment

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

To unblock this PR you can add a TODO for this, if you like.

Expand Down Expand Up @@ -387,7 +384,7 @@ class AndroidLicenseValidator extends DoctorValidator {
try {
final Process process = await _processManager.start(
<String>[_androidSdk!.sdkManagerPath!, '--licenses'],
environment: _androidSdk!.sdkManagerEnv,
environment: _java?.environment,
);
process.stdin.write('n\n');
// We expect logcat streams to occasionally contain invalid utf-8,
Expand Down Expand Up @@ -427,7 +424,7 @@ class AndroidLicenseValidator extends DoctorValidator {
try {
final Process process = await _processManager.start(
<String>[_androidSdk!.sdkManagerPath!, '--licenses'],
environment: _androidSdk!.sdkManagerEnv,
environment: _java?.environment,
);

// The real stdin will never finish streaming. Pipe until the child process
Expand Down