Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3895 closed enhancement (implemented)

Thp package handling inside Thandy

Reported by: chiiph Owned by: nickm
Priority: Medium Milestone:
Component: Archived/Thandy Version:
Severity: Keywords:
Cc: erinn, g.koppen@… Actual Points:
Parent ID: #4460 Points:
Reviewer: Sponsor:

Description

I've implemented the thp spec in my branch chiiph/thp.

It works on the dummy packages I've created, and since I can't find anything really wrong with the code, I thought I'd better put it out for review.

The repo is: https://git.torproject.org/user/chiiph/thandy.git

Should we use this same ticket to may be finish the spec itself?

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by chiiph

Status: newneeds_review

comment:2 Changed 8 years ago 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.

comment:3 in reply to:  2 Changed 8 years ago by chiiph

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

comment:4 in reply to:  2 ; Changed 8 years ago by chiiph

And here are the changes:

Everything was done in my thp branch.

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

comment:5 in reply to:  4 Changed 8 years ago by rransom

Replying to chiiph:

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

import posixpath

comment:6 Changed 8 years ago by gk

Cc: g.koppen@… added

comment:8 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged!

comment:9 Changed 8 years ago by chiiph

Parent ID: #4460
Note: See TracTickets for help on using tickets.