Add --dart-define option #44083

yjbanov · 2019-11-03T23:48:19Z

Add a --dart-define option that forwards the values as -D options to the underlying Dart compiler.

fluttergithubbot · 2019-11-03T23:48:21Z

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.

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

vsmenon · 2019-11-04T20:45:52Z

This might not work with DDC as we're building the SDK (dart_sdk.js) from ninja, and not flutter tool.

This means config options in dart:* files (which I think includes web skia) wouldn't pick this up.

If @jonahwilliams 's incremental approach builds the SDK, we might get this for free when we move to that.

fyi - @jakemac53

yjbanov · 2019-11-04T20:52:31Z

@vsmenon I'm adding it as an advanced hidden option for users who know what they are doing. So I'm not too worried that this doesn't work consistently. The bar is much lower than that used for standard documented options.

jakemac53 · 2019-11-04T20:52:53Z

This likely will work with dart2js since that compiles the whole world including the SDK.

But it won't work in DDC for variables that are used inside of the sdk since dart_sdk.js is precompiled without these options. It doesn't look like this was trying to plumb anything through for DDC anyways though (at least not via build_runner).

jonahwilliams · 2019-11-04T21:07:21Z

This actually needs to allow for bool, int, double, or String values. The syntax could be something like Bar=true,baz=foo which gets turned into -DBar=true,-Dbaz=foo

jakemac53 · 2019-11-04T21:19:15Z

This actually needs to allow for bool, int, double, or String values.

All the actual compiler apis talk in terms of a Map<String, String> though afaik. The specialized fromEnvironment constructors handle the parsing so you can always work in terms of strings.

jonahwilliams · 2019-11-04T21:25:55Z

Yeah, on the command line and tool it should be strings, but as written this only allows specifying which defines you want to be true. See https://github.com/flutter/flutter/pull/44083/files#diff-6edde590c56c73e35381fb97d37eb862R153

jonahwilliams · 2019-11-05T00:11:18Z

packages/flutter_tools/lib/src/build_system/build_system.dart

@@ -369,6 +372,8 @@ class Environment {
  /// which prevents the config from leaking into different builds.
  final Map<String, String> defines;

+  final List<String> dartDefines;


This should be a key under defines so that rebuilds are correct

jonahwilliams · 2019-11-05T00:13:07Z

in lib/src/compile.dart, the KernelCompiler should be updated to take a list of dart defines as well. in build_system/targets/dart.dart (I know, I need to rename it), this can be passed through to the kernel compiler invocation to make this work on mobile/desktop too

vsmenon · 2019-11-05T20:18:22Z

btw, @sigmundch mentioned we may have similar issues with dart2js using a pre-compiled (i.e., ninja generated) dill file for the SDK.

yjbanov · 2019-11-05T23:15:03Z

PTAL, but heads up, I'm unable to run all tools tests locally for some reason. So if there are test failures, it may require a few Cirrus runs to make everything green.

CaseyHillers

Some small nits

CaseyHillers · 2019-11-05T23:21:32Z

packages/flutter_tools/lib/src/build_runner/web_fs.dart

@@ -111,6 +113,7 @@ class WebFs {
  final String _target;
  final BuildInfo _buildInfo;
  final bool _initializePlatform;
+  final List<String> dartDefines;


Should we make dartDefines private here to match with everything else?

Good catch! Done.

CaseyHillers · 2019-11-05T23:22:05Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

@@ -130,6 +132,13 @@ class Dart2JSTarget extends Target {
      ? PackageMap.globalGeneratedPackagesPath
      : PackageMap.globalPackagesPath;
    final File outputFile = environment.buildDir.childFile('main.dart.js');
+
+    // Dart defines are encoded as a JSON array, so we need to parse it. Also JSON


nit: Also, JSON

jonahwilliams · 2019-11-06T16:26:26Z

packages/flutter_tools/test/commands.shard/hermetic/run_test.dart

@@ -263,3 +330,39 @@ class FakeDevice extends Fake implements Device {
    return null;
  }
 }
+
+class MockFeatureFlags extends Mock implements FeatureFlags {


Use TestFeatureFlags

jonahwilliams · 2019-11-06T16:39:53Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

+
+    // Dart defines are encoded as a JSON array, so we need to parse it. Also JSON
+    // parser returns List<dynamic>, so we need to cast it.
+    final List<String> dartDefines = environment.defines.containsKey(kDartDefines)


catch the parse exception and rethrow with an error message

A parser exception is not possible, is it?

flutter assemble -dDartDefines=[23, would throw I believe

Ah, gotcha. Fixing.

jonahwilliams · 2019-11-06T16:40:16Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

@@ -130,6 +131,13 @@ class Dart2JSTarget extends Target {
      ? PackageMap.globalGeneratedPackagesPath
      : PackageMap.globalPackagesPath;
    final File outputFile = environment.buildDir.childFile('main.dart.js');
+
+    // Dart defines are encoded as a JSON array, so we need to parse it. Also JSON
+    // parser returns List<dynamic>, so we need to cast it.


You don't actually need to cast it, since you are only calling toString treat it as a List<Object>

Edited: github hit my <Object>

jonahwilliams · 2019-11-06T17:29:48Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

+      try {
+        dartDefines = jsonDecode(dartDefinesJson);
+      } on FormatException catch (_) {
+        throw Exception(


Does the build system define a more specific exception type that this could be translated to instead?

Since this is only used in a single place, I would leave it as is for now.

jonahwilliams · 2019-11-06T17:31:13Z

Could you also provide the defines to the dart kernel compiler? It might not be plumbed through gradle or xcode yet, but that would be a much smaller change. Otherwise document that it only works for web release builds

yjbanov · 2019-11-07T18:35:28Z

Could you also provide the defines to the dart kernel compiler?

Done: fddf1c5

jonahwilliams · 2019-11-08T22:36:06Z

Could you file issues for plumbing this setting through gradle and xcode? Again, not necessary to do as part of this PR.

yjbanov · 2019-11-08T23:40:41Z

Could you file issues for plumbing this setting through gradle and xcode? Again, not necessary to do as part of this PR.

Done: #44483

LMK what I should do next with this PR.

yjbanov · 2019-11-08T23:43:27Z

BTW, take a look at this change: https://github.com/flutter/flutter/pull/44083/files#diff-7924f19d4eafce28aee7d180926625a0

I did it because when an integration test failed due to a command error the test would get stuck forever. The change catches premature process exits and fails the test immediately.

I hope I didn't mess anything up.

jonahwilliams

LGTM

jonahwilliams · 2019-11-08T23:52:13Z

Seems like a reasonable change

yjbanov · 2019-11-10T01:08:46Z

Build failure is infra failure. Landing on red.

darrinps · 2020-04-28T16:41:29Z

Don't mean to violate protocol commenting on a closed issue but this seemed like the best place to do so.

As this was merged in, I would expect it to be working in at least dev, but I am not able to get it to work as expected. I set the channel to dev (flutter channel dev), then did an upgrade (flutter upgrade). Then I issued the following:
flutter -run -t lib/main_generic.dart --dart-define=ENV=DEV

In my main_generic.dart I do this:
var env = String.fromEnvironment("ENV", defaultValue: "PROD");

The env variable is always returns PROD.

Am I doing something wrong?

jonahwilliams · 2020-04-28T16:48:05Z

A closed PR is not the right place for a bug report. If you file an issue with all of the relevant details (flutter doctor -v please) then we might be able to help

jonahwilliams · 2020-04-28T16:48:35Z

please cc me and I'll take a look

yjbanov added the work in progress; do not review label Nov 3, 2019

yjbanov requested a review from jonahwilliams November 3, 2019 23:48

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 3, 2019

googlebot added the cla: yes label Nov 3, 2019

yjbanov mentioned this pull request Nov 4, 2019

Forward experimental Skia for web environment variable to engine #44140

Closed

9 tasks

jonahwilliams reviewed Nov 5, 2019

View reviewed changes

yjbanov force-pushed the dart-defines branch from 047ebb3 to b58332f Compare November 5, 2019 23:12

yjbanov requested review from jonahwilliams and CaseyHillers November 5, 2019 23:13

CaseyHillers reviewed Nov 5, 2019

View reviewed changes

jonahwilliams reviewed Nov 6, 2019

View reviewed changes

jonahwilliams mentioned this pull request Nov 6, 2019

Add --web-define flag to flutter build web #44232

Closed

9 tasks

jonahwilliams reviewed Nov 6, 2019

View reviewed changes

yjbanov removed the work in progress; do not review label Nov 6, 2019

jonahwilliams reviewed Nov 6, 2019

View reviewed changes

yjbanov force-pushed the dart-defines branch 2 times, most recently from 9fd2d58 to 9c71e61 Compare November 8, 2019 05:02

yjbanov force-pushed the dart-defines branch from 9c71e61 to 3abd78f Compare November 8, 2019 22:29

yjbanov mentioned this pull request Nov 8, 2019

Plumb "DartDefines` through gradle and xcode #44483

Closed

jonahwilliams approved these changes Nov 8, 2019

View reviewed changes

add --dart-defines option

ea59922

yjbanov force-pushed the dart-defines branch from 3abd78f to ea59922 Compare November 9, 2019 23:59

yjbanov added the waiting for tree to go green label Nov 10, 2019

yjbanov merged commit e7073f9 into flutter:master Nov 10, 2019

CaseyHillers mentioned this pull request Nov 12, 2019

Flutter tool should allow passing environment variables for web #44226

Closed

mdebbar changed the title ~~Add --dart-defines option~~ Add --dart-define option Nov 19, 2019

darrinps mentioned this pull request Apr 28, 2020

String.fromEnvironment without a const silently does the wrong thing in the VM #55870

Open

DanTup mentioned this pull request Jun 1, 2020

Display structured errors immediately flutter/flutter-intellij#4587

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Add --dart-define option #44083

Add --dart-define option #44083

Uh oh!

Conversation

yjbanov commented Nov 3, 2019 • edited by mdebbar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fluttergithubbot commented Nov 3, 2019

Uh oh!

vsmenon commented Nov 4, 2019

Uh oh!

yjbanov commented Nov 4, 2019

Uh oh!

jakemac53 commented Nov 4, 2019

Uh oh!

jonahwilliams commented Nov 4, 2019

Uh oh!

jakemac53 commented Nov 4, 2019

Uh oh!

jonahwilliams commented Nov 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 5, 2019

Uh oh!

vsmenon commented Nov 5, 2019

Uh oh!

yjbanov commented Nov 5, 2019

Uh oh!

CaseyHillers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Nov 6, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 6, 2019

Uh oh!

yjbanov commented Nov 7, 2019

Uh oh!

jonahwilliams commented Nov 8, 2019

Uh oh!

yjbanov commented Nov 8, 2019

Uh oh!

yjbanov commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

yjbanov commented Nov 3, 2019 •

edited by mdebbar

Loading

jonahwilliams Nov 6, 2019 •

edited

Loading

yjbanov commented Nov 8, 2019 •

edited

Loading

darrinps commented Apr 28, 2020 •

edited

Loading