[elbe-devel] [PATCH v2 02/13] use qemu from backports instead of self packaged

[Manuel Traut]

On Thu, Dec 07, 2017 at 09:34:34AM +0100, Torben Hohn wrote:
> On Wed, Dec 06, 2017 at 03:18:22PM +0100, John Ogness wrote:
> > On 2017-12-06, Torben Hohn <torben.hohn at linutronix.de> wrote:
> > >>>> diff --git a/elbepack/init/default-init.xml b/elbepack/init/default-init.xml
> > >>>> index e1f61ed7..2df7a3f7 100644
> > >>>> --- a/elbepack/init/default-init.xml
> > >>>> +++ b/elbepack/init/default-init.xml
> > >>>> @@ -42,6 +42,7 @@
> > >>>>  		<pkg-list>
> > >>>>  			<pkg>openssh-server</pkg>
> > >>>>  			<pkg>debootstrap/jessie-backports</pkg>
> > >>>> +			<pkg>qemu-user-static/jessie-backports</pkg>
> > >>>
> > >>> Please do not use this syntax here.
> > >>> We have a dbsfed syntax for this:
> > >>> <pkg origin="jessie-backports">qemu-user-static</pkg>
> > >> 
> > >> I think this is a good idea. But requires extending XML schema:
> > >
> > > sorry. the field is called pin.
> > > this name is not good... but i believe thats what its used for.
> > >
> > > we currently have this:
> > >
> > > ------------------------------------------------------------
> > > % for n in pkgs:
> > > %  if "pin" in n.et.attrib.keys():
> > > Package: ${n.et.text}
> > > Pin: release a=${n.et.attrib["pin"]}
> > > Pin-Priority: 991
> > > ------------------------------------------------------------
> > >
> > > do we really want a= or o= ? 
> > > or both ? 
> > 
> > AFAIK, elbe discourages use of the archive/suite name
> > (i.e. stable/oldstable) because these change with each release. So if
> > only the codenames are used (which they should be) we should be using n=
> > for pin instead of a=.
> > 
> > In that case, I would suggest the following changes (instead of the ones
> > we are commenting on):
> > 
> > diff --git a/elbepack/init/default-init.xml b/elbepack/init/default-init.xml
> > index e1f61ed7..f0af7e88 100644
> > --- a/elbepack/init/default-init.xml
> > +++ b/elbepack/init/default-init.xml
> > @@ -41,7 +41,8 @@
> >  		<suite>jessie</suite>
> >  		<pkg-list>
> >  			<pkg>openssh-server</pkg>
> > -			<pkg>debootstrap/jessie-backports</pkg>
> > +			<pkg pin="jessie-backports">debootstrap</pkg>
> > +			<pkg pin="jessie-backports">qemu-user-static</pkg>
> 
> ok.
> 
> >  		</pkg-list>
> >  		<preseed>
> >  			<conf owner="pbuilder" key="pbuilder/mirrorsite" type="string" value="http://ftp.de.debian.org/debian"/>
> > diff --git a/elbepack/init/preseed.cfg.mako b/elbepack/init/preseed.cfg.mako
> > index 086ad849..80ebcfbd 100644
> > --- a/elbepack/init/preseed.cfg.mako
> > +++ b/elbepack/init/preseed.cfg.mako
> > @@ -106,14 +106,33 @@ d-i finish-install/reboot_in_progress note
> >  d-i pkgsel/include string rng-tools btrfs-tools openssh-client \
> >  debathena-transform-lighttpd \
> >  elbe-soap elbe-buildenv qemu-elbe-user-static \
> > +## add extra packages specified in initvm xml
> >  % for n in pkgs:
> > -% if n.tag == "pkg":
> > -%   if prj.has("mirror/primary_host") or not prj.node("mirror/cdrom") or n.et.text.find('/') == -1:
> > -${n.et.text} \
> > -%   else:
> > -${n.et.text[:n.et.text.find('/')]} \
> > +%   if n.tag == "pkg":
> > +%     if not prj.has("mirror/primary_host") and n.et.text.find('/') > 0:
> > +##      Installing initvm from bin-cdrom.iso and a target release was
> > +##      specified in the element text. This is how target releases were
> > +##      specified for elbe 2.x. The target release must be stripped
> > +##      because it is not included as a separate release in the
> > +##      bin-cdrom.iso repository. The desired package is available in
> > +##      the main release of the bin-cdrom.iso repository.
> > +${n.et.text[:n.et.text.find('/')]}\
> 
> i really hate this line. it pretty unclear what it does.
> and it actually requires, that n.et.text contains '/'
> 
> n.et.text.split ('/')[0] works in any case.
> 
> anyways... this code is too complex for my taste to be sitting inline.
> it should be isolated into a function. which then allows much better
> comments.
> 
> 
> %<
> 	def pkg2preseed (n):
> 		# we have a set of old elbe files, which 
> 		# have pkgnames like:
> 		# pkgname/jessie-backports
> 		# be backwards compatible, and support them
> 		pkgsplit = n.et.text.split ('/')
> 
> 		pkgname = pkgsplit[0]
> 
> 		if len (pkgsplit) > 1:
> 			pkgrel = pkgsplit[1]
> 		else:
> 			pkgrel = None
> 
> 
> 		# pkg pin attrib overrides /
> 		if 'pin' in n.et.attrib:
> 			pkgrel =  n.et.attrib['pin']
> 
> 		# pkg attrib version wins over all, and it can also be
> 		# used with cdrom build.
> 		if 'version' in n.et.attrib:
> 			return pkgname + '=' + n.et.attrib['version']
> 
> 		# for a cdrom build, the pkgrel is resetted to None,
> 		# because the cdrom does not have the release
> 		# information anymore.
> 		if not prj.has("mirror/primary_host"):
> 			pkgrel = None
> 
> 		if pkgrel is None:
> 			return pkgname
> 		
> 
> 		return pkgname + '/' + pkgrel
> %>
> 
> 
> then we just end up with this again:
> 
> % for n in pkgs:
> %  if n.tag == "pkg":
> 	${pkg2preseed (n)} \
> %  endif
> % endfor
> 
> 
> its crucial, that we dont accidentally insert empty lines in 
> a loop that is building up a single line using escaped newlines.
> 
> 			
> 
> 
> > +%     else:
> > +${n.et.text}\
> > +%     endif
> > +##
> > +%     if prj.has("mirror/primary_host"):
> > +##      When not installing initvm from bin-cdrom.iso, add a specified
> > +##      version _or_ target release using apt syntax. Version has
> > +##      precedence.
> > +%       if n.et.attrib.has_key("version"):
> > +=${n.et.attrib["version"]}\
> > +%       elif n.et.attrib.has_key("pin"):
> > +/${n.et.attrib["pin"]}\
> > +%       endif
> > +%     endif
> > + \
> >  %   endif
> > -% endif
> >  % endfor
> >  
> >  passwd passwd/root-password password root
> > diff --git a/elbepack/makofiles/preferences.mako b/elbepack/makofiles/preferences.mako
> > index 0ed2af8b..3c698da5 100644
> > --- a/elbepack/makofiles/preferences.mako
> > +++ b/elbepack/makofiles/preferences.mako
> > @@ -39,7 +39,7 @@ Pin-Priority: ${porg['pin']}
> >  % for n in pkgs:
> >  %  if "pin" in n.et.attrib.keys():
> >  Package: ${n.et.text}
> > -Pin: release a=${n.et.attrib["pin"]}
> > +Pin: release n=${n.et.attrib["pin"]}
> >  Pin-Priority: 991
> >  
> >  %  endif
> > 
> > Thoughts?

I'm fine with that direction, please send a clean patch.

I'll drop the original patch from my V2 queue.