Unverified Commit d29668dd authored by Ian Hickson's avatar Ian Hickson Committed by GitHub

Improve network resources doctor check (#120417)

...and various other minor cleanup:

* Moved "FLUTTER_STORAGE_BASE_URL" into a constant throughout the code. There are other strings that we should do that to but this one was relevant to the code I was changing.

* Fixed the logger's handling of slow warnings. Previously it deleted too much text. Fixed the test for that to actually verify it entirely, too.

* Made the logger delete the slow warning when it's finished.

* Fixed 'Please choose one (To quit, press "q/Q")' message to be the cleaner 'Please choose one (or "q" to quit)'.

* Added a debug toString to ValidationResult for debugging purposes (not used).

* In http_host_validator:

  - Shortened constant names to be clearer (e.g. kPubDevHttpHost -> kPubDev).
  - Added GitHub as a tested host since when you run `flutter` we hit that immediately.
  - Renamed the check "Network resources".
  - Updated the `slowWarning` of the check to say which hosts are pending.
  - Removed all timeout logic. Timeouts violate our style guide.
  - Removed `int.parse(... ?? '10')`; passing a constant to `int.parse` is inefficient.
  - Replaced the `_HostValidationResult` class with `String?` for simplicity.
  - Improved the error messages to be more detailed.
  - Removed all checks that dependened on the stringification of exceptions. That's very brittle.
  - Added a warning specifically for HandshakeException that talks about the implications (MITM attacks).
  - Replaced exception-message-parsing logic with just calling `Uri.tryParse` and validating the result.
  - Replaced a lot of list-filtering logic with just a single for loop to check the results.
  - Replaced code that added a constant to a known-empty list with just returning a constant list.
  - Revamped the logic for deciding which hosts to check to just use a single chain of if/else blocks instead of getters, lists literals with `if` expressions, `??`, functions, etc spread over multiple places in the code.
parent 9d94a51b
...@@ -16,6 +16,7 @@ import '../base/deferred_component.dart'; ...@@ -16,6 +16,7 @@ import '../base/deferred_component.dart';
import '../base/file_system.dart'; import '../base/file_system.dart';
import '../base/io.dart'; import '../base/io.dart';
import '../base/logger.dart'; import '../base/logger.dart';
import '../base/net.dart';
import '../base/platform.dart'; import '../base/platform.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../base/terminal.dart'; import '../base/terminal.dart';
...@@ -696,7 +697,7 @@ void printHowToConsumeAar({ ...@@ -696,7 +697,7 @@ void printHowToConsumeAar({
1. Open ${fileSystem.path.join('<host>', 'app', 'build.gradle')} 1. Open ${fileSystem.path.join('<host>', 'app', 'build.gradle')}
2. Ensure you have the repositories configured, otherwise add them: 2. Ensure you have the repositories configured, otherwise add them:
String storageUrl = System.env.FLUTTER_STORAGE_BASE_URL ?: "https://storage.googleapis.com" String storageUrl = System.env.$kFlutterStorageBaseUrl ?: "https://storage.googleapis.com"
repositories { repositories {
maven { maven {
url '${repoDirectory.path}' url '${repoDirectory.path}'
......
...@@ -1360,7 +1360,9 @@ class AnonymousSpinnerStatus extends Status { ...@@ -1360,7 +1360,9 @@ class AnonymousSpinnerStatus extends Status {
if (seemsSlow) { if (seemsSlow) {
if (!timedOut) { if (!timedOut) {
timedOut = true; timedOut = true;
_clear(_currentLineLength); if (_currentLineLength > _lastAnimationFrameLength) {
_clear(_currentLineLength - _lastAnimationFrameLength);
}
} }
if (_slowWarning == '' && slowWarningCallback != null) { if (_slowWarning == '' && slowWarningCallback != null) {
_slowWarning = slowWarningCallback!(); _slowWarning = slowWarningCallback!();
...@@ -1398,7 +1400,8 @@ class AnonymousSpinnerStatus extends Status { ...@@ -1398,7 +1400,8 @@ class AnonymousSpinnerStatus extends Status {
assert(timer!.isActive); assert(timer!.isActive);
timer?.cancel(); timer?.cancel();
timer = null; timer = null;
_clear(_lastAnimationFrameLength); _clear(_lastAnimationFrameLength + _slowWarning.length);
_slowWarning = '';
_lastAnimationFrameLength = 0; _lastAnimationFrameLength = 0;
super.finish(); super.finish();
} }
......
...@@ -14,6 +14,7 @@ import 'logger.dart'; ...@@ -14,6 +14,7 @@ import 'logger.dart';
import 'platform.dart'; import 'platform.dart';
const int kNetworkProblemExitCode = 50; const int kNetworkProblemExitCode = 50;
const String kFlutterStorageBaseUrl = 'FLUTTER_STORAGE_BASE_URL';
typedef HttpClientFactory = HttpClient Function(); typedef HttpClientFactory = HttpClient Function();
...@@ -106,15 +107,16 @@ class Net { ...@@ -106,15 +107,16 @@ class Net {
} }
response = await request.close(); response = await request.close();
} on ArgumentError catch (error) { } on ArgumentError catch (error) {
final String? overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; final String? overrideUrl = _platform.environment[kFlutterStorageBaseUrl];
if (overrideUrl != null && url.toString().contains(overrideUrl)) { if (overrideUrl != null && url.toString().contains(overrideUrl)) {
_logger.printError(error.toString()); _logger.printError(error.toString());
throwToolExit( throwToolExit(
'The value of FLUTTER_STORAGE_BASE_URL ($overrideUrl) could not be ' 'The value of $kFlutterStorageBaseUrl ($overrideUrl) could not be '
'parsed as a valid url. Please see https://flutter.dev/community/china ' 'parsed as a valid url. Please see https://flutter.dev/community/china '
'for an example of how to use it.\n' 'for an example of how to use it.\n'
'Full URL: $url', 'Full URL: $url',
exitCode: kNetworkProblemExitCode,); exitCode: kNetworkProblemExitCode,
);
} }
_logger.printError(error.toString()); _logger.printError(error.toString());
rethrow; rethrow;
......
...@@ -274,7 +274,7 @@ class UserMessages { ...@@ -274,7 +274,7 @@ class UserMessages {
'Found $count devices with name or id matching $deviceId:'; 'Found $count devices with name or id matching $deviceId:';
String get flutterMultipleDevicesFound => 'Multiple devices found:'; String get flutterMultipleDevicesFound => 'Multiple devices found:';
String flutterChooseDevice(int option, String name, String deviceId) => '[$option]: $name ($deviceId)'; String flutterChooseDevice(int option, String name, String deviceId) => '[$option]: $name ($deviceId)';
String get flutterChooseOne => 'Please choose one (To quit, press "q/Q")'; String get flutterChooseOne => 'Please choose one (or "q" to quit)';
String get flutterSpecifyDeviceWithAllOption => String get flutterSpecifyDeviceWithAllOption =>
'More than one device connected; please specify a device with ' 'More than one device connected; please specify a device with '
"the '-d <deviceId>' flag, or use '-d all' to act on all devices."; "the '-d <deviceId>' flag, or use '-d all' to act on all devices.";
......
...@@ -107,8 +107,8 @@ class DevelopmentArtifact { ...@@ -107,8 +107,8 @@ class DevelopmentArtifact {
/// ///
/// To enable Flutter users in these environments, the Flutter tool supports /// To enable Flutter users in these environments, the Flutter tool supports
/// custom artifact mirrors that the administrators of such environments may /// custom artifact mirrors that the administrators of such environments may
/// provide. To use an artifact mirror, the user defines the /// provide. To use an artifact mirror, the user defines the [kFlutterStorageBaseUrl]
/// `FLUTTER_STORAGE_BASE_URL` environment variable that points to the mirror. /// (`FLUTTER_STORAGE_BASE_URL`) environment variable that points to the mirror.
/// Flutter tool reads this variable and uses it instead of the default URLs. /// Flutter tool reads this variable and uses it instead of the default URLs.
/// ///
/// For more details on specific URLs used to download artifacts, see /// For more details on specific URLs used to download artifacts, see
...@@ -450,15 +450,15 @@ class Cache { ...@@ -450,15 +450,15 @@ class Cache {
/// during the installation of the Flutter SDK. /// during the installation of the Flutter SDK.
/// ///
/// By default the base URL is https://storage.googleapis.com. However, if /// By default the base URL is https://storage.googleapis.com. However, if
/// `FLUTTER_STORAGE_BASE_URL` environment variable is provided, the /// `FLUTTER_STORAGE_BASE_URL` environment variable ([kFlutterStorageBaseUrl])
/// environment variable value is returned instead. /// is provided, the environment variable value is returned instead.
/// ///
/// See also: /// See also:
/// ///
/// * [cipdBaseUrl], which determines how CIPD artifacts are fetched. /// * [cipdBaseUrl], which determines how CIPD artifacts are fetched.
/// * [Cache] class-level dartdocs that explain how artifact mirrors work. /// * [Cache] class-level dartdocs that explain how artifact mirrors work.
String get storageBaseUrl { String get storageBaseUrl {
final String? overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; final String? overrideUrl = _platform.environment[kFlutterStorageBaseUrl];
if (overrideUrl == null) { if (overrideUrl == null) {
return 'https://storage.googleapis.com'; return 'https://storage.googleapis.com';
} }
...@@ -466,7 +466,7 @@ class Cache { ...@@ -466,7 +466,7 @@ class Cache {
try { try {
Uri.parse(overrideUrl); Uri.parse(overrideUrl);
} on FormatException catch (err) { } on FormatException catch (err) {
throwToolExit('"FLUTTER_STORAGE_BASE_URL" contains an invalid URI:\n$err'); throwToolExit('"$kFlutterStorageBaseUrl" contains an invalid URL:\n$err');
} }
_maybeWarnAboutStorageOverride(overrideUrl); _maybeWarnAboutStorageOverride(overrideUrl);
return overrideUrl; return overrideUrl;
...@@ -479,8 +479,8 @@ class Cache { ...@@ -479,8 +479,8 @@ class Cache {
/// from [storageBaseUrl]. /// from [storageBaseUrl].
/// ///
/// By default the base URL is https://chrome-infra-packages.appspot.com/dl. /// By default the base URL is https://chrome-infra-packages.appspot.com/dl.
/// However, if `FLUTTER_STORAGE_BASE_URL` environment variable is provided, /// However, if `FLUTTER_STORAGE_BASE_URL` environment variable is provided
/// then the following value is used: /// ([kFlutterStorageBaseUrl]), then the following value is used:
/// ///
/// FLUTTER_STORAGE_BASE_URL/flutter_infra_release/cipd /// FLUTTER_STORAGE_BASE_URL/flutter_infra_release/cipd
/// ///
...@@ -492,7 +492,7 @@ class Cache { ...@@ -492,7 +492,7 @@ class Cache {
/// which contains information about CIPD. /// which contains information about CIPD.
/// * [Cache] class-level dartdocs that explain how artifact mirrors work. /// * [Cache] class-level dartdocs that explain how artifact mirrors work.
String get cipdBaseUrl { String get cipdBaseUrl {
final String? overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; final String? overrideUrl = _platform.environment[kFlutterStorageBaseUrl];
if (overrideUrl == null) { if (overrideUrl == null) {
return 'https://chrome-infra-packages.appspot.com/dl'; return 'https://chrome-infra-packages.appspot.com/dl';
} }
...@@ -501,7 +501,7 @@ class Cache { ...@@ -501,7 +501,7 @@ class Cache {
try { try {
original = Uri.parse(overrideUrl); original = Uri.parse(overrideUrl);
} on FormatException catch (err) { } on FormatException catch (err) {
throwToolExit('"FLUTTER_STORAGE_BASE_URL" contains an invalid URI:\n$err'); throwToolExit('"$kFlutterStorageBaseUrl" contains an invalid URL:\n$err');
} }
final String cipdOverride = original.replace( final String cipdOverride = original.replace(
...@@ -1065,11 +1065,11 @@ class ArtifactUpdater { ...@@ -1065,11 +1065,11 @@ class ArtifactUpdater {
} }
continue; continue;
} on ArgumentError catch (error) { } on ArgumentError catch (error) {
final String? overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; final String? overrideUrl = _platform.environment[kFlutterStorageBaseUrl];
if (overrideUrl != null && url.toString().contains(overrideUrl)) { if (overrideUrl != null && url.toString().contains(overrideUrl)) {
_logger.printError(error.toString()); _logger.printError(error.toString());
throwToolExit( throwToolExit(
'The value of FLUTTER_STORAGE_BASE_URL ($overrideUrl) could not be ' 'The value of $kFlutterStorageBaseUrl ($overrideUrl) could not be '
'parsed as a valid url. Please see https://flutter.dev/community/china ' 'parsed as a valid url. Please see https://flutter.dev/community/china '
'for an example of how to use it.\n' 'for an example of how to use it.\n'
'Full URL: $url', 'Full URL: $url',
......
...@@ -136,7 +136,7 @@ class UpdatePackagesCommand extends FlutterCommand { ...@@ -136,7 +136,7 @@ class UpdatePackagesCommand extends FlutterCommand {
); );
Future<void> _downloadCoverageData() async { Future<void> _downloadCoverageData() async {
final String urlBase = globals.platform.environment['FLUTTER_STORAGE_BASE_URL'] ?? 'https://storage.googleapis.com'; final String urlBase = globals.platform.environment[kFlutterStorageBaseUrl] ?? 'https://storage.googleapis.com';
final Uri coverageUri = Uri.parse('$urlBase/flutter_infra_release/flutter/coverage/lcov.info'); final Uri coverageUri = Uri.parse('$urlBase/flutter_infra_release/flutter/coverage/lcov.info');
final List<int>? data = await _net.fetchUrl( final List<int>? data = await _net.fetchUrl(
coverageUri, coverageUri,
......
...@@ -15,6 +15,7 @@ import 'base/context.dart'; ...@@ -15,6 +15,7 @@ import 'base/context.dart';
import 'base/file_system.dart'; import 'base/file_system.dart';
import 'base/io.dart'; import 'base/io.dart';
import 'base/logger.dart'; import 'base/logger.dart';
import 'base/net.dart';
import 'base/os.dart'; import 'base/os.dart';
import 'base/platform.dart'; import 'base/platform.dart';
import 'base/terminal.dart'; import 'base/terminal.dart';
...@@ -528,11 +529,11 @@ class FlutterValidator extends DoctorValidator { ...@@ -528,11 +529,11 @@ class FlutterValidator extends DoctorValidator {
messages.add(ValidationMessage(_userMessages.engineRevision(version.engineRevisionShort))); messages.add(ValidationMessage(_userMessages.engineRevision(version.engineRevisionShort)));
messages.add(ValidationMessage(_userMessages.dartRevision(version.dartSdkVersion))); messages.add(ValidationMessage(_userMessages.dartRevision(version.dartSdkVersion)));
messages.add(ValidationMessage(_userMessages.devToolsVersion(_devToolsVersion()))); messages.add(ValidationMessage(_userMessages.devToolsVersion(_devToolsVersion())));
final String? pubUrl = _platform.environment['PUB_HOSTED_URL']; final String? pubUrl = _platform.environment[kPubDevOverride];
if (pubUrl != null) { if (pubUrl != null) {
messages.add(ValidationMessage(_userMessages.pubMirrorURL(pubUrl))); messages.add(ValidationMessage(_userMessages.pubMirrorURL(pubUrl)));
} }
final String? storageBaseUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; final String? storageBaseUrl = _platform.environment[kFlutterStorageBaseUrl];
if (storageBaseUrl != null) { if (storageBaseUrl != null) {
messages.add(ValidationMessage(_userMessages.flutterMirrorURL(storageBaseUrl))); messages.add(ValidationMessage(_userMessages.flutterMirrorURL(storageBaseUrl)));
} }
......
...@@ -207,6 +207,11 @@ class ValidationResult { ...@@ -207,6 +207,11 @@ class ValidationResult {
return 'partial'; return 'partial';
} }
} }
@override
String toString() {
return '$runtimeType($type, $messages, $statusInfo)';
}
} }
/// A status line for the flutter doctor validation to display. /// A status line for the flutter doctor validation to display.
......
...@@ -497,10 +497,7 @@ void main() { ...@@ -497,10 +497,7 @@ void main() {
time.elapse(timeLapse); time.elapse(timeLapse);
List<String> lines = outputStdout(); List<String> lines = outputStdout();
expect( expect(lines.join(), '⣽\ba warning message.⣻');
lines.join(),
contains(warningMessage),
);
spinner.stop(); spinner.stop();
lines = outputStdout(); lines = outputStdout();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment