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

[Zac Medico]

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:

_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.
-- 
Thanks,
Zac