Ticket #2941 enhancement closed fixed

Opened 6 years ago

Last modified 6 years ago

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

1

Changed 6 years ago by exarkun

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

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

2

Changed 6 years ago by exarkun

  • owner radix deleted
  • priority changed from normal to highest
  • keywords review added

ZOMG! please review

3

Changed 6 years ago by therve

  • owner set to exarkun
  • keywords review removed
  • 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!

4

Changed 6 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?

5

Changed 6 years ago by radix

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

6

Changed 6 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.

7

Changed 6 years ago by therve

  • owner set to exarkun
  • keywords review removed

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).

8

Changed 6 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.

9

Changed 6 years ago by therve

  • owner changed from therve to exarkun
  • keywords review removed

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

10

Changed 6 years ago by exarkun

(In [22771]) Import compat set

Refs #2941

:(

11

Changed 6 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

:(

12

Changed 6 years ago by exarkun

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

Refs #2941

:(

13

Changed 6 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

:(

14

Changed 6 years ago by exarkun

(In [22777]) Ignore warnings logged in test_stdout

Refs #2941 Refs #3070

:(

15

Changed 6 years ago by exarkun

  • owner exarkun deleted
  • keywords review added

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

16

Changed 6 years ago by therve

  • owner set to exarkun
  • keywords review removed

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

17

Changed 6 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

Fixed in r22795

18

Changed 3 years ago by <automation>

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