From b66878a61e58642024da7b130371aa117059d2f6 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 16 Aug 2023 17:41:03 -0700 Subject: [PATCH] Treat missing --local-engine-host as fatal on CI-like systems. (#132707) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Partial work towards https://github.com/flutter/flutter/issues/132245. The goal here is to "sniff" out any missing pieces that would block engine builds, rolls, benchmarks and so on before requiring humans to provide the parameter. The implementation is based on a [short discussion with @christopherfujino](https://discord.com/channels/608014603317936148/608022056616853515/1141503921546875110): @matanlurey: > Not sure whether to post here or ⁠hackers-infra-🌡 , but is there a way to (and is it advisable to) detect whether the tool is running in a CI environment? I'd like to "soft enforce" --local-engine-host being provided strictly on CI, make sure that lands well, and then "upgrade" it to being non-CI invocations as well (re: https://github.com/flutter/flutter/issues/132245). > > Also happy to get talked out of this idea 🙂 @christopherfujino: > we have a check, lemme find it > whether or not it is advisable, idk > https://github.com/flutter/flutter/blob/flutter-3.14-candidate.0/packages/flutter_tools/lib/src/base/bot_detector.dart#L30 > > (...) > > is your desire to get early signal before enforcing t his for humans to prevent functionality churn of landing and reverting and re-landing? > > (yes) > > uhh, sure, that's advisable 🙂 --- .../lib/src/base/user_messages.dart | 4 ++-- .../flutter_tools/lib/src/context_runner.dart | 5 ++++ .../lib/src/runner/local_engine.dart | 12 +++++++--- .../commands.shard/hermetic/attach_test.dart | 2 +- .../runner/local_engine_test.dart | 23 +++++++++++++++++++ 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/user_messages.dart b/packages/flutter_tools/lib/src/base/user_messages.dart index 780561311e..6ebaa13471 100644 --- a/packages/flutter_tools/lib/src/base/user_messages.dart +++ b/packages/flutter_tools/lib/src/base/user_messages.dart @@ -309,8 +309,8 @@ class UserMessages { String get runnerLocalEngineOrWebSdkRequired => 'You must specify --local-engine or --local-web-sdk if you are using a locally built engine or web sdk.'; // TODO(matanlurey): Make this an error, https://github.com/flutter/flutter/issues/132245. - String get runnerLocalEngineRequiresHostEngine => - 'Warning! You are using a locally built engine (--local-engine) but have not specified --local-host-engine.\n' + String runnerLocalEngineRequiresHostEngine({bool warning = false}) => + '${warning ? 'Warning! ' : ''}You are using a locally built engine (--local-engine) but have not specified --local-host-engine.\n' 'You may be building with a different engine than the one you are running with. ' 'See https://github.com/flutter/flutter/issues/132245 for details (in the future this will become ' 'an error).'; diff --git a/packages/flutter_tools/lib/src/context_runner.dart b/packages/flutter_tools/lib/src/context_runner.dart index a5a4f92c25..a3f0b268ab 100644 --- a/packages/flutter_tools/lib/src/context_runner.dart +++ b/packages/flutter_tools/lib/src/context_runner.dart @@ -276,6 +276,11 @@ Future runInContext( platform: globals.platform, fileSystem: globals.fs, flutterRoot: Cache.flutterRoot!, + // TODO(matanlurey): https://github.com/flutter/flutter/issues/132245. + // Even though we *think* this feature is stable, we'd like to verify + // that automated tests are all providing `--local-engine-host` before + // enforcing it for all users. + treatMissingLocalEngineHostAsFatal: runningOnBot, ), Logger: () => globals.platform.isWindows ? WindowsStdoutLogger( diff --git a/packages/flutter_tools/lib/src/runner/local_engine.dart b/packages/flutter_tools/lib/src/runner/local_engine.dart index 57c9c19a8b..5170bf59e2 100644 --- a/packages/flutter_tools/lib/src/runner/local_engine.dart +++ b/packages/flutter_tools/lib/src/runner/local_engine.dart @@ -33,17 +33,20 @@ class LocalEngineLocator { required FileSystem fileSystem, required String flutterRoot, required UserMessages userMessages, + bool treatMissingLocalEngineHostAsFatal = false, }) : _platform = platform, _logger = logger, _fileSystem = fileSystem, _flutterRoot = flutterRoot, - _userMessages = userMessages; + _userMessages = userMessages, + _treatMissingLocalEngineHostAsFatal = treatMissingLocalEngineHostAsFatal; final Platform _platform; final Logger _logger; final FileSystem _fileSystem; final String _flutterRoot; final UserMessages _userMessages; + final bool _treatMissingLocalEngineHostAsFatal; /// Returns the engine build path of a local engine if one is located, otherwise `null`. Future findEnginePath({ @@ -206,8 +209,11 @@ class LocalEngineLocator { } if (localHostEngine == null) { - // TODO(matanlurey): https://github.com/flutter/flutter/issues/132245, change to throwToolExit. - _logger.printStatus(_userMessages.runnerLocalEngineRequiresHostEngine); + // TODO(matanlurey): https://github.com/flutter/flutter/issues/132245, always throwToolExit. + if (_treatMissingLocalEngineHostAsFatal) { + throwToolExit(_userMessages.runnerLocalEngineRequiresHostEngine()); + } + _logger.printStatus(_userMessages.runnerLocalEngineRequiresHostEngine(warning: true)); } final String basename = localHostEngine ?? _fileSystem.path.basename(engineBuildPath); final String hostBasename = _getHostEngineBasename(basename); diff --git a/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart index 5740b20c93..d334c8aa9d 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart @@ -276,7 +276,7 @@ void main() { platform: platform, processInfo: processInfo, fileSystem: testFileSystem, - )).run(['attach', '--local-engine-src-path=$localEngineSrc', '--local-engine=$localEngineDir']); + )).run(['attach', '--local-engine-src-path=$localEngineSrc', '--local-engine=$localEngineDir', '--local-engine-host=$localEngineDir']); await Future.wait(>[ completer.future, fakeLogReader.dispose(), diff --git a/packages/flutter_tools/test/general.shard/runner/local_engine_test.dart b/packages/flutter_tools/test/general.shard/runner/local_engine_test.dart index f2f3aedcbf..b18a3633a2 100644 --- a/packages/flutter_tools/test/general.shard/runner/local_engine_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/local_engine_test.dart @@ -145,6 +145,29 @@ void main() { expect(logger.statusText, contains('Warning! You are using a locally built engine (--local-engine) but have not specified --local-host-engine')); }); + testWithoutContext('fails if --local-engine-host is emitted and treatMissingLocalEngineHostAsFatal is set', () async { + final FileSystem fileSystem = MemoryFileSystem.test(); + final Directory localEngine = fileSystem + .directory('$kArbitraryEngineRoot/src/out/android_debug_unopt_arm64/') + ..createSync(recursive: true); + fileSystem.directory('$kArbitraryEngineRoot/src/out/host_debug_unopt/').createSync(recursive: true); + + final BufferLogger logger = BufferLogger.test(); + final LocalEngineLocator localEngineLocator = LocalEngineLocator( + fileSystem: fileSystem, + flutterRoot: 'flutter/flutter', + logger: logger, + userMessages: UserMessages(), + platform: FakePlatform(environment: {}), + treatMissingLocalEngineHostAsFatal: true, + ); + + await expectLater( + localEngineLocator.findEnginePath(localEngine: localEngine.path), + throwsToolExit(message: 'You are using a locally built engine (--local-engine) but have not specified --local-host-engine'), + ); + }); + testWithoutContext('works if --local-engine is specified and --local-engine-src-path ' 'is determined by --local-engine', () async { final FileSystem fileSystem = MemoryFileSystem.test();