-
Notifications
You must be signed in to change notification settings - Fork 29.2k
[tool] Proposal to support dart define config from a json file #108098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2、json raw value named dart-define-from-file-raw-values
Fea json config
merge master
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. |
cc @Jasguerrero & @eliasyishak |
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. |
I have updated it, please review it, thanks |
sync flutter code
Roll Flutter Engine from 6aa226bbce70 to adab20ec9ce8 (1 revision) (#…
There was a problem hiding this 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!
packages/flutter_tools/test/commands.shard/permeable/build_bundle_test.dart
Show resolved
Hide resolved
@@ -580,7 +582,7 @@ abstract class FlutterCommand extends Command<void> { | |||
valueHelp: 'x.y.z'); | |||
} | |||
|
|||
void usesDartDefineOption() { | |||
void usesDartDefineOption({ bool enableDartDefineConfigJsonFileOption = true}) { |
There was a problem hiding this comment.
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){ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
OK, i change it |
return <String, String>{ | ||
final Map<String, String> result = <String, String>{}; | ||
if (dartDefineConfigJsonMap != null) { | ||
final List<String> systemVars=<String>[ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I'm so sorry, I'm overthinking it @Jasguerrero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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!
--dart-define-from-file
--enable-dart-define-from-file-raw-value
[delete]dart_define_from_file_raw_values
[delete]const String.fromEnvironment('filed_xxx');
Supplementary modification code
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.