Skip to content

Conversation

hai046
Copy link
Contributor

@hai046 hai046 commented Jul 21, 2022

  • Add flag --dart-define-from-file
  • Add flag --enable-dart-define-from-file-raw-value [delete]
  • Rename all raw json value name to dart_define_from_file_raw_values [delete]
  • Test web and macOS desktop can be use from const String.fromEnvironment('filed_xxx');

Supplementary modification code

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 flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 21, 2022
@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@hai046 hai046 changed the title Fea json config [tool] Proposal to support dart define config from a json file Jul 21, 2022
@christopherfujino christopherfujino requested review from christopherfujino and removed request for christopherfujino August 4, 2022 20:41
@christopherfujino
Copy link
Contributor

cc @Jasguerrero & @eliasyishak

@eliasyishak
Copy link
Contributor

Additionally, can you add some test cases for this new feature? Here is link to our wiki on writing tests: https://github.com/flutter/flutter/wiki/Running-and-writing-tests

@hai046
Copy link
Contributor Author

hai046 commented Aug 17, 2022

Additionally, can you add some test cases for this new feature? Here is link to our wiki on writing tests: https://github.com/flutter/flutter/wiki/Running-and-writing-tests

There are too many classifications in this test case file. It already belongs to him. I need to study it carefully, and then add it.

@hai046
Copy link
Contributor Author

hai046 commented Sep 19, 2022

I have updated it, please review it, thanks

Copy link
Contributor Author

@hai046 hai046 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I listed the variables one by one to judge the priority, please take a look, thanks!

@@ -580,7 +582,7 @@ abstract class FlutterCommand extends Command<void> {
valueHelp: 'x.y.z');
}

void usesDartDefineOption() {
void usesDartDefineOption({ bool enableDartDefineConfigJsonFileOption = true}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jasguerrero OK,i add it.

But I am a little confused, I think this command may be is bug? or he must rely on the previous command

1、if run assemble with dart-define options is error by current flutter version

flutter assemble --dart-define=k1=v1 -o Output debug_ios_bundle_flutter_assets

Error parsing assemble command: your generated configuration may be out of date. Try re-running 'flutter build ios' or the appropriate build command.

i want use encodeDartDefines but he has dedicated test cases testUsingContext('flutter assemble throws ToolExit if dart-defines are not base64 encoded', ,this description and implementation are somewhat ambiguous

2、The source code file assemble.dart

  @override
  Future<FlutterCommandResult> runCommand() async {
    final List<Target> targets = createTargets();
    final List<Target> nonDeferredTargets = <Target>[];
    final List<Target> deferredTargets = <AndroidAotDeferredComponentsBundle>[];
    for (final Target target in targets) {
      if (deferredComponentsTargets.contains(target.name)) {
        deferredTargets.add(target);
      } else {
        nonDeferredTargets.add(target);
      }
    }
    Target? target;
    List<String> decodedDefines;
    try {
      decodedDefines = decodeDartDefines(environment.defines, kDartDefines);
    } on FormatException {
      throwToolExit(
        'Error parsing assemble command: your generated configuration may be out of date. '
        "Try re-running 'flutter build ios' or the appropriate build command."
      );
    }

The code decodedDefines = decodeDartDefines(environment.defines, kDartDefines); is decode but you can't find where to code

Does this depend on the previous command, because the previous command is encoded ???

So I always feel that the implementation of flutter assemble is not good enough, or it is implemented separately compared to other operations such as build apk ios bundle ipa,
For example reference variable FlutterOptions.kDartDefinesOption only 2 place references

  • AssembleComand#_parseDefines [private method]
  • FlutterCommend#getBuildInfo [all other build/run cmd]

'CODE_SIZE_DIRECTORY'
];
for (final String key in systemVars) {
if(result.remove(key) != null){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this remove, the past implementation you had in which you iterate over result and make an dartDefineConfigJsonMap.containsKey is the way to go. I understand that you weren't able to reproduce the dart define but you can mock the BuildInfo and send the dart defines there to encode

Copy link
Contributor Author

@hai046 hai046 Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK,i changed it another way, I don't know if it works @Jasguerrero

@hai046
Copy link
Contributor Author

hai046 commented Sep 22, 2022

OK, i change it

return <String, String>{
final Map<String, String> result = <String, String>{};
if (dartDefineConfigJsonMap != null) {
final List<String> systemVars=<String>[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do not understand why you need this new variable systemVars if you just compare what the user send on the config file and what you already have on dartDefineConfigJsonMap wouldn't be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood, I thought you let me judge completely if the variable is duplicated. Because the above variable is conditional, if the condition is not satisfied, there is no variable, and I just set the above variable, which will cause coverage

@hai046
Copy link
Contributor Author

hai046 commented Sep 23, 2022

I'm so sorry, I'm overthinking it @Jasguerrero

Copy link
Contributor

@Jasguerrero Jasguerrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing this!

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022
@auto-submit auto-submit bot merged commit cbfe5a5 into flutter:master Sep 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants