[flutter_tools] add --dart-define option for fuchsia build by michaellee8 · Pull Request #55715 · flutter/flutter

michaellee8 · 2020-04-27T04:08:09Z

Description

add ability to use --dart-define for fuchsia build

Related Issues

Didn't open an issue for trivial changes.

Tests

I added the following tests:

Didn't add tests for trivial changes

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

fluttergithubbot · 2020-04-27T04:08:11Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

michaellee8 · 2020-04-27T04:09:41Z

@jonahwilliams @zanderso please have a look at this. This is just a trivial change so that I can set -Ddart.vm.product=true

jonahwilliams · 2020-04-27T05:06:02Z

Ddart.vm.product=true should only be set by the flutter tooling when we're targeting release mode. Is this not working correctly?

jonahwilliams · 2020-04-27T05:07:08Z

Also - yes we should add dart-defines! Thanks for getting started with the PR here.

It does need to be tested, let me take a look at the existing build_fuchsia code and get back to you

jonahwilliams · 2020-04-27T05:09:40Z

Ahh, just looked - seems we're not passing the same arguments to the fuchsia kernel compiler that we are to the others:

flutter/packages/flutter_tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart

Line 73 in ba57ea0

if (buildInfo.mode.isRelease) '-Ddart.vm.release=true',

The fix for release mode would probably be to change this line to dart.vm.product instead of release

jonahwilliams · 2020-04-27T05:14:15Z

This is also where you would need to add the dart-defines from the buildInfo class. Something like:

for (final Object dartDefine in dartDefines)
  '-D$dartDefine',

In the list of command line arguments

michaellee8 · 2020-04-27T05:45:39Z

Oops, but it seems that there are no arguments passed to the kernel compiler command here

[  +51 ms] Building Fuchsia application...
[   +1 ms] executing: /home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/dart-sdk/bin/dart
/home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/artifacts/flutter_runner/flutter/arm64/release/aot/dart_binaries/kernel_compiler.snapshot --target flutter_runner --platform
/home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/artifacts/flutter_runner/flutter/arm64/release/aot/flutter_runner_patched_sdk/platform_strong.dill --filesystem-scheme main-root --filesystem-root
/home/michaellee8/flutter_gallery --packages main-root:///.packages --output build/fuchsia/flutter_gallery.dil --component-name flutter_gallery --aot --tfa --no-embed-sources -Ddart.vm.release=true
-Ddart.developer.causal_async_stacks=false main-root:///lib/main.dart

michaellee8 · 2020-04-27T05:54:14Z

Oops, but it seems that there are no arguments passed to the kernel compiler command here

[  +51 ms] Building Fuchsia application...
[   +1 ms] executing: /home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/dart-sdk/bin/dart
/home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/artifacts/flutter_runner/flutter/arm64/release/aot/dart_binaries/kernel_compiler.snapshot --target flutter_runner --platform
/home/michaellee8/flutter_fuchsia_toolchain/flutter/bin/cache/artifacts/flutter_runner/flutter/arm64/release/aot/flutter_runner_patched_sdk/platform_strong.dill --filesystem-scheme main-root --filesystem-root
/home/michaellee8/flutter_gallery --packages main-root:///.packages --output build/fuchsia/flutter_gallery.dil --component-name flutter_gallery --aot --tfa --no-embed-sources -Ddart.vm.release=true
-Ddart.developer.causal_async_stacks=false main-root:///lib/main.dart

Oh my bad here, I thought that -Ddart.vm.release=true is due to my setting.

jonahwilliams · 2020-04-27T07:08:15Z

The arguments are the flag list here:

flutter/packages/flutter_tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart

Line 51 in ba57ea0

List<String> flags = <String>[

… HEAD

michaellee8 · 2020-04-27T07:27:07Z

@jonahwilliams I wrote the code to add those flags into the kernel compiler, also refactored it to allow easier testing. Please kindly have a look at it.

michaellee8 · 2020-04-27T14:12:10Z

I think the remaining test does not have much to do with my patch? Are there any further problem?

jonahwilliams · 2020-04-27T16:33:38Z


    testUsingContext('parse device-finder output', () async {
-      const String example = '2001:0db8:85a3:0000:0000:8a2e:0370:7334 paper-pulp-bush-angel';
+      const String example =


Could you revert the formatting changes in this file?

jonahwilliams · 2020-04-27T16:34:04Z

+        '--drop-ast',
+      ],
+
+      for (final Object dartDefine in buildInfo.dartDefines) '-D$dartDefine',


I would move '-D$dartDefine', to the next line

jonahwilliams · 2020-04-27T16:36:20Z

  }
+
+  /// Provide flags that are affected by [BuildInfo]
+  List<String> getBuildInfoFlags({


I would make this method static and annotate it with @visibleForTesting from package:meta/meta.dart

michaellee8 · 2020-04-27T17:04:07Z

@jonahwilliams Please have a look at this, I have apply the fixes as requested except the contains one.

jonahwilliams · 2020-04-27T17:05:51Z

+import '../../src/common.dart';
+
+void main() {
+  group('Fuchsia Kernel Compiler', () {


Prefer 2 space indents in this file

I don't understand this one, what should I do? It should be 2 space indents already.

Co-Authored-By: Jonah Williams <[email protected]>

jonahwilliams

LGTM with a minor formatting nit

FYI @zanderso

jonahwilliams · 2020-04-27T17:06:37Z

Thank you for fixing this @michaellee8 !

…el_compiler_test.dart Co-Authored-By: Jonah Williams <[email protected]>

michaellee8 · 2020-04-27T17:21:20Z

Umm are there any formatter config I can use? I just use Crtl+Shift+I in VS Code which should be using dartfmt.

jonahwilliams · 2020-04-27T17:28:37Z

Unfortunately we don't use autoformatting for the flutter/flutter repo. The style guide is outlined here: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

But general, format in way that is readable, don't go too long, and intent things 2 spaces instead of 4

michaellee8 · 2020-04-27T17:32:12Z

@jonahwilliams Oh i can get it. Are there any problems left?

…el_compiler_test.dart Co-Authored-By: Jonah Williams <[email protected]>

… patch-3

michaellee8 · 2020-04-27T18:03:26Z

Hey please don't merge this right now. Let me revert all the bad things caused by auto formatter first.

jonahwilliams

Still LGTM

michaellee8 · 2020-04-27T19:43:25Z

All damages are cleaned and looks good to merge now. LGTM

[flutter_tools] add --dart-define option for fuchsia build

3240932

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 27, 2020

googlebot added the cla: yes label Apr 27, 2020

jonahwilliams self-requested a review April 27, 2020 05:07

michaellee8 added 2 commits April 27, 2020 15:19

[flutter_tools] fix fuchsia build dart define missing

fdd547a

Merge branch 'patch-3' of https://github.com/michaellee8/flutter into…

9aa5f6f

… HEAD

michaellee8 added 2 commits April 27, 2020 15:56

fix: remove package:test for breaking tests

c629bd9

fix: add stub for mock

a09db2f