Generate docgen output along with api_docs as part of the build

Generate docgen output along with api_docs as part of the build

R=danrubel@google.com, efortuna@google.com, kustermann@google.com, rnystrom@google.com

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

[Alan Knight, 2013-11-14 20:54:39]

Generates docgen output into the build along with the existing api_docs output.
Some of this is adding docgen options and generalizing it to not assume where
its output directory is. The other part is cloning the apidoc gyp file and
modifying it for docgen.

[Bob Nystrom, 2013-11-14 22:43:03]

Some suggestions then LGTM. But I'm a gyp noob, so get the LGTM from Peter too
for that.

https://codereview.chromium.org/73113002/diff/1/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/1/dart.gyp#newcode158
dart.gyp:158: 'utils/apidoc/apidoc.gyp:api_docs',
Do you have a plan for when you'll remove this?

Maybe add a TODO?

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

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/bin/docgen.dart#ne...
pkg/docgen/bin/docgen.dart:33: intro : results['introduction'],
Multi-line ternary operator in the middle of an expression? How about a local
variable? :)

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

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:80: /// --exclude-lib
"."

Also, current doc comment convention is that the first sentence should be on its
own:

/// Library names to explicitly exclude.
///
/// Set from the command line option --exclude-lib

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:122: var librariesWeAskedFor = !parseSdk ?
_listLibraries(files) : _listSdk();
"We" seems funny to me. How about "requestedLibraries"?

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:231: return libraries.map((each) =>
path.normalize(path.absolute(each))).toList();
Could also do:

libraries.map(path.absolute).map(path.normalize).toList()

I'm not sure which I prefer. <shrug>

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:798: File file = new
File('$outputDirectory/$filename');
Since you're using path.join() a few lines up, let's use it here too.

[ahe, 2013-11-15 08:34:24]

I'll let Martin review this instead of me.

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

https://codereview.chromium.org/73113002/diff/1/utils/apidoc/docgen.gyp#newco...
utils/apidoc/docgen.gyp:68: '<(PRODUCT_DIR)/api_docs/docgen/library_list.json',
When is this file produced?
Are there other files written out after this file?
Can an error occur while this file is being written?

[Alan Knight, 2013-11-15 18:49:56]

https://codereview.chromium.org/73113002/diff/1/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/1/dart.gyp#newcode158
dart.gyp:158: 'utils/apidoc/apidoc.gyp:api_docs',
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> Do you have a plan for when you'll remove this?
> 
> Maybe add a TODO?

Done.

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

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/bin/docgen.dart#ne...
pkg/docgen/bin/docgen.dart:33: intro : results['introduction'],
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> Multi-line ternary operator in the middle of an expression? How about a local
> variable? :)

Done.

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

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:80: /// --exclude-lib
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> "."
> 
> Also, current doc comment convention is that the first sentence should be on
its
> own:
> 
> /// Library names to explicitly exclude.
> ///
> /// Set from the command line option --exclude-lib

Done.

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:122: var librariesWeAskedFor = !parseSdk ?
_listLibraries(files) : _listSdk();
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> "We" seems funny to me. How about "requestedLibraries"?

Done.

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:231: return libraries.map((each) =>
path.normalize(path.absolute(each))).toList();
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> Could also do:
> 
> libraries.map(path.absolute).map(path.normalize).toList()
> 
> I'm not sure which I prefer. <shrug>

I like yours better. Done.

https://codereview.chromium.org/73113002/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:798: File file = new
File('$outputDirectory/$filename');
On 2013/11/14 22:43:04, Bob Nystrom wrote:
> Since you're using path.join() a few lines up, let's use it here too.

Done.

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

https://codereview.chromium.org/73113002/diff/1/utils/apidoc/docgen.gyp#newco...
utils/apidoc/docgen.gyp:68: '<(PRODUCT_DIR)/api_docs/docgen/library_list.json',
On 2013/11/15 08:34:25, ahe wrote:
> When is this file produced?
> Are there other files written out after this file?
> Can an error occur while this file is being written?

Good point. Changed it to use index.json, which is the absolute last thing we
write. And threw in a null check for text, which is the only thing I can see
possibly going wrong beyond i/o errors during the writeAsStringSync.

[ahe, 2013-11-17 11:27:46]

But please wait for Martin to take a look :-)

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

https://codereview.chromium.org/73113002/diff/1/utils/apidoc/docgen.gyp#newco...
utils/apidoc/docgen.gyp:68: '<(PRODUCT_DIR)/api_docs/docgen/library_list.json',
On 2013/11/15 18:49:56, Alan Knight wrote:
> On 2013/11/15 08:34:25, ahe wrote:
> > When is this file produced?
> > Are there other files written out after this file?
> > Can an error occur while this file is being written?
> 
> Good point. Changed it to use index.json, which is the absolute last thing we
> write. And threw in a null check for text, which is the only thing I can see
> possibly going wrong beyond i/o errors during the writeAsStringSync.

SGTM :-)

[ahe, 2013-11-17 11:28:37]

But please wait for Martin to take a look :-)

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

https://codereview.chromium.org/73113002/diff/1/utils/apidoc/docgen.gyp#newco...
utils/apidoc/docgen.gyp:68: '<(PRODUCT_DIR)/api_docs/docgen/library_list.json',
On 2013/11/15 18:49:56, Alan Knight wrote:
> On 2013/11/15 08:34:25, ahe wrote:
> > When is this file produced?
> > Are there other files written out after this file?
> > Can an error occur while this file is being written?
> 
> Good point. Changed it to use index.json, which is the absolute last thing we
> write. And threw in a null check for text, which is the only thing I can see
> possibly going wrong beyond i/o errors during the writeAsStringSync.

SGTM :-)

[kustermann, 2013-11-18 09:57:07]

https://codereview.chromium.org/73113002/diff/100001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/100001/dart.gyp#newcode161
dart.gyp:161: 'utils/apidoc/docgen.gyp:docgen',
I think this could cause issues. These two dependencies are executed in a random
order (if I'm not missing something).

apidocs is called with:
'--out=<(PRODUCT_DIR)/api_docs',

docgen is called with:
 '--out=<(PRODUCT_DIR)/api_docs/docgen'

Both of these tools will, as a first step, clean the output directory.

Here's what can happen:
 1. docgen is executed
    - will clean "<(PRODUCT_DIR)/api_docs/docgen"
    - will generate documentation into "<(PRODUCT_DIR)/api_docs/docgen"
 2. apidocs is executed
    - will clean "<(PRODUCT_DIR)/api_docs"
    - will generate documentation into "<(PRODUCT_DIR)/api_docs"

In this situation the new documentation was deleted in the 2nd step.

If reasoning is correct, you have the following options
 - have only one gyp target which create the api documentation
    => You could for example write a python script which executes first the old
apidocs tool and then the new docgen tool

 - you could make the new docgen action depend on the output of the old apidocs
(i.e. the input files of the 

 - you could output the new documentation into a different directory than the
old one.

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:25: 'sdk-introduction.md');
Why is this markup file in the docgen/bin directory?

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

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:39: var outputDirectory = 'docs';
Why are some other global variables a few lines down marked with an "_" (i.e.
library private) and this one is not?

Will these variables be accessed from somewher else?

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:103: out: 'docs', List<String> excludeLibraries}) {
It's slightly strange that we have the default value 'docs' here *and* at line
39 (i.e. for the global outputDirectory variable).

[Alan Knight, 2013-11-18 20:34:50]

https://codereview.chromium.org/73113002/diff/100001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/100001/dart.gyp#newcode161
dart.gyp:161: 'utils/apidoc/docgen.gyp:docgen',
On 2013/11/18 09:57:08, kustermann wrote:
> I think this could cause issues. These two dependencies are executed in a
random
> order (if I'm not missing something).
> 
> apidocs is called with:
> '--out=<(PRODUCT_DIR)/api_docs',
> 
> docgen is called with:
>  '--out=<(PRODUCT_DIR)/api_docs/docgen'
> 
> Both of these tools will, as a first step, clean the output directory.
> 
> Here's what can happen:
>  1. docgen is executed
>     - will clean "<(PRODUCT_DIR)/api_docs/docgen"
>     - will generate documentation into "<(PRODUCT_DIR)/api_docs/docgen"
>  2. apidocs is executed
>     - will clean "<(PRODUCT_DIR)/api_docs"
>     - will generate documentation into "<(PRODUCT_DIR)/api_docs"
> 
> In this situation the new documentation was deleted in the 2nd step.
> 
> If reasoning is correct, you have the following options
>  - have only one gyp target which create the api documentation
>     => You could for example write a python script which executes first the
old
> apidocs tool and then the new docgen tool
> 
>  - you could make the new docgen action depend on the output of the old
apidocs
> (i.e. the input files of the 
> 
>  - you could output the new documentation into a different directory than the
> old one.

Made docgen depend on the apidocs target. It seems like that should be
sufficient, but my understanding of gyp is limited. Using the same directory
seemed good because it avoids needing to modify the uploading code, and this is
semantically the same stuff, with the idea that at some point we stop generating
apidocs output.

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:25: 'sdk-introduction.md');
On 2013/11/18 09:57:08, kustermann wrote:
> Why is this markup file in the docgen/bin directory?

I suspect so it was easy to find for bin/docgen.dart. But it doesn't make much
sense. Created a "doc" directory and moved it there.

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

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:39: var outputDirectory = 'docs';
On 2013/11/18 09:57:08, kustermann wrote:
> Why are some other global variables a few lines down marked with an "_" (i.e.
> library private) and this one is not?
> 
> Will these variables be accessed from somewher else?

Made it private.

https://codereview.chromium.org/73113002/diff/100001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:103: out: 'docs', List<String> excludeLibraries}) {
On 2013/11/18 09:57:08, kustermann wrote:
> It's slightly strange that we have the default value 'docs' here *and* at line
> 39 (i.e. for the global outputDirectory variable).

Yes. I can't default the parameter to a variable value, but made a constant and
defaulted it to that. Removed the default value for the variable, as it is
always and only ever initialized in this call.

[ahe, 2013-11-18 20:56:44]

https://codereview.chromium.org/73113002/diff/100001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/100001/dart.gyp#newcode161
dart.gyp:161: 'utils/apidoc/docgen.gyp:docgen',
On 2013/11/18 20:34:50, Alan Knight wrote:
> Made docgen depend on the apidocs target. It seems like that should be
> sufficient, but my understanding of gyp is limited.

Unfortunately, this is one of the biggest gotchas with GYP: depending on a
target has no impact on actions.

For actions to be executed in order, one action must declare an output file, and
the other must list that output file as an input. So you need to change the
docgen action so it lists an output of apidocs as input.

[Emily Fortuna, 2013-11-18 21:23:16]

discussed with Alan. I'm not certain we need this to actually be part of the
default build process. This could be a specific target that gets built if you
ask for it, but not generated by default.

[ahe, 2013-11-18 21:34:37]

On 2013/11/18 21:23:16, Emily Fortuna wrote:
> discussed with Alan. I'm not certain we need this to actually be part of the
> default build process. This could be a specific target that gets built if you
> ask for it, but not generated by default.

If you don't add a target to dart.gyp which builds docgen, then you can't build
it with ./tools/build.py.

If you add a target to dart.gyp, then it will be built when you run
./tools/build.py without specifying a target.

But you can make it not be part of create_sdk.

[Alan Knight, 2013-11-19 00:36:19]

OK, I've made it depend on a file, and added it as a dependency to upload_sdk,
which seems like the right thing.

[kustermann, 2013-11-19 16:40:36]

gyp files lgtm with comments

https://codereview.chromium.org/73113002/diff/380001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/380001/dart.gyp#newcode84
dart.gyp:84: 'target_name': 'upload_sdk',
I'm not familiar with this target. AFAIK this is not needed by our editor bots.
It seems to be archiving to 'gs://dart-dump-render-tree'.

Who uses this target?
Can we remove it [together with the tools/upload_sdk.py script]?

https://codereview.chromium.org/73113002/diff/380001/dart.gyp#newcode98
dart.gyp:98: '<(PRODUCT_DIR)/api_docs/docgen/index.json',
That seems to be unnecessary since the upload_sdk.py script doesn't actually use
'<PRODUCT_DIR)/api_docs', right?

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

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:23: '../../pkg/pkg.gyp:pkg_packages',
I'd still depend on the old apidocs target here.

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:66: '<(PRODUCT_DIR)/api_docs/index.html',
Add a short comment, that this is used for ensuring that "docgen" is executed
after "apidocs".

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:86: '--exclude-lib=docgen',
I'm wondering why you removed so many '--exclude-lib' flags here.

[Alan Knight, 2013-11-19 19:14:46]

https://codereview.chromium.org/73113002/diff/380001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/73113002/diff/380001/dart.gyp#newcode84
dart.gyp:84: 'target_name': 'upload_sdk',
On 2013/11/19 16:40:37, kustermann wrote:
> I'm not familiar with this target. AFAIK this is not needed by our editor
bots.
> It seems to be archiving to 'gs://dart-dump-render-tree'.
> 
> Who uses this target?
> Can we remove it [together with the tools/upload_sdk.py script]?

Ah. I was naively assuming that a target by that name was doing the uploads that
I wanted, which appears not to be the case. I don't know if we can remove it or
not, or even who to ask.

It seems like the thing I actually want is in the editor's build.py script,
which explicitly calls api_docs.

I also came across an explicit build of api_docs in the pub bots, which Bob says
can be removed, so I did that.

https://codereview.chromium.org/73113002/diff/380001/dart.gyp#newcode98
dart.gyp:98: '<(PRODUCT_DIR)/api_docs/docgen/index.json',
On 2013/11/19 16:40:37, kustermann wrote:
> That seems to be unnecessary since the upload_sdk.py script doesn't actually
use
> '<PRODUCT_DIR)/api_docs', right?

Yes. Reverted.

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

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:23: '../../pkg/pkg.gyp:pkg_packages',
On 2013/11/19 16:40:37, kustermann wrote:
> I'd still depend on the old apidocs target here.

Done.

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:66: '<(PRODUCT_DIR)/api_docs/index.html',
On 2013/11/19 16:40:37, kustermann wrote:
> Add a short comment, that this is used for ensuring that "docgen" is executed
> after "apidocs".

Done.

https://codereview.chromium.org/73113002/diff/380001/utils/apidoc/docgen.gyp#...
utils/apidoc/docgen.gyp:86: '--exclude-lib=docgen',
On 2013/11/19 16:40:37, kustermann wrote:
> I'm wondering why you removed so many '--exclude-lib' flags here.

From what I could tell there were two rationales for excluding things. One is
things that, while they were in the pkg directory, weren't being published as
standalone packages. This is a short blacklist found in
tools/publish_all_pkgs.py. The other were things that people at some point
didn't want displayed by default in the sdk docs, but this seems to have been
applied non-systematically. 

The new viewer's default view does not show anything in pkg and allows showing
the documentation for just a single package, so it seems reasonable to generate
all the docs and be able to provide documentation for individual packages that
are published on pub (i.e. everything in pkg not in . Ideally, we'd move to
generating fully individual documentation sets for each package and hosting that
differently, but this is a useful stopgap.

I did add docgen to the list. It should arguably be excluded, since it doesn't
work outside of the SDK (imports dartj2s mirrors via ../../.. paths). But it's
been published already, so if we stop we should probably also remove it and that
seems like a separate CL.

[Emily Fortuna, 2013-11-20 18:25:19]

lgtm on my end

[danrubel, 2013-11-20 21:19:58]

editor build change LGTM... I'll leave the rest to those more knowledgeable
about build.

[Alan Knight, 2013-11-20 21:52:05]

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