Closed Bug 1454640 Opened Last year Closed Last year

|mach doc| should use tools/docs/conf.py when generating sub-trees

Categories

(Firefox Build System :: Generated Documentation, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(6 files)

Right now, running |mach doc <path>| is very naive. It simply looks for a 'docs/conf.py' under <path> and runs sphinx from that directory. This is bad for a few reasons:

1) The conf.py under that subtree is a different configuration from the one in tools/docs/conf.py. This means the generated docs when using a <path> will look different from the ones that will eventually be published to firefox-source-docs.

2) Not all SPHINX_TREES even have a 'conf.py', so it isn't possible to generate them without generating the entire tree.

We should delete all 'conf.py' files from the tree except for the main one, and make sure that's the one we load whenever running |mach doc <path>|.

This change is a bit more complicated than it first appears. Since we still only want to generate the docs under <path>, we'll need some changes to mozbuild and the moztreedocs sphinx extension to ensure we aren't re-generating the entire thing each time. I have some patches that already accomplish this.

This doesn't technically depend on bug 1410424, but my local patches are based on top of the ones in there, so I'll wait for that to land first.
This isn't quite ready. Still have some cleanup and need to make sure it works on Windows. I'll be PTO all next week, so might be a while until I have a chance to submit this for review.
Attachment #8969618 - Flags: review?(core-build-config-reviews)
Attachment #8969619 - Flags: review?(core-build-config-reviews)
Attachment #8969620 - Flags: review?(core-build-config-reviews)
Attachment #8969621 - Flags: review?(core-build-config-reviews)
Attachment #8969622 - Flags: review?(core-build-config-reviews)
Attachment #8969623 - Flags: review?(core-build-config-reviews)
Apologies to whoever has to review this, this took quite a long time of digging into the doc system's internals and lots of trial and error to figure out :). That said, I tried to split this up as best I could and the commit messages should hopefully do a decent job explaining why the changes are being made. If anything is unclear, please ask or flag an isuse.

If you're inclined to test this locally, I expect the following:

1) $ ./mach doc                        # should take a long time, liveserver starts at the end
   $ <edit tools/lint/docs/index.rst>  # rebuilds only tools/lint/docs (relatively fast); changes refreshed in browser
                                       # a bug is that this will clobber the original index, but I don't know how to fix it

2) $ ./mach doc tools/lint             # should use the mozbuild sphinx extension (and be faster than the root generation)
   $ <edit tools/lint/docs/index.rst>  # editing + liveserver should work here as well
Comment on attachment 8969623 [details]
Bug 1454640 - [docs] Lazy load the package and version properties

https://reviewboard.mozilla.org/r/238404/#review248892
Attachment #8969623 - Flags: review+
Attachment #8969623 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969621 [details]
Bug 1454640 - [mozbuild] Ability to find sphinx variables relevant to a given path

https://reviewboard.mozilla.org/r/238400/#review248888
Attachment #8969621 - Flags: review+
Attachment #8969621 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969620 [details]
Bug 1454640 - [docs] Memoize the result of processing sphinx moz.build variables

https://reviewboard.mozilla.org/r/238398/#review248886
Attachment #8969620 - Flags: review+
Attachment #8969620 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969619 [details]
Bug 1454640 - [docs] Use a single SphinxManager instance across all rebuilds

https://reviewboard.mozilla.org/r/238396/#review248880
Attachment #8969619 - Flags: review+
Attachment #8969619 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969618 [details]
Bug 1454640 - [moztreedocs] Move 'create_tarball' into a package submodule

https://reviewboard.mozilla.org/r/238394/#review248878
Attachment #8969618 - Flags: review+
Attachment #8969618 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969622 [details]
Bug 1454640 - [docs] Always build docs with the tools/docs/conf.py

https://reviewboard.mozilla.org/r/238402/#review248890

This patch seems to make things much slower for me. Eg: Using './mach doc tools/lint' starts serving up the page in about ~1s prior to this patch, but ~6s after the patch. Similarly, reload times when modifying index.rst are slower. Is there something wrong with my setup?
Attachment #8969622 - Flags: review?(core-build-config-reviews)
Comment on attachment 8969622 [details]
Bug 1454640 - [docs] Always build docs with the tools/docs/conf.py

https://reviewboard.mozilla.org/r/238402/#review248890

Nope, unfortunately that's expected.

Before this patch, |mach doc| was doing two different things depeneding how you invoked it. With a subpath, it was more or less just shelling out to `sphinx-build <subpath>`. This is really quick, especially for small docs folders. Notably, this only worked if the `<subpath>` had its own `conf.py` file (which not all of our in-tree doc dirs do).

However, when running `mach doc` (with no subpath), it uses `tools/docs/conf.py`. This installs our `mozbuild.sphinx` extension which does all sorts of things like reading `moz.build` variables, generating python API docs and generating a custom index. This adds a fair bit of overhead.

This change makes sure that `mach doc <subpath>` also uses the `mozbuild.sphinx` extension along with all the extra baggage that entails. I believe this tradeoff is worth it because:

1. Now all `<subpath>`s will work, even if they don't have their own `conf.py`
2. Docs generated via `mach doc <subpath>` will actually look the same as they will when they get uploaded to `firefox-source-docs.mozilla.org` (previously, they might look entirely different depending on the contents of their custom `conf.py`).

So the tl;dr is that this patch sacrifices some performance for correctness. Hope that makes sense!

p.s if we wanted we could now remove all of the custom `conf.py` files from the tree. I didn't end up doing that in this patch, but I could file a follow-up to do this.
Comment on attachment 8969622 [details]
Bug 1454640 - [docs] Always build docs with the tools/docs/conf.py

https://reviewboard.mozilla.org/r/238402/#review248890

I'll also add there are some easy perf wins we can implement, though I'd like to do that in a follow-up:

1. Upgrade sphinx
2. Run sphinx with -J
3. Build main docs an api docs in parallel
Comment on attachment 8969622 [details]
Bug 1454640 - [docs] Always build docs with the tools/docs/conf.py

Let me know if the above comment made any sense or not.
Attachment #8969622 - Flags: review?(mshal)
Comment on attachment 8969622 [details]
Bug 1454640 - [docs] Always build docs with the tools/docs/conf.py

https://reviewboard.mozilla.org/r/238402/#review248986

Ahh, that makes sense - thanks!
Attachment #8969622 - Flags: review?(mshal) → review+
Sorry, realized I broke the 'doc' task in CI and the latest push fixes that:
https://treeherder.mozilla.org/#/jobs?repo=try&author=ahalberstadt@mozilla.com&fromchange=511f1f355724d2a9d54cbebb9d5c3c7b7fdaf602&tochange=6d5f258a64992041e71c5033b999e02052e6916d

It happened because we were importing moztreedocs in the mach command's __init__.py, which is before we install sphinx into the virtualenv. My only change was to pull the import out of __init__ and into a lazy-loaded property so that it runs after we populate the virtualenv.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9942d2df3719
[moztreedocs] Move 'create_tarball' into a package submodule r=mshal
https://hg.mozilla.org/integration/autoland/rev/8309212d820b
[docs] Use a single SphinxManager instance across all rebuilds r=mshal
https://hg.mozilla.org/integration/autoland/rev/30b4083534b5
[docs] Memoize the result of processing sphinx moz.build variables r=mshal
https://hg.mozilla.org/integration/autoland/rev/22f2dc60e6d8
[mozbuild] Ability to find sphinx variables relevant to a given path r=mshal
https://hg.mozilla.org/integration/autoland/rev/47fc3e223867
[docs] Always build docs with the tools/docs/conf.py r=mshal
https://hg.mozilla.org/integration/autoland/rev/d03f75986f62
[docs] Lazy load the package and version properties r=mshal
Sorry for the backout, I will reland this bug
Flags: needinfo?(ahalberstadt)
Backout by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb3895af484d
Backed out 6 changesets for bustage at build/src/security/manager/ssl/nsNSSComponent.cpp on a CLOSED TREE
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3783bfd78a0f
[moztreedocs] Move 'create_tarball' into a package submodule r=mshal
https://hg.mozilla.org/integration/autoland/rev/50bb4134a2e4
[docs] Use a single SphinxManager instance across all rebuilds r=mshal
https://hg.mozilla.org/integration/autoland/rev/611810b4fd66
[docs] Memoize the result of processing sphinx moz.build variables r=mshal
https://hg.mozilla.org/integration/autoland/rev/1381b6cea12d
[mozbuild] Ability to find sphinx variables relevant to a given path r=mshal
https://hg.mozilla.org/integration/autoland/rev/155a531ddcdd
[docs] Always build docs with the tools/docs/conf.py r=mshal
https://hg.mozilla.org/integration/autoland/rev/a71038f16918
[docs] Lazy load the package and version properties r=mshal on a CLOSED TREE
Flags: needinfo?(ahalberstadt)
See Also: → 1474313
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.