939367 - Facilitate Sphinx documentation from all parts of the tree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

In bug 917988 we added Sphinx docs for build system documentation. I've heard rumblings that other groups would like in on the party.

I was on a plane today and worked on expanding the Sphinx docs to be generic and work with docs defined in any tree. Will submit a patch in a few minutes.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Allow Sphinx docs to come from all over the tree

And here's my vision.

I moved the generic Sphinx docs foo into tools/docs. I added some
moz.build symbols for declaring directories to include in the Sphinx doc
tree and for defining directories containing Python packages.

I also added a mode to read moz.build files via os.walk() instead of
using the internal traversal data (TIERS, DIRS) from moz.build files.
glandium had expressed interest in doing this, so I went ahead and
implemented it for this patch.

I originally had a config file listing all the directories defining
Sphinx doc dirs and Python packages. But, that violates the principle of
having everything for a particular "module" being defined in a single
directory. We have moz.build files for defining metadata, let's use
them. Although, if you want to use a separate file for defining Sphinx
metadata so we can keep moz.build files build config specific, we can
os.walk for that instead. But, I anticipate that over time Sphinx docs
will be part of the build, so I imagine they'll make it into moz.build
eventually. But YAGNI applies.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Allow Sphinx docs to come from all over the tree, v2

I made it so you don't need to edit index.rst when adding new doc trees.

I made the performance of os.walk much better by skipping ignored directories and ignoring .hg and .git.

I added docs on how everything works.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 833368 [details] [diff] [review]
Allow Sphinx docs to come from all over the tree, v2

Review of attachment 833368 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/docs/conf.py
@@ -1,1 @@
> -# This Source Code Form is subject to the terms of the Mozilla Public

Please hg move this file.

::: python/moz.build
@@ +7,5 @@
> +SPHINX_PYTHON_PACKAGE_DIRS += [
> +    ('codegen',),
> +    ('mozbuild/mozbuild', 'test'),
> +    ('mozbuild/mozpack', 'test'),
> +    ('mozversioncontrol/mozversioncontrol',),

The syntax is really awkward and without reading sandbox_symbols.py, there's no way to guess what it means and why one would need tuples even without something to exclude. Can't we just say that we skip 'test' directories everywhere, and people wanting to use sphinx have to be aware of that?

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +622,5 @@
> +        data from executed moz.build files to drive traversal.
> +
> +        This is a generator of Sandbox instances.
> +        """
> +        # FUTURE we should have an API for walking topsrcdir.

Future should probably use python AST to find all assignments to DIRS instead. Same applies for SPHINX*, by the way, because you're currently relying on the fact that they're at a convenient place, and that running the rest of the moz.build doesn't have side effects (which is why you currently have to modify some of the moz.builds).

@@ +625,5 @@
> +        """
> +        # FUTURE we should have an API for walking topsrcdir.
> +        ignores = [
> +            '.hg',
> +            '.git',

I think you should just ignore everything starting with a ., which using FileFinder from mozpack would do for you.

::: tools/docs/conf.py
@@ +27,5 @@
> +project = u'Mozilla Source Tree'
> +year = datetime.now().year
> +
> +# Grab the version from the source tree's milestone.
> +with open(os.path.join(mozilla_dir, 'config', 'milestone.txt'), 'rt') as fh:

Followup fodder: port milestone.pl to python and use that.

::: tools/docs/index.rst
@@ +28,5 @@
> +--------------------
> +
> +To add new documentation, define the ``SPHINX_TREES`` and
> +``SPHINX_PYTHON_PACKAGE_DIRS`` variables in ``moz.build`` files in
> +thre tree and documentation will automatically get picked up.

the

@@ +45,5 @@
> +   e.g. ``SPHINX_TREES['featureX'] = 'docs'``. This says *the ``docs``
> +   directory under the current directory should be installed into the
> +   Sphinx documentation tree under ``/featureX``*.
> +4. If you have Python packages you would like to generate Python API
> +   documentation for, you can use ``SPHINX_PYTHON_PACAKGE_DIRS`` to

PACKAGE

@@ +47,5 @@
> +   Sphinx documentation tree under ``/featureX``*.
> +4. If you have Python packages you would like to generate Python API
> +   documentation for, you can use ``SPHINX_PYTHON_PACAKGE_DIRS`` to
> +   declare directories containing Python packages. e.g.
> +   ``SPHINX_PYTHON_PACKAGE_DIRS += ('mozpackage',)``.

This doesn't match what's in python/moz.build.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #3)
> ::: python/mozbuild/mozbuild/frontend/reader.py
> @@ +622,5 @@
> > +        data from executed moz.build files to drive traversal.
> > +
> > +        This is a generator of Sandbox instances.
> > +        """
> > +        # FUTURE we should have an API for walking topsrcdir.
> 
> Future should probably use python AST to find all assignments to DIRS
> instead. Same applies for SPHINX*, by the way, because you're currently
> relying on the fact that they're at a convenient place, and that running the
> rest of the moz.build doesn't have side effects (which is why you currently
> have to modify some of the moz.builds).

Using AST to find all DIRS is interesting. Could be difficult if a variable (as opposed to a string literal) is assigned to a *DIRS.

moz.build evaluation should be free of side-effects other than populating an in-memory data structure. There is a reason why we strive to have sandboxing between individual moz.build files as much as possible. export() is a notable exception (which I didn't care much for when we introduced it for this reason).

Comment 5

[Gregory Szorc [:gps]]

Attachment: Allow Sphinx docs to come from all over the tree

Addressed review comments modulo directory traversal foo, which I think
is out of scope. We can always change how directory traversal works in a
followup patch.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8335614 [details] [diff] [review]
Allow Sphinx docs to come from all over the tree

Review of attachment 8335614 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/docs/conf.py
@@ -1,1 @@
> -# This Source Code Form is subject to the terms of the Mozilla Public

Please hg move this file instead of hg rm/hg add.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +636,5 @@
> +        # Ignore object directories.
> +        ignores |= {p for p in os.listdir(self.topsrcdir) if p.startswith('obj')}
> +
> +        ignores = {os.path.normpath(os.path.join(self.topsrcdir, p))
> +            for p in ignores}

cf. comments on the FileFinder patch.

You're also not ignoring test directories besides the mozbuild one.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Allow Sphinx docs to come from all over the tree

Fixed up review comments and rebased on top of new FileFinder ignore
API.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Allow Sphinx docs to come from all over the tree

Upgraded to Sphinx theme 0.4, which makes the page title and other
breadcrumbs report proper values (not MDN specific things).

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8345039 [details] [diff] [review]
Allow Sphinx docs to come from all over the tree

Review of attachment 8345039 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +629,5 @@
> +        # for DIRS references in the AST, even if a directory is added behind
> +        # a conditional. For now, just walk the filesystem and intelligently
> +        # exclude.
> +        ignore = {
> +            'python/mozbuild/mozbuild/test',

The comment about other test directories still applies. Maybe **/test?

@@ +633,5 @@
> +            'python/mozbuild/mozbuild/test',
> +        }
> +
> +        # Ignore object directories.
> +        ignore |= {p for p in os.listdir(self.topsrcdir) if p.startswith('obj')}

How about adding 'obj*' to ignore, instead?

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eec37596a6d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0eec37596a6d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7091d53013f6