[tool] Proposal to multiple defines for --dart-define-from-file by ronnnnn · Pull Request #120878 · flutter/flutter

ronnnnn · 2023-02-16T12:50:49Z

Overview

Make --dart-define-from-file option define multiple times.

Use case

In case of changing values for each environment of flavors and build types, multi --dart-define-from-file is needed.
For example,

// development x debug
flutter build apk --dart-define-from-file=define/flavor/development/android.json --dart-define-from-file=define/flavor/debug/android.json`.

// production x debug
flutter build apk --dart-define-from-file=define/flavor/production/android.json --dart-define-from-file=define/flavor/debug/android.json`.

packages
└ app
　 └ define
　 　 ├ build
　 　 │ ├ debug
　 　 │ │ ├ android.json
　 　 │ │ └ ios.json
　 　 │ └ release
　 　 │ 　 ├ android.json
　 　 │ 　 └ ios.json
　 　 └ flavor
　 　 　 ├ development
　 　 　 │ ├ android.json
　 　 　 │ └ ios.json
　 　 　 └ production
　 　 　 　 ├ android.json
　 　 　 　 └ ios.json

Links

[tool] Proposal to support dart define config from a json file #108098

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-02-16T13:01:03Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

christopherfujino · 2023-02-16T21:46:29Z

Thank you for contributing! You will need to get all CI tests (other than luci-flutter) passing before this will PR will be reviewed.

fix: accept multi options

ronnnnn · 2023-02-19T23:52:28Z

@christopherfujino
I have committed some additional changes and ready for reviews.

christopherfujino · 2023-02-22T19:26:26Z

packages/flutter_tools/lib/src/commands/assemble.dart

-          throwToolExit('Json config define file "--${FlutterOptions.kDartDefineFromFileOption}=$configJsonPath" format err, '
-              'please fix first! format err:\n$err');
+      final List<String> configJsonPaths = stringsArg(FlutterOptions.kDartDefineFromFileOption);
+      if (configJsonPaths.isNotEmpty && configJsonPaths.every((String path) => globals.fs.isFileSync(path))) {


in the case one of these paths is not actually a file, I think we should throwToolExit() with a message explaining exactly what the user did wrong (I know we didn't handle this previously, but I think this is important).

christopherfujino · 2023-02-22T20:10:45Z

packages/flutter_tools/lib/src/commands/assemble.dart

-        } on FormatException catch (err) {
-          throwToolExit('Json config define file "--${FlutterOptions.kDartDefineFromFileOption}=$configJsonPath" format err, '
-              'please fix first! format err:\n$err');
+      final List<String> configJsonPaths = stringsArg(FlutterOptions.kDartDefineFromFileOption);


This is also tech debt from a previous change, but extending this code highlights the fact that this code is not dry, and changes to this block of code also need to be made to flutter_command.dart. Can you instead move this code to a helper function in flutter_command.dart (since AssembleCommand extends FlutterCommand it should be accessible in this file) that parses both the --dart-define-from-file and --dart-define options and returns a single List<String> of dartDefines?

ronnnnn · 2023-02-24T09:50:00Z

@christopherfujino
Thank you for your reviews. I'll try to update them.

ronnnnn · 2023-02-27T02:20:45Z

@christopherfujino
I updated from your reviews. I created two helper functions to extract dart defines because flutter_command.dart needs defineConfigJsonMap as a parameter of BuildInfo. What do you think about it?

christopherfujino · 2023-02-27T20:15:16Z

packages/flutter_tools/test/commands.shard/permeable/build_bundle_test.dart

-    BuildSystem: () => TestBuildSystem.all(BuildResult(success: true), (Target target, Environment environment) {
-      expect(environment.defines[kDartDefines], 'a0ludD0x,a0RvdWJsZT0xLjE=,bmFtZT1kZW5naGFpemh1,dGl0bGU9dGhpcyBpcyB0aXRsZSBmcm9tIGNvbmZpZyBqc29uIGZpbGU=');
+    BuildSystem: () => TestBuildSystem.all(BuildResult(success: false), (Target target, Environment environment) {
+      expect(environment.defines[kDartDefines], 'a0ludD0x,a0RvdWJsZT0xLjE=,bmFtZT1kZW5naGFpemh1,dGl0bGU9dGhpcyBpcyB0aXRsZSBmcm9tIGNvbmZpZyBqc29uIGZpbGU=,Ym9keT10aGlzIGlzIGJvZHkgZnJvbSBjb25maWcganNvbiBmaWxl');


This is my first time seeing this test--what encoding is this?

Also, are you intentionally now testing that this fails?

These are encoded from dartDefines' key and value on encodeDartDefines in build_info.dart.
For example, kInt=1 is encoded to a0ludD0x.

I intended this test should success, this is my mistake. But, I found that assertions on this test file doesn't work as expected because of FakeBundleBuilder that do no-op on build function.
I'll investigate more and fix them to work correctly.

I fixed to use real BundleBuilder so that we can inject BuildSystem on build_bundle_test.dart and assert environment properties.
I checked tests work as expected and also fix flutter_command.dart.

Ahh, so base64. I'm skeptical this is the best way to assert here (given I had to decode it to code review), however, that can be a cleanup in a later PR.

christopherfujino

LGTM, thanks for this PR and the cleanup!

christopherfujino · 2023-03-03T01:47:28Z

@andrewkolos can you take another look and, if you approve, add autosubmit?

andrewkolos · 2023-03-06T00:33:02Z

I updated from upstream/master in an effort to unstuck the Google testing check.

andrewkolos

LGTM :)

…le (flutter/flutter#120878)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 16, 2023

ronnnnn marked this pull request as draft February 16, 2023 12:55

ronnnnn marked this pull request as ready for review February 16, 2023 13:08

ronnnnn marked this pull request as draft February 17, 2023 09:41

ronnnnn marked this pull request as ready for review February 17, 2023 09:51

ronnnnn added 6 commits February 17, 2023 18:57

faet: multiple defines for --dart-define-from-file

ebfc9c4

fix: accept multi options

refactor: fix lint

be065c1

test: fix test fail and add test for conflict

05d3b4e

feat: multiple defines for --dart-define-from-file on assemble command

f831f86

fix: conflict case

b5c5bac

update: merge branch 'master' into feat-multi-dart-define-from-file

d4372ed

Merge branch 'master' into feat-multi-dart-define-from-file

bd00514

christopherfujino reviewed Feb 22, 2023

View reviewed changes

ronnnnn added 5 commits February 25, 2023 13:06

Merge branch 'master' into feat-multi-dart-define-from-file

2cc9a93

Merge branch 'master' into feat-multi-dart-define-from-file

cd35e47

refactor: helper function for parsing dart defines

3271d85

update: check option value is a valid file type

3f3cbd4

fix: not throw if a option can't be found

bfcaf59

ronnnnn requested a review from christopherfujino February 27, 2023 02:17

christopherfujino reviewed Feb 27, 2023

View reviewed changes

fix: use real BundleBuilder for build bundle test

3a4bd11

ronnnnn requested a review from christopherfujino February 28, 2023 01:23

christopherfujino approved these changes Mar 3, 2023

View reviewed changes

christopherfujino requested a review from andrewkolos March 3, 2023 01:47

Merge branch 'master' into feat-multi-dart-define-from-file

27cd46e

andrewkolos approved these changes Mar 6, 2023

View reviewed changes

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 6, 2023

auto-submit bot merged commit acc840e into flutter:master Mar 6, 2023

engine-flutter-autoroll mentioned this pull request Mar 8, 2023

Manual roll Flutter from f2f80059ff87 to 5a279edc239f (44 revisions) flutter/packages#3410

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023

acc840e [tool] Proposal to multiple defines for --dart-define-from-fi…

4695704

…le (flutter/flutter#120878)

engine-flutter-autoroll mentioned this pull request Mar 8, 2023

Roll Flutter from f2f80059ff87 to 5a279edc239f (44 revisions) flutter/packages#3416

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023

acc840e [tool] Proposal to multiple defines for --dart-define-from-fi…

f158798

…le (flutter/flutter#120878)

engine-flutter-autoroll mentioned this pull request Mar 8, 2023

Roll Flutter from f2f80059ff87 to 5a279edc239f (44 revisions) flutter/packages#3418

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2023

acc840e [tool] Proposal to multiple defines for --dart-define-from-fi…

26ae810

…le (flutter/flutter#120878)

cyanglaz mentioned this pull request Mar 8, 2023

Manual roll Flutter from f2f80059ff87 to 5a279edc239f (44 revisions) + fixes flutter/packages#3420

Merged

engine-flutter-autoroll mentioned this pull request Mar 8, 2023

Roll Flutter from f2f80059ff87 to 0cf593e79261 (45 revisions) flutter/packages#3421

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

acc840e [tool] Proposal to multiple defines for --dart-define-from-fi…

4d9febf

…le (flutter/flutter#120878)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

acc840e [tool] Proposal to multiple defines for --dart-define-from-fi…

c697038

…le (flutter/flutter#120878)

Conversation

ronnnnn commented Feb 16, 2023 • edited by andrewkolos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Use case

Links

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 16, 2023

Uh oh!

christopherfujino commented Feb 16, 2023

Uh oh!

ronnnnn commented Feb 19, 2023

Uh oh!

christopherfujino Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

christopherfujino Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ronnnnn commented Feb 24, 2023

Uh oh!

ronnnnn commented Feb 27, 2023

Uh oh!

christopherfujino Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

christopherfujino Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

ronnnnn Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

ronnnnn Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

christopherfujino Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Mar 3, 2023

Uh oh!

andrewkolos commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ronnnnn commented Feb 16, 2023 •

edited by andrewkolos

Loading

christopherfujino left a comment •

edited

Loading

andrewkolos commented Mar 6, 2023 •

edited

Loading