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

[Brian Harring]

On Tue, Nov 22, 2005 at 09:41:46PM +0100, Marius Mauch wrote:
> 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.

Well, I'd personally prefer passing in an object, but that's 
probably not going to occur ;-)

using two params deviates from what we do in other funcs, but it also 
removes hardcoding of path seperator.  For windows support (yes it's 
nuts, but bear with) it would have to split and rejoin the path, 
converting from '/' to os.path.sep.

Pass it in as two seperated components, it can join it internally on 
it's own (if/when we N deep categories, it'll have to split that param 
though).

Debatable obviously, I just dislike the requirement of knowing a heck 
of a lot more about the func prototype then is required-
finddigest(category, package, version) vs 
finddigest(category+"/"+package+"-"+version) fex.  One I can figure 
out from pydoc, the other I'll probably dig up an example in the code 
to see if it's cp or cpv.


> > 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" ;)

Iron out the implementation a bit, and I personally wouldn't have 
issues with it going into svn.

Helluva lot easier to hack on it out of a repo, then via a set of 
patches anyways :)

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

Well, you're hardcoding /Manifest for all calls to it.

Kind of an indication the api should be changed and internally handle 
it imo, or encapsulate in some other fashion.

Yes it's anal, but new additions to portage api I'm after having saner 
functions in use rather then what's there now.  Saner is relative 
obviously, but assume you get what I mean.

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

I can live with intrusive if the new code is better quality then 
existing- stable code sucks, if it needs to be slapped around a bit to 
play nice with new code, works for me.  Chunking size off so it's no 
longer a special case makes sense in my opinion, since otherwise 
each/every chksum func will have to do it's own stat and return size 
just to maintain a similar return to md5.


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

Constants kill kittens.

You don't want to kill kittens, do you? ;)

> > > +	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.
Eliminates a special case though; see comment above about alternative 
chf funcs having to return a list instead of the chksum, and why this 
should be done for the long haul should be clear :)

> > > +		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.
Seperated class potentially, although kind of overkill.

Could potentially make it easier to seperate new and old though, so 
that old can be thrown out easier down the line (instead of an 
invasive removal of code from the combined class).

Haven't looked at the code since the first review, so the paragraph 
above could be mildly insane. :)

> > > +
> > > +	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.
Readability/maintainability ;)

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

Those two or three callers are going to have some funky voodoo in 
place that a year down the line, I'm going to scratch my head over 
(since I'm an idiot).

You're passing data back and flagging an error, exception is really 
the route to go.   It's extensible mainly, such that if the '2 or 3' 
becoms '5 or 6' we don't have magic constants/voodoo spreading through 
the code.

> > > +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 ;)
Might want to see what you can raid from saviors portage.fs.*

Yes it's extra, but the fs objs will wind up in a stable portage 
sooner or later for the contents benefits, and in this case the 
encapsulation might be useful for you.
~harring