Reaply "Use a real package-root for all of our samples (and tests inside a package) instead of buil…

Reaply "Use a real package-root for all of our samples (and tests inside a package) instead of buildDir/packages/"

R=whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31694

[Bill Hesse, 2014-01-10 13:56:21]

lgtm

https://codereview.chromium.org/133653004/diff/30001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/133653004/diff/30001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_options.dart:285: 'but use overrides for the
packages we\'ve available in the'
American English uses "we have" or "we've got" except for the past tense use of
"we've".  Maybe just say "packages available".

[kustermann, 2014-01-10 15:15:26]

https://codereview.chromium.org/133653004/diff/30001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/133653004/diff/30001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_options.dart:285: 'but use overrides for the
packages we\'ve available in the'
On 2014/01/10 13:56:22, Bill Hesse wrote:
> American English uses "we have" or "we've got" except for the past tense use
of
> "we've".  Maybe just say "packages available".

Done.

[kustermann, 2014-01-10 15:15:48]

Committed patchset #3 manually as r31694 (presubmit successful).

[Ivan Posva, 2014-01-10 16:52:52]

Martin,

Can you please explain what changed that the reapply landed without turning the
buildbot red? Where the previous attempt did not.

Thanks,
-Ivan

[kustermann, 2014-01-13 11:50:11]

On 2014/01/10 16:52:52, Ivan Posva wrote:
> Martin,
> 
> Can you please explain what changed that the reapply landed without turning
the
> buildbot red? Where the previous attempt did not.
> 
> Thanks,
> -Ivan

Sure. The first patchset was the original CL, patchset 2 makes it an opt-in
solution (opting in by passing --use-repository-packages or
--use-public-packages).

I don't want to explain the whole story, but I'll sketch it briefly:

We have packages and samples in the repository. Most of them have a pubspec.yaml
file specifying dependencies. With the current "out/ReleaseIA32/packages"-hack
(i.e. just making symlinks) we don't validate that we can actually get the
specified dependencies and whether the tests work with the dependencies.
Our users could download a sample and may not be able to fetch dependencies or
may not be able to build the sample app.

This CL makes sure that we can run sample/pkg tests with a *real* package root
by calling 'pub get' to fetch the dependencies. Once with packages from
pub.dartlang.org and once with the packages we have checked into the repository
(--use-public-packages or --use-repository-packages). Both of these ways involve
calling pub.

The first commit made --use-repository-packages the default. Which made
basically all builders use 'pub get' to build a valid package root for
pkg/samples (not for the normal tests, to be clear here!).

Now the way we invoke pub normally (from test.py) is
out/ReleaseIA32/dart-sdk/bin/pub (as we do with dart2js). If we don't pass the
'--use-sdk' flat to test.py it uses sdk/bin/pub (as we do with dart2js). The
only issue is that sdk/bin/dart2js works without having the SDK built, but
sdk/bin/pub requires the SDK to be built (even if I fix that, it can't determine
the version of the sdk/dart binary).

All of our VM builders build only the 'runtime' target and *not* the
'create_sdk' target, so we're currently not able to call pub there.

Now you might argue that we should not invoke pub on the vm builders anyway, but
some sample/pkg tests are running there right now and they use the broken
out/ReleaseIA32/packages package-root. We might want to move these tests to the
pub builder, but that's not clear yet (by doing so we loose VM coverage, since
running all pkg/sample tests in ia32,x64,debug,release,checked,unchecked
[+simarm,simmips,arm hw, mips hw] has revealed Crashes in the VM or dart:io bugs
in the past [because these tests cover much more than very short co19 test for
example])

This will also be enabled on all dart2js builders eventually.

We had lengthy discussions here, and we decided to wait for the summit before
taking action.

[It was "requested" that we implemented this ASAP.]