Create some release automation for building Sphinx documentation

[khorn]

Split out from ticket #4500

Twisted should have some release automation machinery for building Sphinx documentation, in order to facilitate building the docs once they are converted.

Any automation created should live in twisted.python._release.

[khorn]

Previous review comments by glyph from #4500:

---

Hi khorn! Thanks for writing this branch.

1. First and foremost, I think that we should move this branch out to a different ticket. We can merge in some release-automation code to build the sphinx documentation before we actually commit all of the sphinx input, since reviewing the ReST sources may come up with other, unrelated issues. The more stuff we can get merged in small chunks, the better. Alternately we could open a different ticket for the ReST source migration, but this ticket seems to be pretty clearly aimed at that purpose. So you may want to respond to this review on a different ticket.
2. There's tons of trailing whitespace in this branch; it should be cleaned up.
3. SphinxBuilder.__doc__ isn't very clear.
   1. %SPHINXBUILD%? Give me a break. Hopefully nobody will ever actually run this on Windows (the release process definitely isn't supported there) so this syntax is pretty meaningless.
   2. The docstring should really say what the expectations for ALLSPHINXOPTS and BUILDDIR are.
   3. SphinxBuilder.build.__doc__ doesn't explain its parameters terribly well. What is the expected structure of docDir and buildDir? Where's the sphinx project supposed to be?
4. runCommand takes a list of strings, not a string. It happens to work as-is only because Popen (incorrectly) allows you to pass in a string. There are all kinds of potential problems with quoting this input.
5. Don't put # TODO markers in the code. If something needs to be done, file a new ticket. If not, just add an implementation note.
6. The comments about text/latex/pdf builders should similarly also be separate tickets.
7. Docstrings should always be """-style strings, not '''. Many of the tests use the latter style.
8. @ivar declarations ought to go in the class docstring, not method docstrings, so please move the ones from setUp. Thanks for thinking to document those, though :-).
9. For loops in tests are a bad idea. There's always the chance that you'll accidentally loop over nothing or somehow abort the loop early, and get a false positive. Even when the tests do their job correctly and fail on some bad data, a for loop means that when you get a failure you have to do forsensics to figure out which data element it failed on, since the traceback doesn't tell you. So, instead of writing tests like this:

    data = [[a, b], [c, d], [e, f]]
    for thunk in data:
        assertSomething(thunk)
        assertSomethingElse(thunk)
   

   write them like this:

    def doAsserts(thunk):
        assertSomething(thunk)
        assertSomethingElse(thunk)
    doAsserts([a, b])
    doAsserts([c, d])
    doAsserts([e, f])
   

10. SphinxBuilderTests docstring looks pretty incoherent: was this a debug / note to self that never got updated?
11. Some of the names are not coding-standard compliant. They should be confContent, indexContent, and test_failToBuild respectively.
12. A matter of personal taste: triple-quoted strings (aside from docstrings) should be indented so that their contents are further-indented than their enclosing suite. Since you're using textwrap to re-align them anyway, I figure that should be easy.
13. I'd really like it if this would include a command-line tool to update documentation in the right shape for the web site so that the release process doesn't develop another hole, which is currently plugged by an untested crappy thing on a wiki page. At the very least some new "documentation" (ha ha) of this form needs to be written.
14. As a minor detail,  the tests fail just about everywhere ;-) I believe this is because of the aforementioned string/list of strings Popen thing.

Thanks again Kevin for continuing your work on this. I am looking forward to putting this ticket behind us :-).

[khorn]

(In [32855]) Branching to 'sphinx-automation-5312'

[khorn]

Replying to glyph:

Hi khorn! Thanks for writing this branch. 1. First and foremost, I think that we should move this branch out to a different ticket. We can merge in some release-automation code to build the sphinx documentation before we actually commit all of the sphinx input, since reviewing the ReST sources may come up with other, unrelated issues. The more stuff we can get merged in small chunks, the better. Alternately we could open a different ticket for the ReST source migration, but this ticket seems to be pretty clearly aimed at that purpose. So you may want to respond to this review on a different ticket.

Obviously, I am. :)

2. There's tons of trailing whitespace in this branch; it should be cleaned up.

right.

3. SphinxBuilder.__doc__ isn't very clear.

1. %SPHINXBUILD%? Give me a break. Hopefully nobody will ever actually run this on Windows (the release process definitely isn't supported there) so this syntax is pretty meaningless.

Gotcha, this was a reminder to myself of what the command I was building should look like, and it was wasier to copy & paste something coherent out of the windwos .bat file rather than the make file, because, well, its a make file.

Removed. Or at least made comprehensible.

2. The docstring should really say what the expectations for ALLSPHINXOPTS and BUILDDIR are.

Done.

3. SphinxBuilder.build.__doc__ doesn't explain its parameters terribly well. What is the expected structure of docDir and buildDir? Where's the sphinx project supposed to be?

Done.

4. runCommand takes a list of strings, not a string. It happens to work as-is only because Popen (incorrectly) allows you to pass in a string. There are all kinds of potential problems with quoting this input.

Changed.

5. Don't put # TODO markers in the code. If something needs to be done, file a new ticket. If not, just add an implementation note.

Removed.

6. The comments about text/latex/pdf builders should similarly also be separate tickets.

We'll create these as we decide to add builders.

7. Docstrings should always be """-style strings, not '''. Many of the tests use the latter style.

Ah, that's work's style bleeding over into Twisted. I noticed that I had done it in _release.py, but apparently forgot to fix it in the tests.

Fixed now.

8. @ivar declarations ought to go in the class docstring, not method docstrings, so please move the ones from setUp. Thanks for thinking to document those, though :-).

Weird. Wonder why I did that.

Fixed.

9. For loops in tests are a bad idea. There's always the chance that you'll accidentally loop over nothing or somehow abort the loop early, and get a false positive. Even when the tests do their job correctly and fail on some bad data, a for loop means that when you get a failure you have to do forsensics to figure out which data element it failed on, since the traceback doesn't tell you. So, instead of writing tests like this: {{{ data = [[a, b], [c, d], [e, f]] for thunk in data: assertSomething(thunk) assertSomethingElse(thunk) }}} write them like this: {{{ def doAsserts(thunk): assertSomething(thunk) assertSomethingElse(thunk) doAsserts([a, b]) doAsserts([c, d]) doAsserts([e, f]) }}}

Done. I think.

10. SphinxBuilderTests docstring looks pretty incoherent: was this a debug / note to self that never got updated?

Gah, that was me making notes during our IRC conversation the other night, and putting them in a place where I wouldn't forget them. And then I forgot them.

11. Some of the names are not coding-standard compliant. They should be confContent, indexContent, and test_failToBuild respectively.

Work style bleeding over again. Fixed.

12. A matter of personal taste: triple-quoted strings (aside from docstrings) should be indented so that their contents are further-indented than their enclosing suite. Since you're using textwrap to re-align them anyway, I figure that should be easy.

OK.

13. I'd really like it if this would include a command-line tool to update documentation in the right shape for the web site so that the release process doesn't develop another hole, which is currently plugged by an untested crappy thing on a wiki page. At the very least some new "documentation" (ha ha) of this form needs to be written.

Isn't it kind of insane to create a command-line tool to drive a wrapper I'm writing to wrap a command-line tool? This is why I didn't want to do the release automation stuff in the first place. I thought we should just use sphinx-build.

14. As a minor detail,  the tests fail just about everywhere ;-) I believe this is because of the aforementioned string/list of strings Popen thing.

Oh! You wanted the tests to pass. :)

Thanks again Kevin for continuing your work on this. I am looking forward to putting this ticket behind us :-).

[khorn]

I've committed changes which hopefully address the above review comments, but I don't have time to test them out, so haven't re-submitted for review yet.

I'll take another look later on.

[khorn]

Ok, fixed the problem, I think.

 Build results here

[exarkun]

Thanks. This generally looks like a very nice branch. Just a few comments:

1. At least one of the tests still fails in various places, probably because assertGreater is a fairly new addition to unittest?
2. This point is not actionable. The dependency on Twisted Web in these tests goes the wrong way. Twisted Web depends on Twisted Core, not the other way around. So this potentially creates a circular dependency. However, since the stdlib HTML parser isn't really usable, this is probably a better idea (at least for now) than introducing a new dependency on a third party HTML parser. We should try hard not to repeat this sort of thing very often though.
3. in verifyFileExists, use fpath.getContent instead of open(fpath.path, ...)
4. also in verifyFileExists, the except: squashes the parse exception; someone might want to see that if the test ever fails.
5. (trac requires text here for the markup to parse properly)

   > Isn't it kind of insane to create a command-line tool to drive a wrapper I'm writing to wrap a command-line tool?
   

   It's the same thing we do for pydoctor - bin/admin/build-apidocs. And it's what we eventually would have done for lore if Sphinx hadn't come along (instead we stopped short). The point is to make the release process as simple as possible. That means a set of uniform tools (and eventually a single unified tool), not a lot of third-party tools each with their own interface and style. We want the release manager to be able to do ./build-docs [source] [dest], not some sphinx-build invocation with whatever command line arguments are hip that week (and definitely nothing like the mess of scripting liked above on the release process page).
6. Speaking of the existing tools though - these handle getting the necessary versioning information somehow. One tool infers it from the Twisted source tree being used, the other takes it as a command line argument. It looks like SphinxBuilder needs a feature in this area, I don't see anything related to versioning in this branch.
7. The for loop in SphinxBuilder.build. I guess it's there because at some point this code supported pdf/whatever, or at least there was some thinking about supporting those extra builders? If so, that makes sense, but since we're only going to support html right now, I'd just lose the loop. It's easy to re-add if/when we want it, but maybe we'll decide to support other builders in some other way. The (admittedly minimal) extra complexity isn't needed now, and (somewhat importantly) there's no test coverage, so this can't be tdd code.

Once again, nice branch overall, thanks!

[exarkun]

Oops, one other thing, SphinxBuilder's class docstring is not valid epytext. This seems to be because of the single :. epytext likes :: for introducing block quote-type stuff.

[khorn]

Replying to exarkun:

Thanks. This generally looks like a very nice branch. Just a few comments: 1. At least one of the tests still fails in various places, probably because assertGreater is a fairly new addition to unittest?

Changed to use assertTrue.

1. This point is not actionable. The dependency on Twisted Web in these tests goes the wrong way. Twisted Web depends on Twisted Core, not the other way around. So this potentially creates a circular dependency. However, since the stdlib HTML parser isn't really usable, this is probably a better idea (at least for now) than introducing a new dependency on a third party HTML parser. We should try hard not to repeat this sort of thing very often though.

Added a note in the docstring to warn future readers.

1. in verifyFileExists, use fpath.getContent instead of open(fpath.path, ...)

Done.

1. also in verifyFileExists, the except: squashes the parse exception; someone might want to see that if the test ever fails.

Is re-raising the exception enough here? Or should I do something entirely different?

1. (trac requires text here for the markup to parse properly) {{{

Isn't it kind of insane to create a command-line tool to drive a wrapper I'm writing to wrap a command-line tool?

}}} It's the same thing we do for pydoctor - bin/admin/build-apidocs. And it's what we eventually would have done for lore if Sphinx hadn't come along (instead we stopped short). The point is to make the release process as simple as possible. That means a set of uniform tools (and eventually a single unified tool), not a lot of third-party tools each with their own interface and style. We want the release manager to be able to do ./build-docs [source] [dest], not some sphinx-build invocation with whatever command line arguments are hip that week (and definitely nothing like the mess of scripting liked above on the release process page).

This makes sense, but I haven't addressed it yet.

1. Speaking of the existing tools though - these handle getting the necessary versioning information somehow. One tool infers it from the Twisted source tree being used, the other takes it as a command line argument. It looks like SphinxBuilder needs a feature in this area, I don't see anything related to versioning in this branch.

Hmmm. Not sure which direction to go here. Deferring to sometime not the middle of the night.

1. The for loop in SphinxBuilder.build. I guess it's there because at some point this code supported pdf/whatever, or at least there was some thinking about supporting those extra builders? If so, that makes sense, but since we're only going to support html right now, I'd just lose the loop. It's easy to re-add if/when we want it, but maybe we'll decide to support other builders in some other way. The (admittedly minimal) extra complexity isn't needed now, and (somewhat importantly) there's no test coverage, so this can't be tdd code.

Loop removed.

Once again, nice branch overall, thanks!

Thank you.

Oops, one other thing, SphinxBuilder's class docstring is not valid epytext. This seems to be because of the single :. epytext likes :: for introducing block quote-type stuff.

I think this is fixed, too.

[khorn]

(In [32957]) address some review comments

refs: #5312

[jonathanj]

Replying to exarkun:

It's the same thing we do for pydoctor - bin/admin/build-apidocs. And it's what we eventually would have done for lore if Sphinx hadn't come along (instead we stopped short). The point is to make the release process as simple as possible. That means a set of uniform tools (and eventually a single unified tool), not a lot of third-party tools each with their own interface and style. We want the release manager to be able to do ./build-docs [source] [dest], not some sphinx-build invocation with whatever command line arguments are hip that week (and definitely nothing like the mess of scripting liked above on the release process page).

This is related to #2380.

[tomprince]

(In [37452]) Branching to sphinx-automation-5312-2.