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 636C4138A1A for ; Tue, 17 Feb 2015 19:56:12 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id CEAD6E0804; Tue, 17 Feb 2015 19:56:10 +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 436B9E07FE for ; Tue, 17 Feb 2015 19:56:10 +0000 (UTC) Received: from big_daddy.dol-sen.ca (S010634bdfa9ecf80.vc.shawcable.net [96.49.31.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 4EF4F33BF44 for ; Tue, 17 Feb 2015 19:56:09 +0000 (UTC) Date: Tue, 17 Feb 2015 11:56:07 -0800 From: Brian Dolbec To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH 1/2] Add FEATURES=binpkg-multi-instance (bug 150031) Message-ID: <20150217115607.0eccc56a.dolsen@gentoo.org> In-Reply-To: <54E395E3.6060500@gentoo.org> References: <1424162233-25071-1-git-send-email-zmedico@gentoo.org> <20150217104240.1db9be28.dolsen@gentoo.org> <54E395E3.6060500@gentoo.org> Organization: Gentoo 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 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Archives-Salt: f3458d17-ebf4-460d-a2c0-24003b73791d X-Archives-Hash: 9fadb0843754281a09b50786c9cae34f On Tue, 17 Feb 2015 11:26:27 -0800 Zac Medico 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