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

Use "aliases" in args to fix some technical debt (#81073)

parent 858123dc
...@@ -85,11 +85,6 @@ List<Target> _kDefaultTargets = <Target>[ ...@@ -85,11 +85,6 @@ List<Target> _kDefaultTargets = <Target>[
const ReleaseBundleWindowsAssetsUwp(), const ReleaseBundleWindowsAssetsUwp(),
]; ];
// TODO(ianh): https://github.com/dart-lang/args/issues/181 will allow us to remove useLegacyNames
// and just switch to arguments that use the regular style, which still supporting the old names.
// When fixing this, remove the hack in test/general.shard/args_test.dart that ignores these names.
const bool useLegacyNames = true;
/// Assemble provides a low level API to interact with the flutter tool build /// Assemble provides a low level API to interact with the flutter tool build
/// system. /// system.
class AssembleCommand extends FlutterCommand { class AssembleCommand extends FlutterCommand {
...@@ -128,8 +123,8 @@ class AssembleCommand extends FlutterCommand { ...@@ -128,8 +123,8 @@ class AssembleCommand extends FlutterCommand {
'files will be written. Must be either absolute or relative from the ' 'files will be written. Must be either absolute or relative from the '
'root of the current Flutter project.', 'root of the current Flutter project.',
); );
usesExtraDartFlagOptions(verboseHelp: verboseHelp, useLegacyNames: useLegacyNames); usesExtraDartFlagOptions(verboseHelp: verboseHelp);
usesDartDefineOption(useLegacyNames: useLegacyNames); usesDartDefineOption();
argParser.addOption( argParser.addOption(
'resource-pool-size', 'resource-pool-size',
help: 'The maximum number of concurrent tasks the build system will run.', help: 'The maximum number of concurrent tasks the build system will run.',
...@@ -263,18 +258,18 @@ class AssembleCommand extends FlutterCommand { ...@@ -263,18 +258,18 @@ class AssembleCommand extends FlutterCommand {
final String value = chunk.substring(indexEquals + 1); final String value = chunk.substring(indexEquals + 1);
results[key] = value; results[key] = value;
} }
if (argResults.wasParsed(useLegacyNames ? kExtraGenSnapshotOptions : FlutterOptions.kExtraGenSnapshotOptions)) { if (argResults.wasParsed(FlutterOptions.kExtraGenSnapshotOptions)) {
results[kExtraGenSnapshotOptions] = (argResults[useLegacyNames ? kExtraGenSnapshotOptions : FlutterOptions.kExtraGenSnapshotOptions] as List<String>).join(','); results[kExtraGenSnapshotOptions] = (argResults[FlutterOptions.kExtraGenSnapshotOptions] as List<String>).join(',');
} }
if (argResults.wasParsed(useLegacyNames ? kDartDefines : FlutterOptions.kDartDefinesOption)) { if (argResults.wasParsed(FlutterOptions.kDartDefinesOption)) {
results[kDartDefines] = (argResults[useLegacyNames ? kDartDefines : FlutterOptions.kDartDefinesOption] as List<String>).join(','); results[kDartDefines] = (argResults[FlutterOptions.kDartDefinesOption] as List<String>).join(',');
} }
results[kDeferredComponents] = 'false'; results[kDeferredComponents] = 'false';
if (FlutterProject.current().manifest.deferredComponents != null && isDeferredComponentsTargets() && !isDebug()) { if (FlutterProject.current().manifest.deferredComponents != null && isDeferredComponentsTargets() && !isDebug()) {
results[kDeferredComponents] = 'true'; results[kDeferredComponents] = 'true';
} }
if (argResults.wasParsed(useLegacyNames ? kExtraFrontEndOptions : FlutterOptions.kExtraFrontEndOptions)) { if (argResults.wasParsed(FlutterOptions.kExtraFrontEndOptions)) {
results[kExtraFrontEndOptions] = (argResults[useLegacyNames ? kExtraFrontEndOptions : FlutterOptions.kExtraFrontEndOptions] as List<String>).join(','); results[kExtraFrontEndOptions] = (argResults[FlutterOptions.kExtraFrontEndOptions] as List<String>).join(',');
} }
return results; return results;
} }
......
...@@ -39,7 +39,7 @@ import '../vmservice.dart'; ...@@ -39,7 +39,7 @@ import '../vmservice.dart';
/// With an application already running, a HotRunner can be attached to it /// With an application already running, a HotRunner can be attached to it
/// with: /// with:
/// ``` /// ```
/// $ flutter attach --debug-uri http://127.0.0.1:12345/QqL7EFEDNG0=/ /// $ flutter attach --debug-url http://127.0.0.1:12345/QqL7EFEDNG0=/
/// ``` /// ```
/// ///
/// If `--disable-service-auth-codes` was provided to the application at startup /// If `--disable-service-auth-codes` was provided to the application at startup
...@@ -77,9 +77,10 @@ class AttachCommand extends FlutterCommand { ...@@ -77,9 +77,10 @@ class AttachCommand extends FlutterCommand {
help: '(deprecated) Device port where the observatory is listening. Requires ' help: '(deprecated) Device port where the observatory is listening. Requires '
'"--disable-service-auth-codes" to also be provided to the Flutter ' '"--disable-service-auth-codes" to also be provided to the Flutter '
'application at launch, otherwise this command will fail to connect to ' 'application at launch, otherwise this command will fail to connect to '
'the application. In general, "--debug-uri" should be used instead.', 'the application. In general, "--debug-url" should be used instead.',
)..addOption( )..addOption(
'debug-uri', // TODO(ianh): we should support --debug-url as well (leaving this as an alias). 'debug-url',
aliases: <String>[ 'debug-uri' ], // supported for historical reasons
help: 'The URL at which the observatory is listening.', help: 'The URL at which the observatory is listening.',
)..addOption( )..addOption(
'app-id', 'app-id',
...@@ -153,15 +154,15 @@ known, it can be explicitly provided to attach via the command-line, e.g. ...@@ -153,15 +154,15 @@ known, it can be explicitly provided to attach via the command-line, e.g.
} }
Uri get debugUri { Uri get debugUri {
if (argResults['debug-uri'] == null) { if (argResults['debug-url'] == null) {
return null; return null;
} }
final Uri uri = Uri.tryParse(stringArg('debug-uri')); final Uri uri = Uri.tryParse(stringArg('debug-url'));
if (uri == null) { if (uri == null) {
throwToolExit('Invalid `--debug-uri`: ${stringArg('debug-uri')}'); throwToolExit('Invalid `--debug-url`: ${stringArg('debug-url')}');
} }
if (!uri.hasPort) { if (!uri.hasPort) {
throwToolExit('Port not specified for `--debug-uri`: $uri'); throwToolExit('Port not specified for `--debug-url`: $uri');
} }
return uri; return uri;
} }
...@@ -181,13 +182,13 @@ known, it can be explicitly provided to attach via the command-line, e.g. ...@@ -181,13 +182,13 @@ known, it can be explicitly provided to attach via the command-line, e.g.
debugPort; debugPort;
if (debugPort == null && debugUri == null && argResults.wasParsed(FlutterCommand.ipv6Flag)) { if (debugPort == null && debugUri == null && argResults.wasParsed(FlutterCommand.ipv6Flag)) {
throwToolExit( throwToolExit(
'When the --debug-port or --debug-uri is unknown, this command determines ' 'When the --debug-port or --debug-url is unknown, this command determines '
'the value of --ipv6 on its own.', 'the value of --ipv6 on its own.',
); );
} }
if (debugPort == null && debugUri == null && argResults.wasParsed(FlutterCommand.observatoryPortOption)) { if (debugPort == null && debugUri == null && argResults.wasParsed(FlutterCommand.observatoryPortOption)) {
throwToolExit( throwToolExit(
'When the --debug-port or --debug-uri is unknown, this command does not use ' 'When the --debug-port or --debug-url is unknown, this command does not use '
'the value of --observatory-port.', 'the value of --observatory-port.',
); );
} }
......
...@@ -16,7 +16,7 @@ import '../vmservice.dart'; ...@@ -16,7 +16,7 @@ import '../vmservice.dart';
const String _kOut = 'out'; const String _kOut = 'out';
const String _kType = 'type'; const String _kType = 'type';
const String _kObservatoryUri = 'observatory-uri'; // TODO(ianh): change this to "observatory-url" (but support -uri as an alias) const String _kObservatoryUrl = 'observatory-url';
const String _kDeviceType = 'device'; const String _kDeviceType = 'device';
const String _kSkiaType = 'skia'; const String _kSkiaType = 'skia';
const String _kRasterizerType = 'rasterizer'; const String _kRasterizerType = 'rasterizer';
...@@ -30,7 +30,8 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -30,7 +30,8 @@ class ScreenshotCommand extends FlutterCommand {
help: 'Location to write the screenshot.', help: 'Location to write the screenshot.',
); );
argParser.addOption( argParser.addOption(
_kObservatoryUri, _kObservatoryUrl,
aliases: <String>[ 'observatory-url' ], // for historical reasons
valueHelp: 'URI', valueHelp: 'URI',
help: 'The Observatory URL to which to connect.\n' help: 'The Observatory URL to which to connect.\n'
'This is required when "--$_kType" is "$_kSkiaType" or "$_kRasterizerType".\n' 'This is required when "--$_kType" is "$_kSkiaType" or "$_kRasterizerType".\n'
...@@ -46,8 +47,8 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -46,8 +47,8 @@ class ScreenshotCommand extends FlutterCommand {
_kDeviceType: 'Delegate to the device\'s native screenshot capabilities. This ' _kDeviceType: 'Delegate to the device\'s native screenshot capabilities. This '
'screenshots the entire screen currently being displayed (including content ' 'screenshots the entire screen currently being displayed (including content '
'not rendered by Flutter, like the device status bar).', 'not rendered by Flutter, like the device status bar).',
_kSkiaType: 'Render the Flutter app as a Skia picture. Requires "--$_kObservatoryUri".', _kSkiaType: 'Render the Flutter app as a Skia picture. Requires "--$_kObservatoryUrl".',
_kRasterizerType: 'Render the Flutter app using the rasterizer. Requires "--$_kObservatoryUri."', _kRasterizerType: 'Render the Flutter app using the rasterizer. Requires "--$_kObservatoryUrl."',
}, },
defaultsTo: _kDeviceType, defaultsTo: _kDeviceType,
); );
...@@ -65,10 +66,10 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -65,10 +66,10 @@ class ScreenshotCommand extends FlutterCommand {
Device device; Device device;
Future<void> _validateOptions(String screenshotType, String observatoryUri) async { Future<void> _validateOptions(String screenshotType, String observatoryUrl) async {
switch (screenshotType) { switch (screenshotType) {
case _kDeviceType: case _kDeviceType:
if (observatoryUri != null) { if (observatoryUrl != null) {
throwToolExit('Observatory URI cannot be provided for screenshot type $screenshotType'); throwToolExit('Observatory URI cannot be provided for screenshot type $screenshotType');
} }
device = await findTargetDevice(); device = await findTargetDevice();
...@@ -80,18 +81,18 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -80,18 +81,18 @@ class ScreenshotCommand extends FlutterCommand {
} }
break; break;
default: default:
if (observatoryUri == null) { if (observatoryUrl == null) {
throwToolExit('Observatory URI must be specified for screenshot type $screenshotType'); throwToolExit('Observatory URI must be specified for screenshot type $screenshotType');
} }
if (observatoryUri.isEmpty || Uri.tryParse(observatoryUri) == null) { if (observatoryUrl.isEmpty || Uri.tryParse(observatoryUrl) == null) {
throwToolExit('Observatory URI "$observatoryUri" is invalid'); throwToolExit('Observatory URI "$observatoryUrl" is invalid');
} }
} }
} }
@override @override
Future<FlutterCommandResult> verifyThenRunCommand(String commandPath) async { Future<FlutterCommandResult> verifyThenRunCommand(String commandPath) async {
await _validateOptions(stringArg(_kType), stringArg(_kObservatoryUri)); await _validateOptions(stringArg(_kType), stringArg(_kObservatoryUrl));
return super.verifyThenRunCommand(commandPath); return super.verifyThenRunCommand(commandPath);
} }
...@@ -134,8 +135,8 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -134,8 +135,8 @@ class ScreenshotCommand extends FlutterCommand {
} }
Future<bool> runSkia(File outputFile) async { Future<bool> runSkia(File outputFile) async {
final Uri observatoryUri = Uri.parse(stringArg(_kObservatoryUri)); final Uri observatoryUrl = Uri.parse(stringArg(_kObservatoryUrl));
final FlutterVmService vmService = await connectToVmService(observatoryUri, logger: globals.logger); final FlutterVmService vmService = await connectToVmService(observatoryUrl, logger: globals.logger);
final vm_service.Response skp = await vmService.screenshotSkp(); final vm_service.Response skp = await vmService.screenshotSkp();
if (skp == null) { if (skp == null) {
globals.printError( globals.printError(
...@@ -158,8 +159,8 @@ class ScreenshotCommand extends FlutterCommand { ...@@ -158,8 +159,8 @@ class ScreenshotCommand extends FlutterCommand {
} }
Future<bool> runRasterizer(File outputFile) async { Future<bool> runRasterizer(File outputFile) async {
final Uri observatoryUri = Uri.parse(stringArg(_kObservatoryUri)); final Uri observatoryUrl = Uri.parse(stringArg(_kObservatoryUrl));
final FlutterVmService vmService = await connectToVmService(observatoryUri, logger: globals.logger); final FlutterVmService vmService = await connectToVmService(observatoryUrl, logger: globals.logger);
final vm_service.Response response = await vmService.screenshot(); final vm_service.Response response = await vmService.screenshot();
if (response == null) { if (response == null) {
globals.printError( globals.printError(
......
...@@ -18,7 +18,7 @@ import '../base/user_messages.dart'; ...@@ -18,7 +18,7 @@ import '../base/user_messages.dart';
import '../base/utils.dart'; import '../base/utils.dart';
import '../build_info.dart'; import '../build_info.dart';
import '../build_system/build_system.dart'; import '../build_system/build_system.dart';
import '../build_system/targets/common.dart' show kExtraFrontEndOptions, kExtraGenSnapshotOptions; // for "useLegacyNames" only import '../build_system/targets/common.dart' show kExtraFrontEndOptions, kExtraGenSnapshotOptions; // for deprecated option name aliases only
import '../bundle.dart' as bundle; import '../bundle.dart' as bundle;
import '../cache.dart'; import '../cache.dart';
import '../dart/generate_synthetic_packages.dart'; import '../dart/generate_synthetic_packages.dart';
...@@ -527,9 +527,10 @@ abstract class FlutterCommand extends Command<void> { ...@@ -527,9 +527,10 @@ abstract class FlutterCommand extends Command<void> {
valueHelp: 'x.y.z'); valueHelp: 'x.y.z');
} }
void usesDartDefineOption({ bool useLegacyNames = false }) { void usesDartDefineOption() {
argParser.addMultiOption( argParser.addMultiOption(
useLegacyNames ? kDartDefines : FlutterOptions.kDartDefinesOption, FlutterOptions.kDartDefinesOption,
aliases: <String>[ kDartDefines ], // supported for historical reasons
help: 'Additional key-value pairs that will be available as constants ' help: 'Additional key-value pairs that will be available as constants '
'from the String.fromEnvironment, bool.fromEnvironment, int.fromEnvironment, ' 'from the String.fromEnvironment, bool.fromEnvironment, int.fromEnvironment, '
'and double.fromEnvironment constructors.\n' 'and double.fromEnvironment constructors.\n'
...@@ -700,15 +701,17 @@ abstract class FlutterCommand extends Command<void> { ...@@ -700,15 +701,17 @@ abstract class FlutterCommand extends Command<void> {
/// Enables support for the hidden options --extra-front-end-options and /// Enables support for the hidden options --extra-front-end-options and
/// --extra-gen-snapshot-options. /// --extra-gen-snapshot-options.
void usesExtraDartFlagOptions({ @required bool verboseHelp, bool useLegacyNames = false }) { void usesExtraDartFlagOptions({ @required bool verboseHelp }) {
argParser.addMultiOption(useLegacyNames ? kExtraFrontEndOptions : FlutterOptions.kExtraFrontEndOptions, argParser.addMultiOption(FlutterOptions.kExtraFrontEndOptions,
aliases: <String>[ kExtraFrontEndOptions ], // supported for historical reasons
help: 'A comma-separated list of additional command line arguments that will be passed directly to the Dart front end. ' help: 'A comma-separated list of additional command line arguments that will be passed directly to the Dart front end. '
'For example, "--${FlutterOptions.kExtraFrontEndOptions}=--enable-experiment=nonfunction-type-aliases".', 'For example, "--${FlutterOptions.kExtraFrontEndOptions}=--enable-experiment=nonfunction-type-aliases".',
valueHelp: '--foo,--bar', valueHelp: '--foo,--bar',
splitCommas: true, splitCommas: true,
hide: !verboseHelp, hide: !verboseHelp,
); );
argParser.addMultiOption(useLegacyNames ? kExtraGenSnapshotOptions : FlutterOptions.kExtraGenSnapshotOptions, argParser.addMultiOption(FlutterOptions.kExtraGenSnapshotOptions,
aliases: <String>[ kExtraGenSnapshotOptions ], // supported for historical reasons
help: 'A comma-separated list of additional command line arguments that will be passed directly to the Dart native compiler. ' help: 'A comma-separated list of additional command line arguments that will be passed directly to the Dart native compiler. '
'(Only used in "--profile" or "--release" builds.) ' '(Only used in "--profile" or "--release" builds.) '
'For example, "--${FlutterOptions.kExtraGenSnapshotOptions}=--no-strip".', 'For example, "--${FlutterOptions.kExtraGenSnapshotOptions}=--no-strip".',
......
...@@ -254,7 +254,7 @@ void main() { ...@@ -254,7 +254,7 @@ void main() {
await expectLater( await expectLater(
createTestCommandRunner(command).run(<String>['attach', '--ipv6']), createTestCommandRunner(command).run(<String>['attach', '--ipv6']),
throwsToolExit( throwsToolExit(
message: 'When the --debug-port or --debug-uri is unknown, this command determines ' message: 'When the --debug-port or --debug-url is unknown, this command determines '
'the value of --ipv6 on its own.', 'the value of --ipv6 on its own.',
), ),
); );
...@@ -277,7 +277,7 @@ void main() { ...@@ -277,7 +277,7 @@ void main() {
await expectLater( await expectLater(
createTestCommandRunner(command).run(<String>['attach', '--observatory-port', '100']), createTestCommandRunner(command).run(<String>['attach', '--observatory-port', '100']),
throwsToolExit( throwsToolExit(
message: 'When the --debug-port or --debug-uri is unknown, this command does not use ' message: 'When the --debug-port or --debug-url is unknown, this command does not use '
'the value of --observatory-port.', 'the value of --observatory-port.',
), ),
); );
......
...@@ -29,12 +29,12 @@ void main() { ...@@ -29,12 +29,12 @@ void main() {
}; };
await expectLater(() => createTestCommandRunner(ScreenshotCommand()) await expectLater(() => createTestCommandRunner(ScreenshotCommand())
.run(<String>['screenshot', '--type=skia', '--observatory-uri=http://localhost:8181']), .run(<String>['screenshot', '--type=skia', '--observatory-url=http://localhost:8181']),
throwsA(isA<Exception>().having((dynamic exception) => exception.toString(), 'message', contains('dummy'))), throwsA(isA<Exception>().having((dynamic exception) => exception.toString(), 'message', contains('dummy'))),
); );
await expectLater(() => createTestCommandRunner(ScreenshotCommand()) await expectLater(() => createTestCommandRunner(ScreenshotCommand())
.run(<String>['screenshot', '--type=rasterizer', '--observatory-uri=http://localhost:8181']), .run(<String>['screenshot', '--type=rasterizer', '--observatory-url=http://localhost:8181']),
throwsA(isA<Exception>().having((dynamic exception) => exception.toString(), 'message', contains('dummy'))), throwsA(isA<Exception>().having((dynamic exception) => exception.toString(), 'message', contains('dummy'))),
); );
}); });
...@@ -61,7 +61,7 @@ void main() { ...@@ -61,7 +61,7 @@ void main() {
testUsingContext('device screenshots cannot provided Observatory', () async { testUsingContext('device screenshots cannot provided Observatory', () async {
await expectLater(() => createTestCommandRunner(ScreenshotCommand()) await expectLater(() => createTestCommandRunner(ScreenshotCommand())
.run(<String>['screenshot', '--observatory-uri=http://localhost:8181']), .run(<String>['screenshot', '--observatory-url=http://localhost:8181']),
throwsToolExit(message: 'Observatory URI cannot be provided for screenshot type device'), throwsToolExit(message: 'Observatory URI cannot be provided for screenshot type device'),
); );
}); });
......
...@@ -152,7 +152,8 @@ void main() { ...@@ -152,7 +152,8 @@ void main() {
expect(versionInfo, containsPair('flutterRoot', isNotNull)); expect(versionInfo, containsPair('flutterRoot', isNotNull));
}); });
testWithoutContext('A tool exit is thrown for an invalid debug-uri in flutter attach', () async { testWithoutContext('A tool exit is thrown for an invalid debug-url in flutter attach', () async {
// This test is almost exactly like the next one; update them together please.
final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', 'flutter'); final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', 'flutter');
final String helloWorld = fileSystem.path.join(getFlutterRoot(), 'examples', 'hello_world'); final String helloWorld = fileSystem.path.join(getFlutterRoot(), 'examples', 'hello_world');
final ProcessResult result = await processManager.run(<String>[ final ProcessResult result = await processManager.run(<String>[
...@@ -162,11 +163,29 @@ void main() { ...@@ -162,11 +163,29 @@ void main() {
'attach', 'attach',
'-d', '-d',
'flutter-tester', 'flutter-tester',
'--debug-uri=http://127.0.0.1:3333*/', '--debug-url=http://127.0.0.1:3333*/',
], workingDirectory: helloWorld); ], workingDirectory: helloWorld);
expect(result.exitCode, 1); expect(result.exitCode, 1);
expect(result.stderr, contains('Invalid `--debug-uri`: http://127.0.0.1:3333*/')); expect(result.stderr, contains('Invalid `--debug-url`: http://127.0.0.1:3333*/'));
});
testWithoutContext('--debug-uri is an alias for --debug-url', () async {
// This text is exactly the same as the previous one but with a "l" turned to an "i".
final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', 'flutter');
final String helloWorld = fileSystem.path.join(getFlutterRoot(), 'examples', 'hello_world');
final ProcessResult result = await processManager.run(<String>[
flutterBin,
...getLocalEngineArguments(),
'--show-test-device',
'attach',
'-d',
'flutter-tester',
'--debug-uri=http://127.0.0.1:3333*/', // "uri" not "url"
], workingDirectory: helloWorld);
expect(result.exitCode, 1);
expect(result.stderr, contains('Invalid `--debug-url`: http://127.0.0.1:3333*/')); // _"url"_ not "uri"!
}); });
testWithoutContext('will load bootstrap script before starting', () async { testWithoutContext('will load bootstrap script before starting', () async {
......
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