Refactor and polish the 'felt' tool #12258

mdebbar · 2019-09-12T22:44:50Z

Various functionalities offered by this tool are now organized into commands (e.g. felt test, felt check-licenses).
The felt tool can now be invoked from anywhere, not necessarily from the web_ui directory.
This new structure helps us scale better as we add more commands (e.g. soon a build/watch command is coming).

lib/web_ui/dev/chrome_installer.dart

yjbanov · 2019-09-13T17:46:31Z

lib/web_ui/dev/chrome_installer.dart

  }
+
+  @override
+  String get name => 'chrome-installer';


Should we adopt verbs instead of nouns for command names? "install-chrome", "test", etc.

yjbanov · 2019-09-13T17:47:03Z

lib/web_ui/dev/felt

@@ -44,4 +44,4 @@ then
  ninja -C $HOST_DEBUG_UNOPT_DIR
 fi

-(cd $WEB_UI_DIR && $DART_SDK_DIR/bin/dart dev/felt.dart $@)
+($DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@)


It seems you don't need the parens any more

yjbanov · 2019-09-13T17:47:51Z

lib/web_ui/dev/test_runner.dart

-  _checkExitCode();
-}
+  @override
+  final String name = 'tests';


How about "test"?

yjbanov · 2019-09-13T17:48:03Z

lib/web_ui/dev/licenses.dart

+
+class LicensesCommand extends Command<bool> {
+  @override
+  final String name = 'licenses';


How about "check-licenses"?

Too long? 😛

I imagine it will be used infrequently. Instead, we'll probably have a felt presubmit that we'll encourage everyone to run before sending a PR.

I made the change anyway :)

yjbanov · 2019-09-13T17:49:50Z

lib/web_ui/dev/utils.dart

+import 'environment.dart';
+
+class FilePath {
+  FilePath.fromCwd(String filePath) : _path = path.absolute(filePath);


nit: I'd call it FilePath.relativeToCwd and rename filePath to relativePath.

In case you haven't noticed, there are getters called relativeToCwd and relativeToWebUi. If I rename the constructors, what should I call the getters, any idea?

Ah, nvm then. "From" seems to convey the meaning just fine.

lib/web_ui/dev/utils.dart

yjbanov · 2019-09-13T17:51:13Z

lib/web_ui/dev/utils.dart

+  FilePath.fromWebUi(String filePath)
+      : _path = path.join(environment.webUiRootDir.path, filePath);
+
+  final String _path;


Should this be called _absolutePath to be absolutely clear :)

yjbanov · 2019-09-13T17:51:53Z

lib/web_ui/dev/utils.dart

+
+  @override
+  bool operator ==(dynamic other) {
+    if (other is String) {


Hmm, not sure about this :/ Equality is supposed to be symmetric.

yjbanov · 2019-09-13T17:58:02Z

lib/web_ui/dev/felt

@@ -44,4 +44,4 @@ then
  ninja -C $HOST_DEBUG_UNOPT_DIR
 fi

-(cd $WEB_UI_DIR && $DART_SDK_DIR/bin/dart dev/felt.dart $@)
+($DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@)


Can you please add set -e at the top of the script so it doesn't keep on truckin' when intermediate commands fail?

mdebbar · 2019-09-13T20:21:14Z

lib/web_ui/lib/src/engine/surface/surface.dart

+      final List<String> validationErrors = <String>[];
+      scene.debugValidate(validationErrors);
+      if (validationErrors.isNotEmpty) {
+        print('ENGINE LAYER TREE INCONSISTENT:\n'


@yjbanov FYI this is spamming our test output so I'll skip it in test mode.

It is trying to surface a problem though. I was meaning to look into it.

yjbanov · 2019-09-13T22:01:46Z

lib/web_ui/dev/felt.dart

-      runner.printUsage();
-      io.exit(64); // Exit code 64 indicates a usage error.
-    } else if (!result) {
+    if (result == false) {
      print('Command returned false: `felt ${args.join(' ')}`');


Actually, it's not the felt ARGS that returned false but a sub-command, so more like Sub-command returned false: ${args.join(' '}

git@github.com:flutter/engine.git/compare/2c4ed36c60ae...8a8610a git log 2c4ed36..8a8610a --no-merges --oneline 2019-09-16 liyuqian@google.com Implement Base32Decode (flutter/engine#12253) 2019-09-16 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (#11817)" (flutter/engine#12280) 2019-09-16 mouad.debbar@gmail.com Refactor and polish the 'felt' tool (flutter/engine#12258) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

git@github.com:flutter/engine.git/compare/2c4ed36c60ae...8a8610a git log 2c4ed36..8a8610a --no-merges --oneline 2019-09-16 liyuqian@google.com Implement Base32Decode (flutter/engine#12253) 2019-09-16 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (flutter#11817)" (flutter/engine#12280) 2019-09-16 mouad.debbar@gmail.com Refactor and polish the 'felt' tool (flutter/engine#12258) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

Refactor and polish the 'felt' tool

fc092d2

googlebot added the cla: yes label Sep 12, 2019

Fix a few things caught by Yegor

e78aecd

mdebbar commented Sep 13, 2019

View reviewed changes

lib/web_ui/dev/chrome_installer.dart Outdated Show resolved Hide resolved

yjbanov reviewed Sep 13, 2019

View reviewed changes

lib/web_ui/dev/chrome_installer.dart Outdated Show resolved Hide resolved

yjbanov reviewed Sep 13, 2019

View reviewed changes

Few minor changes

36ab07d

mdebbar commented Sep 13, 2019

View reviewed changes

mdebbar requested a review from yjbanov September 13, 2019 20:21

Correct cirrus commands

7363f82

yjbanov reviewed Sep 13, 2019

View reviewed changes

yjbanov approved these changes Sep 13, 2019

View reviewed changes

Fix nits

50fbd10

mdebbar merged commit 968c3aa into flutter:master Sep 16, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2019

968c3aa Refactor and polish the 'felt' tool (flutter/engine#12258)

fccb9ee

engine-flutter-autoroll mentioned this pull request Sep 16, 2019

Roll engine 2c4ed36c60ae..8a8610a9a514 (3 commits) flutter/flutter#40616

Merged

mdebbar deleted the felt_commands branch September 17, 2019 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and polish the 'felt' tool #12258

Refactor and polish the 'felt' tool #12258

mdebbar commented Sep 12, 2019 •

edited

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

mdebbar Sep 13, 2019

yjbanov Sep 13, 2019

mdebbar Sep 13, 2019

yjbanov Sep 13, 2019

mdebbar Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

mdebbar Sep 13, 2019

yjbanov Sep 13, 2019

yjbanov Sep 13, 2019

Refactor and polish the 'felt' tool #12258

Refactor and polish the 'felt' tool #12258

Conversation

mdebbar commented Sep 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdebbar commented Sep 12, 2019 •

edited