[tor-bugs] #3895 [Thandy]: Thp package handling inside Thandy

[Tor Bug Tracker & Wiki]

#3895: Thp package handling inside Thandy
-------------------------+--------------------------------------------------
 Reporter:  chiiph       |          Owner:  nickm       
     Type:  enhancement  |         Status:  needs_review
 Priority:  normal       |      Milestone:              
Component:  Thandy       |        Version:              
 Keywords:               |         Parent:              
   Points:               |   Actualpoints:              
-------------------------+--------------------------------------------------

Comment(by nickm):

 Reviewing by looking at the diff, since that's slightly shorter than
 the history.

 Generally looks pretty good!  Seems like once the stuff below is
 cleaned up we can merge and test more.  What's the current status on
 spec-compliance and testing of this?  Has it installed any programs?


 All files seem to have become executable.  Git mistake, I assume.

 In ClientCLI, why is installable still there?  Looks like it's
 disused, and you just ripped out any idea that there could be
 another kind of installable package.  Why not return these
 "transactions" as "installable", enhancing the interface to
 "installable" as necessary?  Previously, ClientCLI didn't need to
 know about particular package systems.  This does what I'd hoped it
 wouldn't: it not only breaks the existing RPM/EXE packagesystems,
 but it changes the generic code to make it THP-only.

 In SignerCLI, I don't think making a package is naturally part of
 being a "signer"; it's something else.  (SignerCLI didn't have a way
 to delegate to rpmbuild, after all.)

 Also, in SignerCLI, I think the "make a package" function should
 warn if there are any unused files in the scripts directory, or any
 parts of the data directory that aren't in the manifest.

 Also, I think I'm missing something key there: calling
 shutil.copytree(dataPath, os.path.join(tmpPath,"contents")) copies
 *everything* from dataPath; why not copy only the things from the
 manifest?  And why copy all this stuff at all and mess around with
 tempfiles?  Why not add to the zip file directly from the dataPath?

 The new code in ThpPackages doesn't seem to have any comments or
 documentation.

 In ThpDB, you've got a few check-then-open blocks where you do (the
 equivalent of)

    if os.path.exits(foo):
      open(foo)

 In general, this kind of code has a potential race condition that
 automatic analysis tools like to complain about, even if you're
 using lock files to (hopefully) prevent it.  Better to do


    try:
       open(foo)
    except OSError, e:
       ...


 In ThpInstaller, the only valid way to split paths in python is
 os.path.join and os.path.split.  Doing string manipulation with
 os.path.sep is not in fact guaranteed portable.  Furthermore,
 consider what happens if a package is built on one platform but
 installed on another.  Probably we should specify that Thp paths are
 always separated with /, but installed with platform-local
 separators.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3895#comment:2>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online