Properly label vm_tests as tests

Properly label vm_tests as tests

This will allow build tools that are interested in target types to work
properly.

BUG=
R=zra@google.com

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

[jmatt, 2016-11-29 22:28:54]

Description was changed from

==========
Properly label vm_tests as tests

This will allow build tools that are interested in target types to work
properly.

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

to

==========
Properly label vm_tests as tests

This will allow build tools that are interested in target types to work
properly.

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

[jmatt, 2016-11-29 22:28:54]

jmatt@google.com changed reviewers:
+ zra@google.com

[zra, 2016-11-30 15:53:19]

This breaks the standalone GN build. See the targets in //dart/BUILD.gn that
depend on run_vm_tests, but which aren't marked testonly. Since I'm not really
sure what testonly is supposed to mean, I'm not sure about marking all of those
targets testonly. The targets that include run_vm_tests also include other deps
that are not tests.

[jmatt, 2016-11-30 17:41:37]

On 2016/11/30 15:53:19, zra wrote:
> This breaks the standalone GN build. See the targets in //dart/BUILD.gn that
> depend on run_vm_tests, but which aren't marked testonly. Since I'm not really
> sure what testonly is supposed to mean, I'm not sure about marking all of
those
> targets testonly. The targets that include run_vm_tests also include other
deps
> that are not tests.

Yikes! Let's not break that. Sorry, I should have looked further up the chain.

My understanding of the 'testonly' attribute is that it denotes things which are
only for testing purposes and not part of what you would deploy as shipping
code. If this understanding is correct, then the test targets should be marked
'testonly' and at the higher level, the test currently in groups mingled with
non-tests should be broken out. Since my familiarity is limited with Dart and
what automated systems might be expecting to hold as true, can you provide some
guidance on how to make these changes without breaking things?

[zra, 2016-11-30 17:53:36]

On 2016/11/30 17:41:37, jmatt wrote:
> On 2016/11/30 15:53:19, zra wrote:
> > This breaks the standalone GN build. See the targets in //dart/BUILD.gn that
> > depend on run_vm_tests, but which aren't marked testonly. Since I'm not
really
> > sure what testonly is supposed to mean, I'm not sure about marking all of
> those
> > targets testonly. The targets that include run_vm_tests also include other
> deps
> > that are not tests.
> 
> Yikes! Let's not break that. Sorry, I should have looked further up the chain.
> 
> My understanding of the 'testonly' attribute is that it denotes things which
are
> only for testing purposes and not part of what you would deploy as shipping
> code. If this understanding is correct, then the test targets should be marked
> 'testonly' and at the higher level, the test currently in groups mingled with
> non-tests should be broken out. Since my familiarity is limited with Dart and
> what automated systems might be expecting to hold as true, can you provide
some
> guidance on how to make these changes without breaking things?

Since we're still in the middle of migrating from gyp to GN, our standalone GN
build currently needs to mirror whatever the gyp build is doing. At the moment,
the 'runtime' target (for example) in the gyp build builds the standalone Dart
VM as well as run_vm_tests, so the GN build needs to do the same thing, and to
make a long story short, we can't split them apart.

If this is a high priority, and we must split these apart, we'll have to rope in
a couple more folks because we'll have to update the recipes on all of our
buildbots with whatever the new targets are going to be, and we'll need to have
a transition period during which both the old and new targets are available.

However, if this isn't blocking anyone, I would suggesting delaying until our
transition to GN is completed.

[jmatt, 2016-11-30 18:18:19]

On 2016/11/30 17:53:36, zra wrote:
> On 2016/11/30 17:41:37, jmatt wrote:
> > On 2016/11/30 15:53:19, zra wrote:
> > > This breaks the standalone GN build. See the targets in //dart/BUILD.gn
that
> > > depend on run_vm_tests, but which aren't marked testonly. Since I'm not
> really
> > > sure what testonly is supposed to mean, I'm not sure about marking all of
> > those
> > > targets testonly. The targets that include run_vm_tests also include other
> > deps
> > > that are not tests.
> > 
> > Yikes! Let's not break that. Sorry, I should have looked further up the
chain.
> > 
> > My understanding of the 'testonly' attribute is that it denotes things which
> are
> > only for testing purposes and not part of what you would deploy as shipping
> > code. If this understanding is correct, then the test targets should be
marked
> > 'testonly' and at the higher level, the test currently in groups mingled
with
> > non-tests should be broken out. Since my familiarity is limited with Dart
and
> > what automated systems might be expecting to hold as true, can you provide
> some
> > guidance on how to make these changes without breaking things?
> 
> Since we're still in the middle of migrating from gyp to GN, our standalone GN
> build currently needs to mirror whatever the gyp build is doing. At the
moment,
> the 'runtime' target (for example) in the gyp build builds the standalone Dart
> VM as well as run_vm_tests, so the GN build needs to do the same thing, and to
> make a long story short, we can't split them apart.
> 
> If this is a high priority, and we must split these apart, we'll have to rope
in
> a couple more folks because we'll have to update the recipes on all of our
> buildbots with whatever the new targets are going to be, and we'll need to
have
> a transition period during which both the old and new targets are available.
> 
> However, if this isn't blocking anyone, I would suggesting delaying until our
> transition to GN is completed.

This is causing significant pain for Fuchsia because its filesystem is currently
loaded into RAM. The Dart tests are ~45MB, which means in the common case we're
losing all this memory but I'm not sure we get anything in return. Any
suggestions for how we can label this large target as a test (which we can then
filter) and satisfy the other requirements?

[jmatt, 2016-11-30 18:23:53]

On 2016/11/30 18:18:19, jmatt wrote:
> On 2016/11/30 17:53:36, zra wrote:
> > On 2016/11/30 17:41:37, jmatt wrote:
> > > On 2016/11/30 15:53:19, zra wrote:
> > > > This breaks the standalone GN build. See the targets in //dart/BUILD.gn
> that
> > > > depend on run_vm_tests, but which aren't marked testonly. Since I'm not
> > really
> > > > sure what testonly is supposed to mean, I'm not sure about marking all
of
> > > those
> > > > targets testonly. The targets that include run_vm_tests also include
other
> > > deps
> > > > that are not tests.
> > > 
> > > Yikes! Let's not break that. Sorry, I should have looked further up the
> chain.
> > > 
> > > My understanding of the 'testonly' attribute is that it denotes things
which
> > are
> > > only for testing purposes and not part of what you would deploy as
shipping
> > > code. If this understanding is correct, then the test targets should be
> marked
> > > 'testonly' and at the higher level, the test currently in groups mingled
> with
> > > non-tests should be broken out. Since my familiarity is limited with Dart
> and
> > > what automated systems might be expecting to hold as true, can you provide
> > some
> > > guidance on how to make these changes without breaking things?
> > 
> > Since we're still in the middle of migrating from gyp to GN, our standalone
GN
> > build currently needs to mirror whatever the gyp build is doing. At the
> moment,
> > the 'runtime' target (for example) in the gyp build builds the standalone
Dart
> > VM as well as run_vm_tests, so the GN build needs to do the same thing, and
to
> > make a long story short, we can't split them apart.
> > 
> > If this is a high priority, and we must split these apart, we'll have to
rope
> in
> > a couple more folks because we'll have to update the recipes on all of our
> > buildbots with whatever the new targets are going to be, and we'll need to
> have
> > a transition period during which both the old and new targets are available.
> > 
> > However, if this isn't blocking anyone, I would suggesting delaying until
our
> > transition to GN is completed.
> 
> This is causing significant pain for Fuchsia because its filesystem is
currently
> loaded into RAM. The Dart tests are ~45MB, which means in the common case
we're
> losing all this memory but I'm not sure we get anything in return. Any
> suggestions for how we can label this large target as a test (which we can
then
> filter) and satisfy the other requirements?

It seems like if I add another executable target labeled "testonly" which
depends on "run_vm_tests" then I get the desired result. What do you think if I
create the "run_vm_tests_alias" with some comments in the file about why this
odd arrangement exists?

[zra, 2016-11-30 18:30:49]

https://codereview.chromium.org/2540853002/diff/1/runtime/bin/BUILD.gn
File runtime/bin/BUILD.gn (right):

https://codereview.chromium.org/2540853002/diff/1/runtime/bin/BUILD.gn#newcod...
runtime/bin/BUILD.gn:767: testonly = true
It looks like this can be set conditionally, e.g:

if (defined(is_fuchsia) && is_fuchsia) {
  testonly = true
}

If that works for you, I would prefer that over adding an alias for the target.

[jmatt, 2016-11-30 19:00:09]

On 2016/11/30 18:30:49, zra wrote:
> https://codereview.chromium.org/2540853002/diff/1/runtime/bin/BUILD.gn
> File runtime/bin/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2540853002/diff/1/runtime/bin/BUILD.gn#newcod...
> runtime/bin/BUILD.gn:767: testonly = true
> It looks like this can be set conditionally, e.g:
> 
> if (defined(is_fuchsia) && is_fuchsia) {
>   testonly = true
> }
> 
> If that works for you, I would prefer that over adding an alias for the
target.

Yup, that works for me. Which is great, because it turns out I can't make the
alias thing work anyway. I'd fooled myself into believing it worked. I'll revise
the change.

[zra, 2016-11-30 20:26:44]

lgtm

[jmatt, 2016-11-30 22:15:57]

On 2016/11/30 20:26:44, zra wrote:
> lgtm

I *think* I don't have commit access, can someone merge this for me?

[zra, 2016-11-30 22:34:40]

Description was changed from

==========
Properly label vm_tests as tests

This will allow build tools that are interested in target types to work
properly.

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

to

==========
Properly label vm_tests as tests

This will allow build tools that are interested in target types to work
properly.

BUG=
R=zra@google.com

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

[zra, 2016-11-30 22:34:41]

Committed patchset #3 (id:40001) manually as
c8ecd31aa529376f1d7160938cef25730cc519d7 (presubmit successful).