Change defaults for build args to accommodate Flutter and Fuchsia.

Flutter and Fuchsia build stripped down [platform] version of dart sdk, so make it default(while standalone builds full dart sdk)

Both Flutter and Fuchsia expect stripped dart in standard out directory, so make that default too(while standalone build puts stripped into exe.stripped/).

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/1d50f112dcbf57aeb69739b855e5df93cfed168f

[aam, 2017-08-04 22:29:46]

aam@google.com changed reviewers:
+ rmacnak@google.com, zra@google.com

[aam, 2017-08-04 22:29:46]

PTAL, thanks!

[zra, 2017-08-04 22:40:41]

https://codereview.chromium.org/2998503002/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/2998503002/diff/1/build/config/BUILDCONFIG.gn...
build/config/BUILDCONFIG.gn:199: is_flutter = false
This is probably not the right place for this since Flutter isn't really an OS.
Instead, this should probably be a new GN argument under a new file, like
//build/flutter_args.gni (in case we need to add more Flutter-specific arguments
later.) Then you can import build/flutter_args.gni where you need is_flutter,
and not worry about saying (defined(is_flutter) && is_flutter) everywhere.

[aam, 2017-08-04 22:51:50]

On 2017/08/04 22:40:41, zra wrote:
> https://codereview.chromium.org/2998503002/diff/1/build/config/BUILDCONFIG.gn
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/2998503002/diff/1/build/config/BUILDCONFIG.gn...
> build/config/BUILDCONFIG.gn:199: is_flutter = false
> This is probably not the right place for this since Flutter isn't really an
OS.
> Instead, this should probably be a new GN argument under a new file, like
> //build/flutter_args.gni (in case we need to add more Flutter-specific
arguments
> later.) Then you can import build/flutter_args.gni where you need is_flutter,
> and not worry about saying (defined(is_flutter) && is_flutter) everywhere.

Okay, thanks. Is patch set 2 closer to what you have in mind?

[aam, 2017-08-07 16:19:01]

Description was changed from

==========
Similarly to is_fuchsia, introduce is_flutter flag.

This is needed to produce platform dart_sdk built with Flutter.

BUG=
==========

to

==========
Introduce is_flutter flag that is set for flutter build.

This is needed to produce platform dart_sdk built with Flutter.

BUG=
==========

[zra, 2017-08-07 22:11:14]

https://codereview.chromium.org/2998503002/diff/20001/build/flutter_args.gni
File build/flutter_args.gni (right):

https://codereview.chromium.org/2998503002/diff/20001/build/flutter_args.gni#...
build/flutter_args.gni:6: is_flutter = false
A comment above this line will display when someone runs the command, for
example 'gn args out/ReleaseX64 --list'

https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
File sdk/BUILD.gn (right):

https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
sdk/BUILD.gn:21: dart_platform_sdk = false
Sorry for not noticing this sooner: Since both Flutter and Fuchsia want the
platform SDK, and only the standalone Dart SDK build wants the full SDK, I think
it makes sense to switch the default sense of this flag to true. Then we can
pass 'false' by default from our //tools/gn.py (except for the architectures
listed below). This way, we can avoid the need to add an is_flutter GN argument.

https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode231
sdk/BUILD.gn:231: } else if (is_fuchsia || is_fuchsia_host || is_flutter) {
As with the platform_sdk flag, we should change this case to be the default with
a GN flag, like dart_stripped_binary_path = "". Then //tools/gn.py can override
it to "exe.stripped/".

[aam, 2017-08-07 23:19:07]

https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
File sdk/BUILD.gn (right):

https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
sdk/BUILD.gn:21: dart_platform_sdk = false
On 2017/08/07 22:11:14, zra wrote:
> Sorry for not noticing this sooner: Since both Flutter and Fuchsia want the
> platform SDK, and only the standalone Dart SDK build wants the full SDK, I
think
> it makes sense to switch the default sense of this flag to true. Then we can
> pass 'false' by default from our //tools/gn.py (except for the architectures
> listed below). This way, we can avoid the need to add an is_flutter GN
argument.

tools/gn.py pulls the value for this flag from environment variable
DART_MAKE_PLATFORM_SDK, which is then being overriden with command line argument
--platform-sdk. Changing the default will require renaming those to
DART_MAKE_FULL_SDK and --full-sdk respectively and updating the places where
they are (do we know where are they used? I was not able to find ) Is it worth
it?

[zra, 2017-08-08 17:02:39]

On 2017/08/07 23:19:07, aam wrote:
> https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
> File sdk/BUILD.gn (right):
> 
> https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
> sdk/BUILD.gn:21: dart_platform_sdk = false
> On 2017/08/07 22:11:14, zra wrote:
> > Sorry for not noticing this sooner: Since both Flutter and Fuchsia want the
> > platform SDK, and only the standalone Dart SDK build wants the full SDK, I
> think
> > it makes sense to switch the default sense of this flag to true. Then we can
> > pass 'false' by default from our //tools/gn.py (except for the architectures
> > listed below). This way, we can avoid the need to add an is_flutter GN
> argument.
> 
> tools/gn.py pulls the value for this flag from environment variable
> DART_MAKE_PLATFORM_SDK, which is then being overriden with command line
argument
> --platform-sdk. Changing the default will require renaming those to
> DART_MAKE_FULL_SDK and --full-sdk respectively and updating the places where
> they are (do we know where are they used? I was not able to find ) Is it worth
> it?

You can leave DART_MAKE_PLATFORM_SDK as-is, and in gn.py in ToGnArgs, you can
say something like:

if not args.platform_sdk and not gn_args['target_cpu'].startswith('arm'):
  gn_args['dart_platform_sdk'] = false

Then, in sdk/BUILD.gn, you change the default value for dart_platform_sdk from
false to true.

[aam, 2017-08-08 19:01:47]

On 2017/08/08 17:02:39, zra wrote:
> On 2017/08/07 23:19:07, aam wrote:
> > https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
> > File sdk/BUILD.gn (right):
> > 
> > https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
> > sdk/BUILD.gn:21: dart_platform_sdk = false
> > On 2017/08/07 22:11:14, zra wrote:
> > > Sorry for not noticing this sooner: Since both Flutter and Fuchsia want
the
> > > platform SDK, and only the standalone Dart SDK build wants the full SDK, I
> > think
> > > it makes sense to switch the default sense of this flag to true. Then we
can
> > > pass 'false' by default from our //tools/gn.py (except for the
architectures
> > > listed below). This way, we can avoid the need to add an is_flutter GN
> > argument.
> > 
> > tools/gn.py pulls the value for this flag from environment variable
> > DART_MAKE_PLATFORM_SDK, which is then being overriden with command line
> argument
> > --platform-sdk. Changing the default will require renaming those to
> > DART_MAKE_FULL_SDK and --full-sdk respectively and updating the places where
> > they are (do we know where are they used? I was not able to find ) Is it
worth
> > it?
> 
> You can leave DART_MAKE_PLATFORM_SDK as-is, and in gn.py in ToGnArgs, you can
> say something like:
> 
> if not args.platform_sdk and not gn_args['target_cpu'].startswith('arm'):
>   gn_args['dart_platform_sdk'] = false
> 
> Then, in sdk/BUILD.gn, you change the default value for dart_platform_sdk from
> false to true.

So in current set up it is possible to override environment setting
(DART_MAKE_PLATFORM_SDK=false) with command line argument(--platform-sdk), so
you end up with dart_platform_sdk=true.
With the change you are proposing you won't be able to override environment
setting with command line argument. So if you have DART_MAKE_PLATFORM_SDK=true,
there is no way for you to get dart_platform_sdk=false. I guess it's not that
critical?

[zra, 2017-08-08 19:53:06]

On 2017/08/08 19:01:47, aam wrote:
> On 2017/08/08 17:02:39, zra wrote:
> > On 2017/08/07 23:19:07, aam wrote:
> > > https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
> > > File sdk/BUILD.gn (right):
> > > 
> > >
https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
> > > sdk/BUILD.gn:21: dart_platform_sdk = false
> > > On 2017/08/07 22:11:14, zra wrote:
> > > > Sorry for not noticing this sooner: Since both Flutter and Fuchsia want
> the
> > > > platform SDK, and only the standalone Dart SDK build wants the full SDK,
I
> > > think
> > > > it makes sense to switch the default sense of this flag to true. Then we
> can
> > > > pass 'false' by default from our //tools/gn.py (except for the
> architectures
> > > > listed below). This way, we can avoid the need to add an is_flutter GN
> > > argument.
> > > 
> > > tools/gn.py pulls the value for this flag from environment variable
> > > DART_MAKE_PLATFORM_SDK, which is then being overriden with command line
> > argument
> > > --platform-sdk. Changing the default will require renaming those to
> > > DART_MAKE_FULL_SDK and --full-sdk respectively and updating the places
where
> > > they are (do we know where are they used? I was not able to find ) Is it
> worth
> > > it?
> > 
> > You can leave DART_MAKE_PLATFORM_SDK as-is, and in gn.py in ToGnArgs, you
can
> > say something like:
> > 
> > if not args.platform_sdk and not gn_args['target_cpu'].startswith('arm'):
> >   gn_args['dart_platform_sdk'] = false
> > 
> > Then, in sdk/BUILD.gn, you change the default value for dart_platform_sdk
from
> > false to true.
> 
> So in current set up it is possible to override environment setting
> (DART_MAKE_PLATFORM_SDK=false) with command line argument(--platform-sdk), so
> you end up with dart_platform_sdk=true.
> With the change you are proposing you won't be able to override environment
> setting with command line argument. So if you have
DART_MAKE_PLATFORM_SDK=true,
> there is no way for you to get dart_platform_sdk=false. I guess it's not that
> critical?

You can add a --no-platform-sdk flag. See e.g. --asan/--no-asan.

[aam, 2017-08-08 20:03:16]

On 2017/08/08 19:53:06, zra wrote:
> On 2017/08/08 19:01:47, aam wrote:
> > On 2017/08/08 17:02:39, zra wrote:
> > > On 2017/08/07 23:19:07, aam wrote:
> > > > https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn
> > > > File sdk/BUILD.gn (right):
> > > > 
> > > >
> https://codereview.chromium.org/2998503002/diff/20001/sdk/BUILD.gn#newcode21
> > > > sdk/BUILD.gn:21: dart_platform_sdk = false
> > > > On 2017/08/07 22:11:14, zra wrote:
> > > > > Sorry for not noticing this sooner: Since both Flutter and Fuchsia
want
> > the
> > > > > platform SDK, and only the standalone Dart SDK build wants the full
SDK,
> I
> > > > think
> > > > > it makes sense to switch the default sense of this flag to true. Then
we
> > can
> > > > > pass 'false' by default from our //tools/gn.py (except for the
> > architectures
> > > > > listed below). This way, we can avoid the need to add an is_flutter GN
> > > > argument.
> > > > 
> > > > tools/gn.py pulls the value for this flag from environment variable
> > > > DART_MAKE_PLATFORM_SDK, which is then being overriden with command line
> > > argument
> > > > --platform-sdk. Changing the default will require renaming those to
> > > > DART_MAKE_FULL_SDK and --full-sdk respectively and updating the places
> where
> > > > they are (do we know where are they used? I was not able to find ) Is it
> > worth
> > > > it?
> > > 
> > > You can leave DART_MAKE_PLATFORM_SDK as-is, and in gn.py in ToGnArgs, you
> can
> > > say something like:
> > > 
> > > if not args.platform_sdk and not gn_args['target_cpu'].startswith('arm'):
> > >   gn_args['dart_platform_sdk'] = false
> > > 
> > > Then, in sdk/BUILD.gn, you change the default value for dart_platform_sdk
> from
> > > false to true.
> > 
> > So in current set up it is possible to override environment setting
> > (DART_MAKE_PLATFORM_SDK=false) with command line argument(--platform-sdk),
so
> > you end up with dart_platform_sdk=true.
> > With the change you are proposing you won't be able to override environment
> > setting with command line argument. So if you have
> DART_MAKE_PLATFORM_SDK=true,
> > there is no way for you to get dart_platform_sdk=false. I guess it's not
that
> > critical?
> 
> You can add a --no-platform-sdk flag. See e.g. --asan/--no-asan.

I don't have strong opinion on this except it feels weird to have
--no-platform-sdk, when if you avoid negative it becomes  --full-sdk.
Otherwise, I updated the cl per your recommendations.

[zra, 2017-08-08 21:10:24]

lgtm w/ comment on GN arg

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn
File sdk/BUILD.gn (right):

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn#newcode21
sdk/BUILD.gn:21: dart_stripped_binary = "dart"
Please add a comment.

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn#newcode221
sdk/BUILD.gn:221: "$dart_out/dart.exe",
May need to check in with bkonyi to see if this is where this will go in the
engine build on Windows.

[aam, 2017-08-08 21:18:24]

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn
File sdk/BUILD.gn (right):

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn#newcode21
sdk/BUILD.gn:21: dart_stripped_binary = "dart"
On 2017/08/08 21:10:24, zra wrote:
> Please add a comment.

Done.

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn#newcode221
sdk/BUILD.gn:221: "$dart_out/dart.exe",
On 2017/08/08 21:10:24, zra wrote:
> May need to check in with bkonyi to see if this is where this will go in the
> engine build on Windows.

Do you mean in Flutter engine build on Windows once it works on that platform?

[aam, 2017-08-08 21:24:54]

Description was changed from

==========
Introduce is_flutter flag that is set for flutter build.

This is needed to produce platform dart_sdk built with Flutter.

BUG=
==========

to

==========
Flutter and Fuchsia build stripped down [platform] version of dart sdk, so make
it default(while standalone builds full dart sdk)

Both Flutter and Fuchsia expect stripped dart in standard out directory, so make
that default too(while standalone build puts stripped into exe.stripped/).

BUG=
==========

[aam, 2017-08-08 21:42:16]

Description was changed from

==========
Flutter and Fuchsia build stripped down [platform] version of dart sdk, so make
it default(while standalone builds full dart sdk)

Both Flutter and Fuchsia expect stripped dart in standard out directory, so make
that default too(while standalone build puts stripped into exe.stripped/).

BUG=
==========

to

==========
Flutter and Fuchsia build stripped down [platform] version of dart sdk, so make
it default(while standalone builds full dart sdk)

Both Flutter and Fuchsia expect stripped dart in standard out directory, so make
that default too(while standalone build puts stripped into exe.stripped/).

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1d50f112dcbf57aeb69739b855e5df93cfed168f
==========

[aam, 2017-08-08 21:42:18]

Committed patchset #4 (id:60001) manually as
1d50f112dcbf57aeb69739b855e5df93cfed168f (presubmit successful).

[zra, 2017-08-08 21:42:42]

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn
File sdk/BUILD.gn (right):

https://codereview.chromium.org/2998503002/diff/40001/sdk/BUILD.gn#newcode221
sdk/BUILD.gn:221: "$dart_out/dart.exe",
On 2017/08/08 21:18:24, aam wrote:
> On 2017/08/08 21:10:24, zra wrote:
> > May need to check in with bkonyi to see if this is where this will go in the
> > engine build on Windows.
> 
> Do you mean in Flutter engine build on Windows once it works on that platform?

Yes