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

[Brian Dolbec]

On Tue, 17 Feb 2015 00:37:12 -0800
Zac Medico <zmedico@g.o> wrote:

> FEATURES=binpkg-multi-instance causes an integer build-id to be
> associated with each binary package instance. Inclusion of the
> build-id in the file name of the binary package file makes it
> possible to store an arbitrary number of binary packages built from
> the same ebuild.
>
> Having multiple instances is useful for a number of purposes, such as
> retaining builds that were built with different USE flags or linked
> against different versions of libraries. The location of any
> particular package within PKGDIR can be expressed as follows:
>
> 	${PKGDIR}/${CATEGORY}/${PN}/${PF}-${BUILD_ID}.xpak
>
> The build-id starts at 1 for the first build of a particular ebuild,
> and is incremented by 1 for each new build. It is possible to share a
> writable PKGDIR over NFS, and locking ensures that each package added
> to PKGDIR will have a unique build-id. It is not necessary to migrate
> an existing PKGDIR to the new layout, since portage is capable of
> working with a mixed PKGDIR layout, where packages using the old
> layout are allowed to remain in place.
>
> The new PKGDIR layout is backward-compatible with binhost clients
> running older portage, since the file format is identical, the
> per-package PATH attribute in the 'Packages' index directs them to
> download the file from the correct URI, and they automatically use
> BUILD_TIME metadata to select the latest builds.
>
> There is currently no automated way to prune old builds from PKGDIR,
> although it is possible to remove packages manually, and then run
> 'emaint --fix binhost' to update the ${PKGDIR}/Packages index.
>
> It is not necessary to migrate an existing PKGDIR to the new layout,
> since portage is capable of working with a mixed PKGDIR layout, where
> packages using the old layout are allowed to remain in-place.
>
> There is currently no automated way to prune old builds from PKGDIR,
> although it is possible to remove packages manually, and then run
> 'emaint --fix binhost' update the ${PKGDIR}/Packages index. Support
> for FEATURES=binpkg-multi-instance is planned for eclean-pkg.
>
> X-Gentoo-Bug: 150031
> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=150031
> ---
>  bin/quickpkg                                       |   1 -
>  man/make.conf.5                                    |  27 +
>  pym/_emerge/Binpkg.py                              |  33 +-
>  pym/_emerge/BinpkgFetcher.py                       |  13 +-
>  pym/_emerge/BinpkgVerifier.py                      |   6 +-
>  pym/_emerge/EbuildBinpkg.py                        |   9 +-
>  pym/_emerge/EbuildBuild.py                         |  36 +-
>  pym/_emerge/Package.py                             |  67 +-
>  pym/_emerge/Scheduler.py                           |   6 +-
>  pym/_emerge/clear_caches.py                        |   1 -
>  pym/_emerge/resolver/output.py                     |  21 +-
>  pym/portage/const.py                               |   2 +
>  pym/portage/dbapi/__init__.py                      |  10 +-
>  pym/portage/dbapi/bintree.py                       | 842
> +++++++++++----------
> pym/portage/dbapi/vartree.py                       |   8 +-
> pym/portage/dbapi/virtual.py                       | 113 ++-
> pym/portage/emaint/modules/binhost/binhost.py      |  47 +-
> pym/portage/package/ebuild/config.py               |   3 +-
> pym/portage/tests/resolver/ResolverPlayground.py   |  26
> +- .../resolver/binpkg_multi_instance/__init__.py     |   2
> + .../resolver/binpkg_multi_instance/__test__.py     |   2
> + .../binpkg_multi_instance/test_rebuilt_binaries.py | 101 +++
> pym/portage/versions.py                            |  48 +- 23 files
> changed, 932 insertions(+), 492 deletions(-) create mode 100644
>
>


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.

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


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



-- 
Brian Dolbec <dolsen>