[elbe-devel] [PATCH 1/3] elbeproject: split build_cdroms() out of build()

[Manuel Traut]

Hi John,

On Fri, Jul 20, 2018 at 03:21:31PM +0200, John Ogness wrote:
> Hi Manuel,
> 
> It would have been helpful if this patch was split into 2 patches. The
> first one pulling the build cdrom code into the new build_cdroms() (but
> without adding any new functionality), then a second patch adding the
> new functionality. It makes it much easier to review.

Oops, sorry. This happened by accident not by purpose.

> Comments inline...
> 
> On 2018-07-12, Manuel Traut <manut at linutronix.de> wrote:
> > this allows building cdroms at different stages in the future.
> > e.g. after building the sysroot, to include the dev packages
> > on the cdroms.
> >
> > Signed-off-by: Manuel Traut <manut at linutronix.de>
> > ---
> >  elbepack/elbeproject.py | 69 +++++++++++++++++++++++++----------------
> >  1 file changed, 43 insertions(+), 26 deletions(-)
> >
> > diff --git a/elbepack/elbeproject.py b/elbepack/elbeproject.py
> > index 6a8090ff..a50b47c2 100644
> > --- a/elbepack/elbeproject.py
> > +++ b/elbepack/elbeproject.py
> > @@ -349,6 +349,48 @@ class ElbeProject (object):
> >          # pdebuild_build(-1) means use all cpus
> >          self.pdebuild_build(cpuset=-1)
> >  
> > +
> > +    def build_cdroms(self, build_bin=True, build_sources=False, cdrom_size=None):
> > +        self.repo_images = []
> > +
> > +        elog = ASCIIDocLog(self.validationpath, True)
> > +        elog.h1("Source CD")
> 
> Since this function builds binary and source CD's, I would expect this
> new log entry to be added for each of the CD's. I inserted the lines
> where I think it is more appropriate.

Absolutely right. I'll send a v2. Thanks!

> > +
> > +        env = None
> > +        if os.path.exists(self.sysrootpath):
> > +            env = BuildEnv(self.xml, self.log, self.sysrootpath,
> > +                    build_sources=build_sources, clean=False)
> > +        else:
> > +            env = BuildEnv(self.xml, self.log, self.chrootpath,
> > +                    build_sources=build_sources, clean=False)
> > +
> > +        with env:
> > +            init_codename = self.xml.get_initvm_codename()
> > +
> > +
> > +            if build_bin:
> 
>                    elog.h1("Binary CD")
> 
> > +                self.repo_images += mk_binary_cdrom(env.rfs,
> > +                                                    self.arch,
> > +                                                    self.codename,
> > +                                                    init_codename,
> > +                                                    self.xml,
> > +                                                    self.builddir,
> > +                                                    self.log,
> > +                                                    cdrom_size=cdrom_size)
> > +            if build_sources:
> 
>                    if os.path.exists(self.sysrootpath):
>                        elog.h1("Source CD (including sysroot packages)")
>                    else:
>                        elog.h1("Source CD")
> 
> > +                try:
> > +                    self.repo_images += mk_source_cdrom(env.rfs,
> > +                                                        self.arch,
> > +                                                        self.codename,
> > +                                                        init_codename,
> > +                                                        self.builddir,
> > +                                                        self.log,
> > +                                                        cdrom_size=cdrom_size,
> > +                                                        xml=self.xml)
> > +                except SystemError as e:
> > +                    # e.g. no deb-src urls specified
> > +                    elog.printo(str(e))
> > +
> >      def build(self, skip_debootstrap=False, build_bin=False,
> >                build_sources=False, cdrom_size=None, debug=False,
> >                skip_pkglist=False, skip_pbuild=False):
> > @@ -504,32 +546,7 @@ class ElbeProject (object):
> >              grub_fw_type = ""
> >          self.targetfs.part_target(self.builddir, grub_version, grub_fw_type)
> >  
> > -        # Build cdrom images
> > -        self.repo_images = []
> > -        with self.buildenv:
> > -            init_codename = self.xml.get_initvm_codename()
> > -            if build_bin:
> > -                self.repo_images += mk_binary_cdrom(self.buildenv.rfs,
> > -                                                    self.arch,
> > -                                                    self.codename,
> > -                                                    init_codename,
> > -                                                    self.xml,
> > -                                                    self.builddir,
> > -                                                    self.log,
> > -                                                    cdrom_size=cdrom_size)
> > -            if build_sources:
> > -                try:
> > -                    self.repo_images += mk_source_cdrom(self.buildenv.rfs,
> > -                                                        self.arch,
> > -                                                        self.codename,
> > -                                                        init_codename,
> > -                                                        self.builddir,
> > -                                                        self.log,
> > -                                                        cdrom_size=cdrom_size,
> > -                                                        xml=self.xml)
> > -                except SystemError as e:
> > -                    # e.g. no deb-src urls specified
> > -                    self.log.printo(str(e))
> > +        self.build_cdroms(build_bin, build_sources, cdrom_size)
> >  
> >          if self.postbuild_file:
> >              self.log.h2("postbuild script:")