Add a snapshot for docgen and use it in the build

Add a snapshot for docgen and use it in the build

R=efortuna@google.com, ricow@google.com, rnystrom@google.com

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

[Emily Fortuna, 2014-01-07 23:10:25]

overall good.. (I'm leaving gyp to Peter)
Just a few questions.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/doc/README.txt
File pkg/docgen/doc/README.txt (right):

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/doc/README.txt#ne...
pkg/docgen/doc/README.txt:1: Note that there is a copy of this hard-coded into
the docgen.dart sources as
sorry... I couldn't parse this sentence. Could you please rephrase? :-)

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen#newcode2
sdk/bin/docgen:2: # Copyright (c) 2014, the Dart project authors.  Please see
the AUTHORS file
can/should this script be merged in some way with pkg/bin/dartdoc.py ?
"dartdoc.py -d" *should* give the same functionality I believe

[Bob Nystrom, 2014-01-08 00:16:57]

A couple of nits but LGTM, aside from the gyp stuff which is black magic to me.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart#n...
pkg/docgen/lib/docgen.dart:49: /**
Use a ///-style doc comment like the rest of this file.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart#n...
pkg/docgen/lib/docgen.dart:55: const String DEFAULT_SDK_INTRODUCTION = """
You don't need to type annotate constants.

https://codereview.chromium.org/116043013/diff/1/utils/compiler/create_snapsh...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/1/utils/compiler/create_snapsh...
utils/compiler/create_snapshot.dart:87: * --dart2js_main=val   The path to the
dart2js main script releative to root.
Update this?

[Alan Knight, 2014-01-08 01:22:51]

In addition to changes from review, noticed two problems.
  - We were always passing in the SDK package-root in the script
  - If we were given a relative path to the target and no package-root we would
infer a package root that was also a relative path and the mirror system doesn't
like relative paths.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/doc/README.txt
File pkg/docgen/doc/README.txt (right):

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/doc/README.txt#ne...
pkg/docgen/doc/README.txt:1: Note that there is a copy of this hard-coded into
the docgen.dart sources as
On 2014/01/07 23:10:25, Emily Fortuna wrote:
> sorry... I couldn't parse this sentence. Could you please rephrase? :-)

Done.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart#n...
pkg/docgen/lib/docgen.dart:49: /**
On 2014/01/08 00:16:57, Bob Nystrom wrote:
> Use a ///-style doc comment like the rest of this file.

Done.

https://codereview.chromium.org/116043013/diff/1/pkg/docgen/lib/docgen.dart#n...
pkg/docgen/lib/docgen.dart:55: const String DEFAULT_SDK_INTRODUCTION = """
On 2014/01/08 00:16:57, Bob Nystrom wrote:
> You don't need to type annotate constants.

Done.

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen#newcode2
sdk/bin/docgen:2: # Copyright (c) 2014, the Dart project authors.  Please see
the AUTHORS file
On 2014/01/07 23:10:25, Emily Fortuna wrote:
> can/should this script be merged in some way with pkg/bin/dartdoc.py ?
> "dartdoc.py -d" *should* give the same functionality I believe

The idea is to have the snapshot in the SDK so it can be used without requiring
a checkout of the repo. So we can't use dartdoc.py, unless we move that out of
the docgen package into the SDK. Also, I suspect it can't be python, as we
wouldn't want to require a python install.

It might be possible to have dartdoc.py switch to use this script to invoke
docgen. I started to do that, but thought that it might provide better coverage
to have dartdoc.py using the source and the build using the snapshot so that
both get exercised.

https://codereview.chromium.org/116043013/diff/1/utils/compiler/create_snapsh...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/1/utils/compiler/create_snapsh...
utils/compiler/create_snapshot.dart:87: * --dart2js_main=val   The path to the
dart2js main script releative to root.
On 2014/01/08 00:16:57, Bob Nystrom wrote:
> Update this?

Done.

[ricow1, 2014-01-08 06:39:49]

Is this ready for prime time, this is enabling it in the sdk

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:52: /// in docgen/doc/sdk-introduction.md
any reason to keep it in that file then? if we only have it here there is no
confusion and less code

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen#newcode28
sdk/bin/docgen:28: exec "$BIN_DIR"/dart --heap_growth_rate=32
--old_gen_heap_size=1024 \
is this increased old gen size needed? (it is not an issue if it is, but if not
just leave it out). Also, does specialcasing the growth rate have a performance
impact on dartdoc, if not don't do it

https://codereview.chromium.org/116043013/diff/90001/utils/apidoc/docgen.gyp
File utils/apidoc/docgen.gyp (right):

https://codereview.chromium.org/116043013/diff/90001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:60:
'<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)/dart-sdk/bin/docgen<(script_suffix)',
shouldn't we depend on ../../sdk/bin/docgen{'',.bat} instead here (put it below
with dart2js)
By doing this we are depending on it indirectly through the create_sdk script, I
think it is better to have a direct input since it makes it easier to read this

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/compiler.gyp
File utils/compiler/compiler.gyp (right):

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/compiler....
utils/compiler/compiler.gyp:23: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../sdk/lib/_internal/compiler", "../../runtime/lib",
"../../sdk/lib/_internal/dartdoc", "../../pkg/docgen"])',
we probably already have this indirectly through the packages.stamp input below

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
utils/compiler/create_snapshot.dart:90: * --package-root=val   The package-root
used to find packages for the snapshot.
thank you

[ricow1, 2014-01-08 06:40:52]

+sgjesse who have been trying to come up with a solution for removing our awful
shell scripts (just FYI)

[kustermann, 2014-01-08 08:39:17]

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:205: var dependentsJson =
Process.runSync('/Users/alanknight/dart-git/dart/xcodebuild/ReleaseIA32/dart-sdk/bin/pub',
['list-package-dirs'],
Why do you have a hardcoded path to pub here?

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:387: String introduction: ''}) {
Since you pass 'introduction' to 'readIntro(String introFileName)', you could
rename it to 'introductionFilename' (or something like this) so it states
clearly, it's not the introduction but a filename.

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:411: String readIntro(String introFileName) {
Rename to readIntroductionFile?

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen#newcode34
sdk/bin/docgen:34: fi
I really don't like the fact that we make more and more copies of these shell
scripts.

Why can't we at least share common routines in a sdk/bin/_common.sh file and
source that here? [or some other mechanism]

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
utils/compiler/create_snapshot.dart:36:
dart2jsMain.main(arguments.skip(1).toList());
As far as I remember we have a separate dart2js snapshot. With this we have
actually two versions of dart2js:

- snapshots/utils_wrapper.dart.snapshot
- snapshots/dart2js.dart.snapshot

This just blows up the sdk size!

[Alan Knight, 2014-01-08 22:19:02]

PTAL

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:52: /// in docgen/doc/sdk-introduction.md
On 2014/01/08 06:39:49, ricow1 wrote:
> any reason to keep it in that file then? if we only have it here there is no
> confusion and less code

Sounds reasonable. Done.

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:205: var dependentsJson =
Process.runSync('/Users/alanknight/dart-git/dart/xcodebuild/ReleaseIA32/dart-sdk/bin/pub',
['list-package-dirs'],
On 2014/01/08 08:39:18, kustermann wrote:
> Why do you have a hardcoded path to pub here?

Yikes. That was debugging code. Removed, thanks.

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:387: String introduction: ''}) {
On 2014/01/08 08:39:18, kustermann wrote:
> Since you pass 'introduction' to 'readIntro(String introFileName)', you could
> rename it to 'introductionFilename' (or something like this) so it states
> clearly, it's not the introduction but a filename.

Done. Also re-arranged a bit since we no longer read the default SDK intro from
a file.

https://codereview.chromium.org/116043013/diff/90001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:411: String readIntro(String introFileName) {
On 2014/01/08 08:39:18, kustermann wrote:
> Rename to readIntroductionFile?

Done.

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen#newcode28
sdk/bin/docgen:28: exec "$BIN_DIR"/dart --heap_growth_rate=32
--old_gen_heap_size=1024 \
On 2014/01/08 06:39:49, ricow1 wrote:
> is this increased old gen size needed? (it is not an issue if it is, but if
not
> just leave it out). Also, does specialcasing the growth rate have a
performance
> impact on dartdoc, if not don't do it

The heap size used to be needed or it wouldn't run at all. The growth rate was
in there from dartdoc and I just left it in. In practice neither appears to make
any difference to performance so I've removed them.

https://codereview.chromium.org/116043013/diff/90001/sdk/bin/docgen#newcode34
sdk/bin/docgen:34: fi
On 2014/01/08 08:39:18, kustermann wrote:
> I really don't like the fact that we make more and more copies of these shell
> scripts.
> 
> Why can't we at least share common routines in a sdk/bin/_common.sh file and
> source that here? [or some other mechanism]

This one should at least be script-neutral in that we would expect to delete the
dartdoc scripts once the new viewer is fully deployed. Created
https://code.google.com/p/dart/issues/detail?id=15973 for the general issue.

https://codereview.chromium.org/116043013/diff/90001/utils/apidoc/docgen.gyp
File utils/apidoc/docgen.gyp (right):

https://codereview.chromium.org/116043013/diff/90001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:60:
'<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)/dart-sdk/bin/docgen<(script_suffix)',
On 2014/01/08 06:39:49, ricow1 wrote:
> shouldn't we depend on ../../sdk/bin/docgen{'',.bat} instead here (put it
below
> with dart2js)
> By doing this we are depending on it indirectly through the create_sdk script,
I
> think it is better to have a direct input since it makes it easier to read
this

OK, but then shouldn't we also be running the one from that directory and not
from PRODUCT_DIR below? It seems like we could be in the position where the
PRODUCT_DIR one doesn't exist yet. But it doesn't work if I do that, so leaving
it (the one below) as is and changing this one.

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/compiler.gyp
File utils/compiler/compiler.gyp (right):

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/compiler....
utils/compiler/compiler.gyp:23: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../sdk/lib/_internal/compiler", "../../runtime/lib",
"../../sdk/lib/_internal/dartdoc", "../../pkg/docgen"])',
On 2014/01/08 06:39:49, ricow1 wrote:
> we probably already have this indirectly through the packages.stamp input
below

OK, removed. I had some problems with it not rebuilding the snapshot when I made
code changes, but wasn't sure exactly what was going on.

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/90001/utils/compiler/create_sn...
utils/compiler/create_snapshot.dart:36:
dart2jsMain.main(arguments.skip(1).toList());
On 2014/01/08 08:39:18, kustermann wrote:
> As far as I remember we have a separate dart2js snapshot. With this we have
> actually two versions of dart2js:
> 
> - snapshots/utils_wrapper.dart.snapshot
> - snapshots/dart2js.dart.snapshot
> 
> This just blows up the sdk size!
> 

Created https://code.google.com/p/dart/issues/detail?id=15967

[Alan Knight, 2014-01-08 22:26:40]

> Is this ready for prime time, this is enabling it in the sdk

It's already running in the SDK with each build. You can see the output at
https://dartdoc-next-dot-dartlang-api.appspot.com/apidocs/channels/be/#home 

There are still some issues that Emily's been working on with referencing
re-exported names and properly suppressing things that should be hidden (e.g.
private classes like Interceptor, mixin applications) but it already does a
better job on those than the published dartdoc.

[Emily Fortuna, 2014-01-08 23:17:17]

LGTM!

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen
File sdk/bin/docgen (right):

https://codereview.chromium.org/116043013/diff/1/sdk/bin/docgen#newcode2
sdk/bin/docgen:2: # Copyright (c) 2014, the Dart project authors.  Please see
the AUTHORS file
On 2014/01/08 01:22:52, Alan Knight wrote:
> On 2014/01/07 23:10:25, Emily Fortuna wrote:
> > can/should this script be merged in some way with pkg/bin/dartdoc.py ?
> > "dartdoc.py -d" *should* give the same functionality I believe
> 
> The idea is to have the snapshot in the SDK so it can be used without
requiring
> a checkout of the repo. So we can't use dartdoc.py, unless we move that out of
> the docgen package into the SDK. Also, I suspect it can't be python, as we
> wouldn't want to require a python install.
> 
> It might be possible to have dartdoc.py switch to use this script to invoke
> docgen. I started to do that, but thought that it might provide better
coverage
> to have dartdoc.py using the source and the build using the snapshot so that
> both get exercised.

Okay. Yeah. I'll work on making dartdoc.py call these scripts... after this
other big refactor.

[Alan Knight, 2014-01-10 15:32:59]

Ping

[ricow1, 2014-01-13 13:14:20]

Should we not remove the old document generator

https://codereview.chromium.org/116043013/diff/330001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/330001/pkg/docgen/lib/docgen.d...
pkg/docgen/lib/docgen.dart:198: introFileName: introFileName);
do we ever read this from a file now? if not just remove this argument here,
above and below

https://codereview.chromium.org/116043013/diff/330001/sdk/bin/docgen.bat
File sdk/bin/docgen.bat (right):

https://codereview.chromium.org/116043013/diff/330001/sdk/bin/docgen.bat#newc...
sdk/bin/docgen.bat:31: :: 9499.
I am not sure this is valid anymore

https://codereview.chromium.org/116043013/diff/330001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/116043013/diff/330001/tools/create_sdk.py#new...
tools/create_sdk.py:108: for executable in ['dart2js', 'dartanalyzer',
'dartdoc', 'docgen', 'pub']:
do we wan't to remove the old one? - Having two different documentation
generators seems a bit to much

[Alan Knight, 2014-01-13 17:36:04]

https://codereview.chromium.org/116043013/diff/330001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/116043013/diff/330001/pkg/docgen/lib/docgen.d...
pkg/docgen/lib/docgen.dart:198: introFileName: introFileName);
On 2014/01/13 13:14:20, ricow1 wrote:
> do we ever read this from a file now? if not just remove this argument here,
> above and below

We don't in the SDK, but if we're using this to generate docs for an individual
package we want them to be able to provide an introductory text.

https://codereview.chromium.org/116043013/diff/330001/sdk/bin/docgen.bat
File sdk/bin/docgen.bat (right):

https://codereview.chromium.org/116043013/diff/330001/sdk/bin/docgen.bat#newc...
sdk/bin/docgen.bat:31: :: 9499.
On 2014/01/13 13:14:20, ricow1 wrote:
> I am not sure this is valid anymore

It doesn't seem to be. Removed here and from the dartdoc.bat script.

https://codereview.chromium.org/116043013/diff/330001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/116043013/diff/330001/tools/create_sdk.py#new...
tools/create_sdk.py:108: for executable in ['dart2js', 'dartanalyzer',
'dartdoc', 'docgen', 'pub']:
On 2014/01/13 13:14:20, ricow1 wrote:
> do we wan't to remove the old one? - Having two different documentation
> generators seems a bit to much

We will, but only once the new one is fully operating as a replacement. Part of
that is having a snapshot so it can be run for external packages from just an
SDK download.

[Alan Knight, 2014-01-16 01:03:22]

Pulled out everything except the directly snapshot-connected code into a
separate CL. Working on another CL to delete apidoc.

[ricow1, 2014-01-21 00:52:15]

https://codereview.chromium.org/116043013/diff/490001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/116043013/diff/490001/tools/create_sdk.py#new...
tools/create_sdk.py:108: for executable in ['dart2js', 'dartanalyzer',
'dartdoc', 'docgen', 'pub']:
pull out dartdoc?

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/compiler...
File utils/compiler/compiler.gyp (right):

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/compiler...
utils/compiler/compiler.gyp:37:
'--dartdoc_main=sdk/lib/_internal/dartdoc/bin/dartdoc.dart',
remove dartdoc

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
File utils/compiler/create_snapshot.dart (right):

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
utils/compiler/create_snapshot.dart:27: import '${dartdoc.toFilePath(windows:
false)}' as dartdocMain;
remove dartdoc

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
utils/compiler/create_snapshot.dart:37: } else if (tool == "dartdoc") {
remove

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
utils/compiler/create_snapshot.dart:91: * --dartdoc_main=val   The path to the
dartdoc main script relative to root.
remove

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
utils/compiler/create_snapshot.dart:96: var validArguments = ["--output_dir",
"--dart2js_main", "--dartdoc_main",
remove dartdoc

https://codereview.chromium.org/116043013/diff/490001/utils/compiler/create_s...
utils/compiler/create_snapshot.dart:108: if (!args.containsKey("dartdoc_main"))
throw "Please specify dartdoc_main";
remove

[ricow1, 2014-01-21 00:53:45]

Sorry, did not see the comment about you wanting to remove the old one in a
separate cl - LGTM

[Alan Knight, 2014-01-27 18:41:10]

Committed patchset #6 manually as r32025 (presubmit successful).