From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 4A8D9138A1A for ; Tue, 17 Feb 2015 19:26:32 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 27658E0864; Tue, 17 Feb 2015 19:26:31 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 82035E0850 for ; Tue, 17 Feb 2015 19:26:30 +0000 (UTC) Received: from [10.0.31.59] (unknown [100.42.98.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: zmedico) by smtp.gentoo.org (Postfix) with ESMTPSA id 4EB283407ED for ; Tue, 17 Feb 2015 19:26:29 +0000 (UTC) Message-ID: <54E395E3.6060500@gentoo.org> Date: Tue, 17 Feb 2015 11:26:27 -0800 From: Zac Medico User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@lists.gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org MIME-Version: 1.0 To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH 1/2] Add FEATURES=binpkg-multi-instance (bug 150031) References: <1424162233-25071-1-git-send-email-zmedico@gentoo.org> <20150217104240.1db9be28.dolsen@gentoo.org> In-Reply-To: <20150217104240.1db9be28.dolsen@gentoo.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Archives-Salt: 17398b98-666e-4d2e-baca-c40a143c37ae X-Archives-Hash: 7cc202a7c933df147f718e2d70ec2b1d 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