Dynamically load packages for dartdevc tests in test.dart.

Dynamically load packages for dartdevc tests in test.dart.

This involves a few pieces:

- Add support to DDC for specifying the module name associated with a
  given summary. This lets test.dart invoke DDC using summaries in the
  build directory outside of the directory containing the test itself.

- Add support to the build scripts for building the packages. This adds
  a new GN target that builds everything needed to run test.dart with
  dartdevc. In particular, it invokes build_pkgs.dart to compile the
  relevant packages to JS+summary so that the tests can use them.

  This requires some changes to build_pkgs.dart so it can output to a
  given directory.

- In test.dart, when tests are compiled with dartdevc, pass in the
  summaries for the packages so they don't get compiled in. Then, when
  the test is run, configure require.js with the right paths to their
  JS files so they can be loaded.

  I also removed a bunch of unneeded buildDir parameters being passed
  around the various CompilerConfiguration class methods now that they
  have direct access to the configuration.

Fix #29923.

R=vsm@google.com, whesse@google.com, zra@google.com

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

[Bob Nystrom, 2017-06-22 23:43:44]

rnystrom@google.com changed reviewers:
+ vsm@google.com, whesse@google.com, zra@google.com

[Bob Nystrom, 2017-06-22 23:43:45]

vsm for dev_compiler stuff, zra for GN changes, and whesse for the rest.

Thank you!

[zra, 2017-06-23 05:00:39]

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn
File utils/dartdevc/BUILD.gn (right):

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:87: ":dartdevc",
deps from the current file should come first.

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:113: "$root_gen_dir/dartdevc/pkg/async_helper.js",
$root_gen_dir and $root_out_dir are somewhat valuable real estate, and manually
creating subdirectories of $root_gen_dir is potentially unsafe because they
could conflict with the $target_gen_dir for a BUILD.gn file that someone might
create at //dartdevc, for example.

If at all possible, please use $target_gen_dir here instead. This would make the
output go in out/ReleaseX64/gen/utils/dartdevc/pkg instead of
out/ReleaseX64/gen/dartdevc/pkg.

[vsm, 2017-06-23 14:29:12]

lgtm on ddc part

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/lib/src/an...
File pkg/dev_compiler/lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/lib/src/an...
pkg/dev_compiler/lib/src/analyzer/context.dart:35: final Map<String, String>
customSummaryModules = {};
Perhaps combine these last two data structs into a single Map and compute it at
startup?

final Map<String, String> summaryPathMap;

After that is built, we don't really care whether it was custom or not (I hope).

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/tool/build...
File pkg/dev_compiler/tool/build_test_pkgs.sh (right):

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/tool/build...
pkg/dev_compiler/tool/build_test_pkgs.sh:5: ./tool/build_pkgs.dart
gen/codegen_output/pkg
Maybe get rid of this file?  I don't ever call it directly - not sure anyone
does.

[Bob Nystrom, 2017-06-24 00:29:37]

Thanks!

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/lib/src/an...
File pkg/dev_compiler/lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/lib/src/an...
pkg/dev_compiler/lib/src/analyzer/context.dart:35: final Map<String, String>
customSummaryModules = {};
On 2017/06/23 14:29:08, vsm wrote:
> Perhaps combine these last two data structs into a single Map and compute it
at
> startup?
> 
> final Map<String, String> summaryPathMap;
> 
> After that is built, we don't really care whether it was custom or not (I
hope).

I like that idea, but I think it might require moving more code around than is
worth. We need the module root to calculate all of the non-customized module
names, and we don't know that yet at the point that the AnalyzerOptions are
created.

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/tool/build...
File pkg/dev_compiler/tool/build_test_pkgs.sh (right):

https://codereview.chromium.org/2955513002/diff/1/pkg/dev_compiler/tool/build...
pkg/dev_compiler/tool/build_test_pkgs.sh:5: ./tool/build_pkgs.dart
gen/codegen_output/pkg
On 2017/06/23 14:29:08, vsm wrote:
> Maybe get rid of this file?  I don't ever call it directly - not sure anyone
> does.

Done.

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn
File utils/dartdevc/BUILD.gn (right):

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:87: ":dartdevc",
On 2017/06/23 05:00:38, zra wrote:
> deps from the current file should come first.

Done.

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:113: "$root_gen_dir/dartdevc/pkg/async_helper.js",
On 2017/06/23 05:00:38, zra wrote:
> $root_gen_dir and $root_out_dir are somewhat valuable real estate, and
manually
> creating subdirectories of $root_gen_dir is potentially unsafe because they
> could conflict with the $target_gen_dir for a BUILD.gn file that someone might
> create at //dartdevc, for example.
> 
> If at all possible, please use $target_gen_dir here instead. This would make
the
> output go in out/ReleaseX64/gen/utils/dartdevc/pkg instead of
> out/ReleaseX64/gen/dartdevc/pkg.

I considered that, but this path appears in test.dart since it serves the
resulting files over HTTP for the browser tests. It felt weird to me to have
"utils/dartdevc" in that path since that's an implementation detail of the build
file layout and not relevant to the tests or test.dart.

If you want, I will, but I prefer the shorter path if that's OK.

[zra, 2017-06-26 05:47:33]

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn
File utils/dartdevc/BUILD.gn (right):

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:113: "$root_gen_dir/dartdevc/pkg/async_helper.js",
On 2017/06/24 00:29:36, Bob Nystrom wrote:
> On 2017/06/23 05:00:38, zra wrote:
> > $root_gen_dir and $root_out_dir are somewhat valuable real estate, and
> manually
> > creating subdirectories of $root_gen_dir is potentially unsafe because they
> > could conflict with the $target_gen_dir for a BUILD.gn file that someone
might
> > create at //dartdevc, for example.
> > 
> > If at all possible, please use $target_gen_dir here instead. This would make
> the
> > output go in out/ReleaseX64/gen/utils/dartdevc/pkg instead of
> > out/ReleaseX64/gen/dartdevc/pkg.
> 
> I considered that, but this path appears in test.dart since it serves the
> resulting files over HTTP for the browser tests. It felt weird to me to have
> "utils/dartdevc" in that path since that's an implementation detail of the
build
> file layout and not relevant to the tests or test.dart.
> 
> If you want, I will, but I prefer the shorter path if that's OK.

Yeah, sorry for the inconvenience, but as far as I'm concerned a shorter path
isn't worth the potential name/path conflicts that this approach can lead to.

[Bob Nystrom, 2017-06-28 23:04:03]

Friendly ping for Bill to look at the test.dart stuff.

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn
File utils/dartdevc/BUILD.gn (right):

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:113: "$root_gen_dir/dartdevc/pkg/async_helper.js",
On 2017/06/26 05:47:33, zra wrote:
> On 2017/06/24 00:29:36, Bob Nystrom wrote:
> > On 2017/06/23 05:00:38, zra wrote:
> > > $root_gen_dir and $root_out_dir are somewhat valuable real estate, and
> > manually
> > > creating subdirectories of $root_gen_dir is potentially unsafe because
they
> > > could conflict with the $target_gen_dir for a BUILD.gn file that someone
> might
> > > create at //dartdevc, for example.
> > > 
> > > If at all possible, please use $target_gen_dir here instead. This would
make
> > the
> > > output go in out/ReleaseX64/gen/utils/dartdevc/pkg instead of
> > > out/ReleaseX64/gen/dartdevc/pkg.
> > 
> > I considered that, but this path appears in test.dart since it serves the
> > resulting files over HTTP for the browser tests. It felt weird to me to have
> > "utils/dartdevc" in that path since that's an implementation detail of the
> build
> > file layout and not relevant to the tests or test.dart.
> > 
> > If you want, I will, but I prefer the shorter path if that's OK.
> 
> Yeah, sorry for the inconvenience, but as far as I'm concerned a shorter path
> isn't worth the potential name/path conflicts that this approach can lead to.

Done.

[Bill Hesse, 2017-06-29 14:18:46]

test files LGTM

https://codereview.chromium.org/2955513002/diff/40001/tools/testing/dart/brow...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/2955513002/diff/40001/tools/testing/dart/brow...
tools/testing/dart/browser_test.dart:71: // TODO(29923): Add paths to the
packages that are used in tests once they
Remove this TODO?

[zra, 2017-06-29 15:06:01]

GN lgtm

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn
File utils/dartdevc/BUILD.gn (right):

https://codereview.chromium.org/2955513002/diff/1/utils/dartdevc/BUILD.gn#new...
utils/dartdevc/BUILD.gn:113: "$root_gen_dir/dartdevc/pkg/async_helper.js",
On 2017/06/28 23:04:03, Bob Nystrom wrote:
> On 2017/06/26 05:47:33, zra wrote:
> > On 2017/06/24 00:29:36, Bob Nystrom wrote:
> > > On 2017/06/23 05:00:38, zra wrote:
> > > > $root_gen_dir and $root_out_dir are somewhat valuable real estate, and
> > > manually
> > > > creating subdirectories of $root_gen_dir is potentially unsafe because
> they
> > > > could conflict with the $target_gen_dir for a BUILD.gn file that someone
> > might
> > > > create at //dartdevc, for example.
> > > > 
> > > > If at all possible, please use $target_gen_dir here instead. This would
> make
> > > the
> > > > output go in out/ReleaseX64/gen/utils/dartdevc/pkg instead of
> > > > out/ReleaseX64/gen/dartdevc/pkg.
> > > 
> > > I considered that, but this path appears in test.dart since it serves the
> > > resulting files over HTTP for the browser tests. It felt weird to me to
have
> > > "utils/dartdevc" in that path since that's an implementation detail of the
> > build
> > > file layout and not relevant to the tests or test.dart.
> > > 
> > > If you want, I will, but I prefer the shorter path if that's OK.
> > 
> > Yeah, sorry for the inconvenience, but as far as I'm concerned a shorter
path
> > isn't worth the potential name/path conflicts that this approach can lead
to.
> 
> Done.

Thanks!

[Bob Nystrom, 2017-06-29 20:45:59]

Description was changed from

==========
Dynamically load packages for dartdevc tests in test.dart.

This involves a few pieces:

- Add support to DDC for specifying the module name associated with a
  given summary. This lets test.dart invoke DDC using summaries in the
  build directory outside of the directory containing the test itself.

- Add support to the build scripts for building the packages. This adds
  a new GN target that builds everything needed to run test.dart with
  dartdevc. In particular, it invokes build_pkgs.dart to compile the
  relevant packages to JS+summary so that the tests can use them.

  This requires some changes to build_pkgs.dart so it can output to a
  given directory.

- In test.dart, when tests are compiled with dartdevc, pass in the
  summaries for the packages so they don't get compiled in. Then, when
  the test is run, configure require.js with the right paths to their
  JS files so they can be loaded.

  I also removed a bunch of unneeded buildDir parameters being passed
  around the various CompilerConfiguration class methods now that they
  have direct access to the configuration.

Fix #29923.
==========

to

==========
Dynamically load packages for dartdevc tests in test.dart.

This involves a few pieces:

- Add support to DDC for specifying the module name associated with a
  given summary. This lets test.dart invoke DDC using summaries in the
  build directory outside of the directory containing the test itself.

- Add support to the build scripts for building the packages. This adds
  a new GN target that builds everything needed to run test.dart with
  dartdevc. In particular, it invokes build_pkgs.dart to compile the
  relevant packages to JS+summary so that the tests can use them.

  This requires some changes to build_pkgs.dart so it can output to a
  given directory.

- In test.dart, when tests are compiled with dartdevc, pass in the
  summaries for the packages so they don't get compiled in. Then, when
  the test is run, configure require.js with the right paths to their
  JS files so they can be loaded.

  I also removed a bunch of unneeded buildDir parameters being passed
  around the various CompilerConfiguration class methods now that they
  have direct access to the configuration.

Fix #29923.

R=vsm@google.com, whesse@google.com, zra@google.com

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

[Bob Nystrom, 2017-06-29 20:46:01]

Committed patchset #4 (id:60001) manually as
0b2c9b048d613bd81caf07acd285f61cd01b79dd (presubmit successful).

[Bob Nystrom, 2017-06-29 20:46:37]

https://codereview.chromium.org/2955513002/diff/40001/tools/testing/dart/brow...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/2955513002/diff/40001/tools/testing/dart/brow...
tools/testing/dart/browser_test.dart:71: // TODO(29923): Add paths to the
packages that are used in tests once they
On 2017/06/29 14:18:46, Bill Hesse wrote:
> Remove this TODO?

Done.