This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Refactor args parsing/environment constructor for scenario_app
#50980
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,188 +7,80 @@ import 'dart:convert'; | |
| import 'dart:io'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'package:dir_contents_diff/dir_contents_diff.dart' show dirContentsDiff; | ||
| import 'package:engine_repo_tools/engine_repo_tools.dart'; | ||
| import 'package:path/path.dart'; | ||
| import 'package:process/process.dart'; | ||
| import 'package:skia_gold_client/skia_gold_client.dart'; | ||
|
|
||
| import 'utils/adb_logcat_filtering.dart'; | ||
| import 'utils/environment.dart'; | ||
| import 'utils/logs.dart'; | ||
| import 'utils/options.dart'; | ||
| import 'utils/process_manager_extension.dart'; | ||
| import 'utils/screenshot_transformer.dart'; | ||
|
|
||
| // If you update the arguments, update the documentation in the README.md file. | ||
| void main(List<String> args) async { | ||
| // Get some basic environment information to guide the rest of the program. | ||
| final Environment environment = Environment( | ||
| isCi: Platform.environment['LUCI_CONTEXT'] != null, | ||
| showVerbose: Options.showVerbose(args), | ||
| logsDir: Platform.environment['FLUTTER_LOGS_DIR'], | ||
| ); | ||
|
|
||
| // Determine if the CWD is within an engine checkout. | ||
| final Engine? localEngineDir = Engine.tryFindWithin(); | ||
|
|
||
| // Show usage if requested. | ||
| if (Options.showUsage(args)) { | ||
| stdout.writeln(Options.usage( | ||
| environment: environment, | ||
| localEngineDir: localEngineDir, | ||
| )); | ||
| return; | ||
| } | ||
|
|
||
| // Parse the command line arguments. | ||
| final Options options; | ||
| try { | ||
| options = Options.parse( | ||
| args, | ||
| environment: environment, | ||
| localEngine: localEngineDir, | ||
| ); | ||
| } on FormatException catch (error) { | ||
| stderr.writeln(error); | ||
| stderr.writeln(Options.usage( | ||
| environment: environment, | ||
| localEngineDir: localEngineDir, | ||
| )); | ||
| exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| runZonedGuarded( | ||
| () async { | ||
| await _run( | ||
| verbose: options.verbose, | ||
| outDir: Directory(options.outDir), | ||
| adb: File(options.adb), | ||
| smokeTestFullPath: options.smokeTest, | ||
| useSkiaGold: options.useSkiaGold, | ||
|
There was a problem hiding this comment. We can't use spread operators for named arguments? bummer. Filed issue, looks like the spread operator isn't documented on the operators page: dart-lang/site-www#5599 |
||
| enableImpeller: options.enableImpeller, | ||
| impellerBackend: _ImpellerBackend.tryParse(options.impellerBackend), | ||
| logsDir: Directory(options.logsDir), | ||
| contentsGolden: options.outputContentsGolden, | ||
| ndkStack: options.ndkStack, | ||
| ); | ||
| exit(0); | ||
| }, | ||
| (Object error, StackTrace stackTrace) { | ||
| if (error is! Panic) { | ||
| stderr.writeln('Unhandled error: $error'); | ||
| stderr.writeln(stackTrace); | ||
| } | ||
| exitCode = 1; | ||
| }, | ||
| ); | ||
| } | ||
|
|
@@ -222,18 +114,6 @@ Future<void> _run({ | |
| required String ndkStack, | ||
| }) async { | ||
| const ProcessManager pm = LocalProcessManager(); | ||
| final String scenarioAppPath = join(outDir.path, 'scenario_app'); | ||
| final String logcatPath = join(logsDir.path, 'logcat.txt'); | ||
|
|
||
|
|
@@ -521,7 +401,7 @@ Future<void> _run({ | |
| // TODO(matanlurey): Resolve this in a better way. On CI this file always exists. | ||
| File(join(screenshotPath, 'noop.txt')).writeAsStringSync(''); | ||
| // TODO(gaaclarke): We should move this into dir_contents_diff. | ||
| _withTemporaryCwd(dirname(contentsGolden), () { | ||
| final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath); | ||
| if (exitCode != 0) { | ||
| panic(<String>['Output contents incorrect.']); | ||
|
|
@@ -531,3 +411,14 @@ Future<void> _run({ | |
| } | ||
| } | ||
| } | ||
|
|
||
| void _withTemporaryCwd(String path, void Function() callback) { | ||
| final String originalCwd = Directory.current.path; | ||
| Directory.current = Directory(path).path; | ||
|
|
||
| try { | ||
| callback(); | ||
| } finally { | ||
| Directory.current = originalCwd; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // 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'; | ||
|
|
||
| /// An overridable collection of values provided by the environment. | ||
| @immutable | ||
| final class Environment { | ||
| /// Creates a new environment from the given values. | ||
| const Environment({ | ||
| required this.isCi, | ||
| required this.showVerbose, | ||
| required this.logsDir, | ||
| }); | ||
|
|
||
| /// Whether the current program is running on a CI environment. | ||
| /// | ||
| /// Useful for determining if certain features should be enabled or disabled | ||
| /// based on the environment, or to add safety checks (for example, using | ||
| /// confusing or ambiguous flags). | ||
| final bool isCi; | ||
|
|
||
| /// Whether the user has requested verbose logging and program output. | ||
| final bool showVerbose; | ||
|
|
||
| /// What directory to store logs and screenshots in. | ||
| final String? logsDir; | ||
|
|
||
| @override | ||
| bool operator ==(Object o) { | ||
| return o is Environment && | ||
| o.isCi == isCi && | ||
| o.showVerbose == showVerbose && | ||
| o.logsDir == logsDir; | ||
| } | ||
|
|
||
| @override | ||
| int get hashCode => Object.hash(isCi, showVerbose, logsDir); | ||
|
|
||
| @override | ||
| String toString() { | ||
| return 'Environment(isCi: $isCi, showVerbose: $showVerbose, logsDir: $logsDir)'; | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that uncaught exceptions will also set the exit code. Can we rely on that instead of setting the exitCode and trying to short circuit the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a caught exception though.
I could
rethrowbut then the output will look meh?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ugly but informative for developers since it will have the stack trace. I ended up going the uncaught route since trying to manually short circuit and maintain a global variable (exitCode) seemed to be fighting the language. Your call. I may even have ended up doing it both ways in different places =P