Opened 7 years ago

Closed 7 years ago

#2941 enhancement closed fixed (fixed)

Need first-class Book generation code

Reported by: radix Owned by:
Priority: highest Milestone: twisted-8.0
Component: release management Keywords:
Cc: Branch: branches/book-builder-2941
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

admin/process-docs can build a .pdf of our developer (user) guide. We need to be able to do this in a well tested and documented way, so that we can distribute the results on our web site.

Change History (18)

comment:1 Changed 7 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/book-builder-2941

(In [22615]) Branching to 'book-builder-2941'

comment:2 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner radix deleted
  • Priority changed from normal to highest

ZOMG! please review

comment:3 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • Why using os.system? Can't we use our awesome spawnProcess API?
  • test_build, test_buildPDF and test_noSideEffects need latex to be installed (among other things). That doesn't look very unity :). You can probably parametrize run for not really running the commands. If we want a real functional test involving latex, it should be skipped when necessary.
  • build should probably have an option for removing generated tex files once the pdf is created
  • BookBuilderTests.getArbitraryOutput has an empty docstring

Thanks!

comment:4 Changed 7 years ago by radix

  1. The mixing could have just been a free function, since it doesn't use anything on self. Of course, the way you did it means that it can be overridden in subclasses. I just wanted to point it out, I'm not bothered about it.
  2. can you please write a better test for the PDF generation? I realize asserting the contents of the PDF would be weird, but perhaps you could mock the system execution facilities and make sure the expected command line is being run?

comment:5 Changed 7 years ago by radix

For example, the tests continue to pass if I change the number of times that texToDVI is run.

comment:6 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • The tests are skipped if the necessary executables are unavailable
  • There's a test for the number of times latex is run
  • getArbitraryOutput has a docstring
  • The generated tex files are removed

The code uses popen4 now instead of os.system. It doesn't use spawnProcess because I don't expect (and radix confirms) that the reactor will actually be running for this tool.

I tried to add some tests that don't run child processes at all, but since lots of stuff depends on the output of the commands it was pretty hard, so I gave up.

comment:7 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

The code looks great, not many things to say about it. I've generated the book, and have 2 comments:

  • pnmtops prints stuff (probably on stderr?). For example, pnmtops: writing color PostScript...
  • CommandFailed should have a string representation containing the output, or at least a part of it, because it's hard to find out a problem without (in my case, I forgot to generate the man pages before).

comment:8 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to therve

I think pnmtops is a separate task, I'll create a ticket.

The latex output is pretty long generally, I'd rather not see all that by default when the exception happens. The higher-level tool can report this to the user, I think. How does that sound?

I made buildTeX fail in a better way (reject non-existent directories) since I mistyped something while playing with the code interactively in response to your review.

comment:9 Changed 7 years ago by therve

  • Keywords review removed
  • Owner changed from therve to exarkun

Alright, that's ok for me, +1.

comment:10 Changed 7 years ago by exarkun

(In [22771]) Import compat set

Refs #2941

:(

comment:11 Changed 7 years ago by exarkun

(In [22772]) Make popen2.Popen4 only necessary for the BookBuilder tests, not the rest. It is not available on Windows.

Refs #2941

:(

comment:12 Changed 7 years ago by exarkun

(In [22773]) Pass CommandFailed args to Exception so the repr shows more information useful for debugging.

Refs #2941

:(

comment:13 Changed 7 years ago by exarkun

(In [22775]) Use a directory in /tmp instead of next to the input directory since ps2pdf13 freaks out if paths are very long

Refs #2941

:(

comment:14 Changed 7 years ago by exarkun

(In [22777]) Ignore warnings logged in test_stdout

Refs #2941
Refs #3070

:(

comment:15 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Some problems when I ran the branch on the supported builders before merging. Fixed referenced above.

comment:16 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Sorry for not checking further, buildbot seems happy now. Please merge.

comment:17 Changed 7 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r22795

comment:18 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.