Re: [gentoo-portage-dev] [PATCH] inital Manifest2 support

[Marius Mauch]

On Fri, 18 Nov 2005 18:40:14 -0600
Brian Harring <ferringb@g.o> wrote:

> On Wed, Nov 16, 2005 at 04:33:02AM +0100, Marius Mauch wrote:
> > -	digestfn  =
> > mysettings["FILESDIR"]+"/digest-"+mysettings["PF"]
> > +	digestfn  =
> > portdb.finddigest(mysettings["CATEGORY"]+"/"+mysettings["PF"])
> Like this mod personally, but don't like the '/'.  You're passing in
> two chunks of data, pass them in seperate imo, rather then collapsing
> it for every call.

Not sure I understand this, you mean changing finddigest() to use two
params? Doesn't really make sense to me, should just work for any CPV
entry.

> Kind of disliking the 'calledFor' being thrown around here, plus the
> v2 check, then resorting to a seperate method of checking.
> Integrate 'em potentially?
>
> You're basically treating it as two seperate funcs (from how I look
> at it), should split it up (even if the seperated funcs call each
> other, or a common func).

Yeah, was just going the least intrusive route for now, so keep the API
compatible. This wasn't intended for inclusion yet anyway, just some
"sneak preview" ;)

> > @@ -2218,7 +2229,7 @@
> >  	return 1
> >
> >
> > -def digestParseFile(myfilename):
> > +def digestParseFile(myfilename, calledFor="manifest"):
> >  	"""(filename) -- Parses a given file for entries matching:
> >  	MD5 MD5_STRING_OF_HEX_CHARS FILE_NAME FILE_SIZE
> >  	Ignores lines that do not begin with 'MD5' and returns a
> > @@ -2230,6 +2241,10 @@
> >
> >  	mydigests={}
> >  	for x in mylines:
> > +		tmp = portage_manifest.convertManifest2ToDigest(x,
> > calledAs=calledFor)
> > +		if tmp[0] == True:
> > +			mydigests[tmp[1]] = tmp[2]
> > +			continue
> >  		myline=string.split(x)
> >  		if len(myline) < 4:
> >  			#invalid line
> > @@ -2258,7 +2273,10 @@
> >  	the basedir given. Returns 1 only if all files exist and
> > match the md5s. """
> >  	for x in myfiles:
> > -		if not mydigests.has_key(x):
> > +		if
> > portage_manfest.getManifestVersion(basedir+"/Manifest") == 2 and
> > x.startswith("files/digest-"):
> hardcoding sucks, kthnx
> If possible, try to shove it into the func so modification (down the
> line) occurs in one spot, rather then having to resort to
> search/replace :)

Shove what into which func? I'm bad at guessing today.

>
> > Index: pym/portage_checksum.py
> > ===================================================================
> > --- pym/portage_checksum.py	(revision 2312)
> > +++ pym/portage_checksum.py	(working copy)
> > @@ -126,3 +126,12 @@
> >  		portage_locks.unlockfile(mylock)
> >
> >  	return (myhash,mysize)
> > +
> > +
> > +hashfunc_map = {"MD5": md5hash, "SHA1": sha1hash}
> > +
> > +def perform_multiple_checksums(filename, hashes=["MD5"],
> > calc_prelink=0):
> > +	rVal = {}
> > +	for x in hashes:
> > +		rVal[x] = perform_checksum(filename,
> > hashfunc_map[x], calc_prelink)[0]
> > +	return rVal
> Not sure if I like the [0] tbh.
> Still think the size test should be broken off as it's own pseudo
> hashfunc, and implemented in that way.  Course, would need to weight
> it somehow, so that size test is done first, then the more expensive
> chf tests...

Yep, see above about "least intrusive".

> > --- pym/portage_manifest.py	(revision 0)
> > +++ pym/portage_manifest.py	(revision 0)
> > @@ -0,0 +1,163 @@
> > +import os
> > +
> > +import portage, portage_exception, portage_checksum,
> > portage_versions +
> > +class Manifest2Entry:
> > +	def __init__(self, entrytype, name, config,
> > methods=portage_checksum.get_valid_checksum_keys()):
> The methods trick you're doing is a one time evaluation... not sure
> if that's the best approach tbh.
>
> > +		if entrytype not in ["EBUILD", "AUXFILE",
> > "MISCFILE", "SRCURI"]:
> > +			raise
> > portage_exception.PortageException("unknown Manifest entry type:
> > %s" % entrytype)
> break that out into a class constant, and please kill off the capps
> (they kill kittens, or something).

Ok on the constant, but I like the caps here ;)

> > +		self.type = entrytype
> > +		self.name = name
> > +		self.methods = methods
> > +		self.config = config
> I realize stable does this all over, but I really am not much for
> storing a config instance on this attribute.
>
> Thoughts on yanking the settings you need up front?

No problem, just took the easy route for now.

>
> > +	def generate(self, pkgdir, genOldStyle=True):
> > +		if self.type == "EBUILD" or self.type ==
> > "MISCFILE":
> > +			filename = pkgdir+"/"+self.name
> > +		elif self.type == "AUXFILE":
> > +			filename = pkgdir+"/files/"+self.name
> > +		elif self.type == "SRCURI":
> > +			filename =
> > self.config["DISTDIR"]+"/"+self.name
> > +		if not os.path.exists(filename):
> > +			raise
> > portage_exception.PortageException("file for Manifest entry doesn't
> > exist: %s" % filename)
> > +
> > +		self.size = os.stat(filename).st_size
> > +		self.checksums =
> > portage_checksum.perform_multiple_checksums(filename, self.methods)
> See comment about making size into a pseudo chf func- would eliminate
> this case.
> Chunking this up into type handlers might be wise- would make it
> easier to extend (modify a class dict rather then N funcs).  You've
> already got the seperated beast, just throw it into a mapping and
> it'll be nice and pretty.

Yeah, though somehow having a function just for filename composition
seems a bit overkill. Don't mind either way.

> > +		cs_list = [x+" "+self.checksums[x] for x in
> > self.checksums.keys()]
> > +		self.entry = " ".join([self.type, self.name,
> > str(self.size)]+cs_list)
> > +		if genOldStyle and self.type != "SRCURI":
> > +			if "MD5" in self.methods:
> > +				oldentry =
> > Manifest2Entry._makeOldEntry(pkgdir, filename, size=self.size,
> > md5sum=self.checksums["MD5"])
> > +			else:
> > +				oldentry =
> > Manifest2Entry._makeOldEntry(pkgdir, filename, size=self.size)
> Kinda iffy on this approach also, seems unclean (quite subjective I
> realize).

Better ideas welcome.

> > +def createFullManifest2(pkgdir, config, checkExisting=False,
> > requireAllSrcUriFiles=True, createOldDigests=True,
> > manifestCompatible=True, debug=1):
> > +	mydb = portage.db["/"]["porttree"].dbapi
> Really don't like that.
> Pass it in.

No problem.

> > +
> > +	if debug:
> > +		print "pkgdir:", pkgdir
> > +
> > +	cpv_list = [pkgdir.rstrip("/").split("/")[-2]+"/"+x[:-7]
> > for x in portage.listdir(pkgdir) if x.endswith(".ebuild")]
> os.path.sep...

Cosmetics.

> not much for the portage.listdir reference there either, kind of an
> indication the evil listdir/cachedir should be broken out into their
> own module.

Beyond the scope of this patch.

> > +
> > +	return (0, None)
> Eh?
> Exceptions seem like a better route here then magic constants and
> string error msgs.

Eh, I don't return error messages but the name of the object that
caused the failure. Wether to use exceptions for this comes down on who
is going to call this, don't really see the point in using exceptions
here as I expect only two or three callers (namely repoman and ebuild)
which have to act on the return code directly.

> > +def convertManifest2ToDigest(manifestline, calledAs="manifest"):
> > +	mysplit = manifestline.split()
> > +	if mysplit[0] not in ["EBUILD", "AUXFILE", "MISCFILE",
> > "SRCURI"] or ((len(mysplit) - 3) % 2) != 0:
> > +		return (False, None, None)
> > +	if mysplit[0] == "SRCURI" and calledAs == "manifest":
> > +		return (False, None, None)
> > +	if mysplit[0] in ["EBUILD", "AUXFILE", "MISCFILE"] and
> > calledAs == "digest":
> > +		return (False, None, None)
> > +
> > +	checksums = {}
> > +
> > +	mytype = mysplit.pop(0)
> > +	filename = mysplit.pop(0)
> > +	if mytype == "AUXFILE":
> > +		filename = "files/"+filename
> > +	size = long(mysplit.pop(0))
> > +	checksums = dict(zip(mysplit[::2], mysplit[1::2]))
> > +	if not checksums.has_key("size"):
> > +		checksums["size"] = size
> > +
> > +	return (True, filename, checksums)
> > +
> > +def getManifestVersion(manifest):
> > +	rVal = 0
> > +
> > +	if not os.path.exists(manifest):
> > +		return rVal
> > +	myfile = open(manifest, "r")
> > +	lines = myfile.readlines()
> > +	for l in lines:
> > +		mysplit = l.split()
> > +		if mysplit[0] == "MD5" and len(mysplit) == 4 and
> > rVal == 0:
> > +			rVal = 1
> > +		elif mysplit[0] in ["EBUILD", "AUXFILE",
> > "MISCFILE", "SRCURI"] and ((len(mysplit) - 3) % 2) == 0 and rVal in
> > [0,1]:
> > +			rVal = 2
> > +	return rVal
>
> Offhand... I really think you need to do some subclassing above.
> Flow's mostly there, but a lot of icky stuff for long term
> maintenance (imo).  Seems like a refactoring of the patch would bring
> line count down, and up readability.

Maybe, but I'm no such an OO maniac and also suck at refactoring ;)

Marius

-- 
Public Key at http://www.genone.de/info/gpg-key.pub

In the beginning, there was nothing. And God said, 'Let there be
Light.' And there was still nothing, but you could see a bit better.