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

 Replying to [comment:2 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?

 Afaict, it's spec-compliant. The spec had some ambiguities, that I
 discussed in #tor-dev while working on them. May be I should grab the spec
 and the code, and try patching the spec with the disambiguation path I
 took?

 > Has it installed any programs?

 Only dummy packages so far. But there isn't any indication that it could
 fail with a real package. It's just copying files after all.

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

 Hm, yes.

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

 You mean returning the ThpTransaction as an installable package itself?
 In one of the discussions we agreed that Thp would be the only kind of
 package we will use, and that we can get rid of rpm and exe. But returning
 the transaction as part of installable seems like a good idea, even if we
 get rid of everything else.

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

 Sure, I just thought it would integrate nicely with the rest of the
 workflow. But it can be a separate CLI without much trouble.

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

 Will do.

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

 Hm, yes, I see your point. I need the tmpPath just for the metadata file.

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

 I'll fix that.

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

 Will do.

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

 Hm, yeah.

 Ok. I just wanted to comment on the things that may lead to some
 discussion. I'll update this again with commitdiffs for every point in a
 few days.

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