Code review comment for lp://qastaging/~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball

On Tue, May 10, 2016 at 8:36 PM, Ming Lei <email address hidden> wrote:

> Hi Steve,
>
> On Wed, May 11, 2016 at 2:06 AM, Steve Langasek
> <email address hidden> wrote:
> > On Tue, May 10, 2016 at 02:08:20AM -0000, Ming Lei wrote:
> >> > === added directory 'build/boot/arm64'
> >> > === added directory 'build/boot/arm64/grub'
> >> > === added file 'build/boot/arm64/grub/grub-efi.cfg'
> >> > --- build/boot/arm64/grub/grub-efi.cfg 1970-01-01 00:00:00 +0000
> >> > +++ build/boot/arm64/grub/grub-efi.cfg 2016-04-02 15:32:40 +0000
> >> > @@ -0,0 +1,7 @@
> >> > +set menu_color_normal=cyan/blue
> >> > +set menu_color_highlight=white/blue
> >> > +menuentry 'Install' {
> >> > + set background_color=black
> >> > + linux /ubuntu-installer/arm64/linux --- quiet
> >>
> >> Given the change is only for trusty, I suggest to make things simple by
> this way, otherwise we have to
> >> introduce TEMP_SYSLINUX on arm64 for this substituation.
> >
> > I prefer a slightly larger backport now, to make the code as similar as
> > possible to xenial to make other backports easier later.
>
> Since it is SRU, I'd rather to make it simple, and don't consider too much
> about future things, maybe there will never be such kind of backport which
> depends on bootvars-subst for trusty.
>
> Also for trusty, only grub netboot is started to be supported by this
> patch, and the path is fixed now, so looks not necessary to introduce extra
> TEMP_SYSLINUX for trusty.
>
> That is also the way suggested by Adam Conrad and Dann.
>
> >
> >> > + initrd /ubuntu-installer/arm64/initrd.gz
> >> > +}
> >> >
> >> > === modified file 'build/config/arm64/generic/netboot.cfg'
> >> > --- build/config/arm64/generic/netboot.cfg 2015-06-02 20:49:49 +0000
> >> > +++ build/config/arm64/generic/netboot.cfg 2016-04-02 15:32:40 +0000
> >> > @@ -1,11 +1,19 @@
> >> > MEDIA_TYPE = netboot image
> >> > -TARGET = $(TEMP_INITRD) $(TEMP_KERNEL) all-generic
> >> > +TARGET = $(TEMP_INITRD) $(TEMP_KERNEL) $(NETBOOT_DIR) $(NETBOOT_TAR)
> all-generic
> >> > EXTRANAME = $(MEDIUM)/
> >> > INITRD_FS = initramfs
> >> >
> >> > MANIFEST-INITRD = "netboot initrd"
> >> > MANIFEST-KERNEL = "kernel image to netboot"
> >> >
> >> > +NETBOOT_DIR_TARGETS = $(TEMP_INITRD) $(TEMP_KERNEL)
> >> > +MANIFEST-NETBOOT_DIR = "PXE boot directory for tftp server"
> >> > +MANIFEST-NETBOOT_TAR = "tarball of PXE boot directory"
> >> > +GRUB_EFI=y
> >
> >> Looks they aren't set in build/config/arm64.cfg of
> lp:~ubuntu-core-dev/debian-installer/trusty-proposed.
> >> so could you share me what the trunk you are refering to is?
> >
> > I'm referring to lp:~ubuntu-core-dev/debian-installer/ubuntu/
> >
> > i.e. these changes are done differently in xenial than trusty, and there
> > doesn't seem to be a reason for it.
>
> OK, I will keep it as so since it is trusty only, :-)
>
> I will create a new merge propasal for review.
>
> Thanks,
> Ming
>

Hey Steve,
Ming created a new MP per above at
https://code.launchpad.net/~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball-v1
 .

Can you please help review ?

Thanks Much,
Raghu

>
> >
> >
> > Thanks,
> > --
> > Steve Langasek Give me a lever long enough and a Free
> OS
> > Debian Developer to set it on, and I can move the
> world.
> > Ubuntu Developer
> http://www.debian.org/
> > <email address hidden>
> <email address hidden>
> >
> >
> https://code.launchpad.net/~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball/+merge/290798
> > You are the owner of
> lp:~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball.
>
> --
>
> https://code.launchpad.net/~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball/+merge/290798
> You are subscribed to branch
> lp:~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball.
>