Merge lp://staging/~tom-leiming/debian-installer/trusty-for-generating-netboot-tarball into lp://staging/~ubuntu-core-dev/debian-installer/trusty-proposed

Hi Guys,

Could you give a review on this patch?

Thanks,

Comments inline. Thanks for taking this on!

Hi Steve,

Thanks for your review!

I have replied inline.

Thanks,
Ming

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.

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

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>

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

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

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