Run dartdoc for the given task. #322

isoos · 2017-08-29T17:02:59Z

No description provided.

kevmoo · 2017-08-29T19:13:16Z

app/lib/dartdoc/dartdoc_runner.dart

+    } finally {
+      await tempDir.delete(recursive: true);
+    }
+    return false; // no race detection


explain the comment?

The method should return true if there were a race of processing detected (e.g. in analyzer), false otherwise. Not sure how we want to comment on it in such case.

mkustermann

looks fine

mkustermann · 2017-08-30T13:24:56Z

app/lib/dartdoc/dartdoc_runner.dart

+
+    try {
+      final pkgLocation =
+          await pubEnv.getLocation(task.package, version: task.version);


Does this end up downloading from the proper https://pub.dartlang.org/packages/<name>/versions/<version>.tar.gz url?

(so we don't have the memcache issue)

Unfortunately pub does a sanity check before doing that:
https://github.com/dart-lang/pub/blob/master/lib/src/command/cache_add.dart#L57
https://github.com/dart-lang/pub/blob/master/lib/src/source/hosted.dart#L130

Btw. this affects pana the very same way. /cc @kevmoo

We don't have to use pub to download the package! We only need pub to download a package's dependencies via pub get.

mkustermann · 2017-08-30T13:33:08Z

app/lib/dartdoc/dartdoc_runner.dart

+      final pkgPath = pkgLocation.location;
+      await _runDartdoc(task, pkgPath);
+      await _writeMetadata(task, pkgPath);
+      // TODO: upload doc/api to the appropriate bucket


Ideally the bucket organization would use a scheme like gs://<bucket-name>/<dartdoc-version>/<package>/<version>/{index.html,...} here.

mkustermann · 2017-08-30T13:34:53Z

app/lib/dartdoc/dartdoc_runner.dart

+          await pubEnv.getLocation(task.package, version: task.version);
+      final pkgPath = pkgLocation.location;
+      await _runDartdoc(task, pkgPath);
+      await _writeMetadata(task, pkgPath);


If _runDartdoc failed, this call will fail, since the doc folder is not there.

TaskScheduler has a proper error handling, so we don't really need to worry about handling each of these errors separately. In fact, not having a doc directory helps us to get an exception without explicitly checking for it (and then throwing ourselves).

It's generally not advisable running commands / steps if they have assumptions about previous commands without ensuring that these previous commands completed successfully.

That's precisely why shell scripts often use set -e to abort after the first failed command.

Just assume that the package author already had a doc folder, then we might accidentally upload that, incorrect doc folder, to cloud storage and claim everything is fine, even though we didn't generate proper docs.

I'll add a comment further down about it.

mkustermann · 2017-08-30T13:36:44Z

app/lib/dartdoc/dartdoc_runner.dart

-    return false; // no race detected
+    final tempDir =
+        await Directory.systemTemp.createTemp('pub-dartlang-dartdoc');
+    final pubCacheDir = await tempDir.resolveSymbolicLinks();


Ideally we would reuse a pub cache, instead of just re-downloading all transitive dependencies for every single package we generate documentation for.

What is this PubEnvironment stuff doing?

Is pubCacheDir the real PUB_CACHE or is it a directory where this particular (package, version) tarball will be downloaded to?

We have had an issue with the analyzer service early on, when I've left out the temp dir creation and cleanup: the packages fill up the disk quickly.

In this case pubCacheDir is a clean environment, initialized and clear for every dartdoc run. However, pub cache does not download the transitive dependencies, only the specified package, so it is fine to use it a a clean env.

PubEnvironment encapsulates the various executables, commands and environment variables for pana. It felt better to reuse it here, than to reinvent everything.

... does not download the transitive dependencies ...

How does it then generate the correct docs? I mean if one package has a class inheriting from another package's class, how does dartdoc know about inherited members. It needs to know the other package's class for it.

Therefore I would naively conclude that dartdoc needs, in order to generate documentation for package A, the source for all packages A depends on (transitively).

PubEnvironment encapsulates the various executables, commands and environment variables for pana. It felt better
to reuse it here, than to reinvent everything.

It depends on whether it does the right thing. e.g. using the correct URL for downloading tarballs (to avoid hitting the memcache issue)

mkustermann · 2017-08-30T14:56:42Z

app/lib/dartdoc/dartdoc_runner.dart

-    return false; // no race detected
+    final tempDir =
+        await Directory.systemTemp.createTemp('pub-dartlang-dartdoc');
+    final pubCacheDir = await tempDir.resolveSymbolicLinks();


... does not download the transitive dependencies ...

How does it then generate the correct docs? I mean if one package has a class inheriting from another package's class, how does dartdoc know about inherited members. It needs to know the other package's class for it.

Therefore I would naively conclude that dartdoc needs, in order to generate documentation for package A, the source for all packages A depends on (transitively).

PubEnvironment encapsulates the various executables, commands and environment variables for pana. It felt better
to reuse it here, than to reinvent everything.

It depends on whether it does the right thing. e.g. using the correct URL for downloading tarballs (to avoid hitting the memcache issue)

mkustermann · 2017-08-30T14:57:36Z

app/lib/dartdoc/dartdoc_runner.dart

+
+    try {
+      final pkgLocation =
+          await pubEnv.getLocation(task.package, version: task.version);


We don't have to use pub to download the package! We only need pub to download a package's dependencies via pub get.

mkustermann · 2017-08-30T15:02:56Z

app/lib/dartdoc/dartdoc_runner.dart

+          await pubEnv.getLocation(task.package, version: task.version);
+      final pkgPath = pkgLocation.location;
+      await _runDartdoc(task, pkgPath);
+      await _writeMetadata(task, pkgPath);


It's generally not advisable running commands / steps if they have assumptions about previous commands without ensuring that these previous commands completed successfully.

That's precisely why shell scripts often use set -e to abort after the first failed command.

Just assume that the package author already had a doc folder, then we might accidentally upload that, incorrect doc folder, to cloud storage and claim everything is fine, even though we didn't generate proper docs.

I'll add a comment further down about it.

mkustermann · 2017-08-30T15:03:45Z

app/lib/dartdoc/dartdoc_runner.dart

+          'exitCode: ${pr.exitCode}\n'
+          'stdout: ${pr.stdout}\n'
+          'stderr: ${pr.stderr}\n');
+    }


Please throw an exception or give a boolean to the caller to indicate whether it was successful. If we fail to generate dartdocs, we should not try running gcs uploading code!

mkustermann · 2017-08-30T15:04:15Z

app/lib/dartdoc/dartdoc_runner.dart

+          'exitCode: ${pr.exitCode}\n'
+          'stdout: ${pr.stdout}\n'
+          'stderr: ${pr.stderr}\n');
+      return null;


Ditto, if we can't determine the version, we should not continue.

isoos · 2017-08-31T10:49:34Z

PTAL, I think I've addressed most of the concerns (dependencies are fetched, separate directory for the dartdoc content, throwing on dartdoc failures).

Postponed:

direct download (some pana support is required for analyzer too, want to do that the same time)
upload (btw. I'm not sure that the bucket file path needs to have the dartdoc version, a direct mirror of the dartdocs.org dir structure could be more beneficial)

mkustermann · 2017-09-01T08:18:41Z

upload (btw. I'm not sure that the bucket file path needs to have the dartdoc version, a direct mirror of the
dartdocs.org dir structure could be more beneficial)

The normal dartdocs URL space could just redirect to the active dartdoc-version GCS URLs.

There are multiple advantages of doing this: If we roll a new dartdoc version

it could change the entire look & feel: So we would have a time period where some of the generated docs have the old style and some new, it's not the best user experience
we avoid needing to delete old files & create new files (and have short time where there are no docs)
the new dartdoc version might have a bug and fail on some percentage of packages, then we can easily go back to the old available docs

mkustermann · 2017-09-01T08:24:37Z

direct download (some pana support is required for analyzer too, want to do that the same time)

I've filed dart-lang/pana#78, so we don't forget about it.

googlebot added the cla: yes label Aug 29, 2017

kevmoo approved these changes Aug 29, 2017

View reviewed changes

mkustermann reviewed Aug 30, 2017

View reviewed changes

isoos force-pushed the dartdoc_runner branch from 7301f6c to 6e293a5 Compare August 31, 2017 10:45

Run dartdoc for the given task. Related issue: dart-lang#227.

6e293a5

mkustermann merged commit b6f886a into dart-lang:master Sep 1, 2017

isoos mentioned this pull request Sep 1, 2017

Migrate documentation hosting to pub site #227

Closed

isoos deleted the dartdoc_runner branch September 1, 2017 12:30

Run dartdoc for the given task. #322

Run dartdoc for the given task. #322

Uh oh!

Conversation

isoos commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkustermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isoos commented Aug 31, 2017

Uh oh!

mkustermann commented Sep 1, 2017

Uh oh!

mkustermann commented Sep 1, 2017

Uh oh!

Uh oh!