Re: [gentoo-portage-dev] [PATCH 1/2] Add FEATURES=binpkg-multi-instance (bug 150031)

[Brian Dolbec]

On Tue, 17 Feb 2015 11:26:27 -0800
Zac Medico <zmedico@g.o> wrote:

> On 02/17/2015 10:42 AM, Brian Dolbec wrote:
> >
> > overall, there is no way I know the code well enough to know if you
> > screwed up.  But the code looks decent, so...
> >
> > My only questions are:
> >
> > pym/portage/dbapi/bintree.py:
> >
> > You removed several functions from the binarytree class and
> > essentially reduced prevent_collision to a warning message.  Can
> > you briefly say why they are not needed please.
>
> Okay, I'll include this info in an updated patch:
>

Actually, I think this one patch could be split into a few logical
ones.  Tag them 

binpkg-multi-instance 1 of...

 in the commit message so it is clear they
belong together.  That way the commit messages can more clearly be
relevant to the file(s) changed.

The commit message is already a short story in length ;) before adding
these new explanations.


> _pkgindex_cpv_map_latest_build:
>
> This is what binhost clients running older versions of portage will
> use to select to select the latest builds when their binhost server
> switches to FEATURES=binpkg-multi-instance. New portage won't need
> this anymore because it is capable of examining multiple builds and
> it uses sorting to ensure that the latest builds are preferred when
> appropriate.
>
> _remove_symlink, _create_symlink, prevent_collision, _move_to_all, and
> _move_from_all:
>
> These are all related to the oldest PKGDIR layout, which put all of
> the tbz2 files in $PKGDIR/All, and created symlinks to them in the
> category directories. The $PKGDIR/All layout should be practically
> extinct by now. New portage recognizes all existing layouts, or
> mixtures of them, and uses the old packages in place. It never puts
> new packages in $PKGDIR/All, so there's no need to move packages
> around to prevent file name collisions between packages from
> different categories. It also only uses regular files (any symlinks
> are ignored).
>
> >    _populate()  ==> it is some 500+ LOC nasty.  From what I can see
> > I think you added slightly more loc than you deleted/changed.
> > While I don't expect a breakup of this function to be directly a
> > part of this commit.
> >
> >    IT IS BADLY NEEDED.
> >
> >    I'd like to see this function properly split into pieces and act
> > as a driver for the smaller tasks.  Currently it is a mess of nested
> >    if/else, for,... that is extremely difficult to keep straight and
> >    make logic changes to.  It's almost as bad as repomans 1000+ LOC
> >    main loop.  Why "if TRUE:" <== just fix the indent...  At the
> > same time, I noticed _ensure_dirs, _file_permissions functions
> > defined in that class. Shouldn't these just be util functions
> > available everywhere.
>
> Yeah, I will work on cleaning that up, and submit it as a separate
> patch.
>
> >
> >
> > pym/portage/versions.py
> >
> > class _pkg_str(_unicode)
> >
> > in __init__()
> >
> > 		if build_time is not None:
> > 			try:
> > 				build_time = long(build_time)
> > 			except ValueError:
> > 				if build_time:
> > 					build_time = -1
> > 				else:
> > 					build_time = 0
> > 		if build_id is not None:
> > 			try:
> > 				build_id = long(build_id)
> > 			except ValueError:
> > 				if build_id:
> > 					build_id = -1
> > 				else:
> > 					build_id = None
> > 		if file_size is not None:
> > 			try:
> > 				file_size = long(file_size)
> > 			except ValueError:
> > 				if file_size:
> > 					file_size = -1
> > 				else:
> > 					file_size = None
> > 		if mtime is not None:
> > 			try:
> > 				mtime = long(mtime)
> > 			except ValueError:
> > 				if mtime:
> > 					mtime = -1
> > 				else:
> > 					mtime = None
> >
> >
> > I hate repeating code.  This can be done using one universal small
> > function and will clean up the __init__()
> >
> >    @static_method
> >    def _long(var, default):
> > 	if var is not None:
> > 		try:
> > 			var = long(var)
> > 		except ValueError:
> > 			if var:
> > 				var = -1
> > 			else:
> > 				var = default
> > 	return var
>
> Okay, I'll do that and post the updated patch.



-- 
Brian Dolbec <dolsen>