[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 chiiph):

 And here are the changes:

 Everything was done in my thp branch.

 Replying to [comment:2 nickm]:
 > All files seem to have become executable.  Git mistake, I assume.

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/2d5be8cb7b4fe527df86751bb3e1f27d8cf9cd65

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

 Still WIP.

 I'm having some trouble figuring out the best way of handling this.
 Because we have changed the logic of "install a package" to "install a
 bundle", because of the dependencies established inside a thp package.

 If we change the general logic to "install a bundle", then the
 PS.Installer would be the ThpTransaction instead of the ThpInstaller, and
 canInstall() would be the transactions isReady().
 The transactions would be created inside repository.getFilesToUpdate(),
 just before the last return.
 How does that sound?

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

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/48cdba812c660fccd16052f7f2119b867a736ec2

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

 I don't see a reason for this if only the files in the manifest are added.
 May be to let the user know that she might've forgotten to add a file?

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

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/15de7549fbc0d9ea111710e6a3d46c74cd76ab67

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

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/ae54ecfe477546cff95d3c6b2ffafbf7314d0554

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

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/53e9b2b3c0b10b3b0075da6acc59c032553813b7

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

 https://gitweb.torproject.org/user/chiiph/thandy.git/commitdiff/78e260013786d048589172a74dbe6288ef4edba6

 This separator thing should be added to the spec.

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