Unverified Commit b66878a6 authored by Matan Lurey's avatar Matan Lurey Committed by GitHub

Treat missing --local-engine-host as fatal on CI-like systems. (#132707)

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 🙂
parent af0ca978
......@@ -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).';
......
......@@ -276,6 +276,11 @@ Future<T> runInContext<T>(
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(
......
......@@ -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<EngineBuildPaths?> 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);
......
......@@ -276,7 +276,7 @@ void main() {
platform: platform,
processInfo: processInfo,
fileSystem: testFileSystem,
)).run(<String>['attach', '--local-engine-src-path=$localEngineSrc', '--local-engine=$localEngineDir']);
)).run(<String>['attach', '--local-engine-src-path=$localEngineSrc', '--local-engine=$localEngineDir', '--local-engine-host=$localEngineDir']);
await Future.wait<void>(<Future<void>>[
completer.future,
fakeLogReader.dispose(),
......
......@@ -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: <String, String>{}),
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();
......
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