Merge ~paelzer/ubuntu/+source/ipxe:merge-ipxe-groovy-36a4c85-5 into ubuntu/+source/ipxe:debian/sid

Proposed by Christian Ehrhardt 
Status: Merged
Approved by: Christian Ehrhardt 
Approved revision: 1c10dbf0f174c2d1b64bbbcf753617f124c076ca
Merge reported by: Christian Ehrhardt 
Merged at revision: 1c10dbf0f174c2d1b64bbbcf753617f124c076ca
Proposed branch: ~paelzer/ubuntu/+source/ipxe:merge-ipxe-groovy-36a4c85-5
Merge into: ubuntu/+source/ipxe:debian/sid
Diff against target: 1711 lines (+1299/-148)
13 files modified
debian/changelog (+443/-0)
debian/control (+15/-2)
debian/copyright (+560/-136)
debian/grub-ipxe.install (+3/-0)
debian/grub-ipxe.postinst (+1/-1)
debian/grub-ipxe.postrm (+3/-3)
debian/ipxe.install (+0/-3)
debian/patches/0005-strip-802.1Q-VLAN-0-priority-tags.patch (+121/-0)
debian/patches/handle-dhcp-nack.patch (+43/-0)
debian/patches/series (+2/-0)
debian/rules (+27/-1)
debian/tree/ipxe/etc/grub.d/20_ipxe (+15/-2)
debian/util/check-rom-sizes (+66/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack Approve
Canonical Server Pending
git-ubuntu developers Pending
Review via email: mp+386372@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Download full text (3.8 KiB)

PPA: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4116/+packages

FYI I've had some debug enabled to ensure the non EFI roms kept https enabled:
70398 grep -H __feature_https src/bin-i386-pcbios/*.tmp.map src/bin-x86_64-efi/*drv*.map || /bin/true
70399 src/bin-i386-pcbios/808610d3.rom.tmp.map: 0x0000000000027b10 __feature_https
70400 src/bin-i386-pcbios/808610d3.rom.tmp.map:00027b10 g O .textdata»00000004 .hidden __feature_https
70401 src/bin-i386-pcbios/82540em.rom.tmp.map: 0x0000000000027b10 __feature_https
70402 src/bin-i386-pcbios/82540em.rom.tmp.map:00027b10 g O .textdata»·00000004 .hidden __feature_https
70403 src/bin-i386-pcbios/eepro100.rom.tmp.map: 0x00000000000277fc __feature_https
70404 src/bin-i386-pcbios/eepro100.rom.tmp.map:000277fc g O .textdata»00000004 .hidden __feature_https
70405 src/bin-i386-pcbios/ipxe.lkrn.tmp.map: 0x00000000000bdfe0 __feature_https
70406 src/bin-i386-pcbios/ipxe.lkrn.tmp.map:000bdfe0 g O .textdata»···00000004 .hidden __feature_https
70407 src/bin-i386-pcbios/ipxe.pxe.tmp.map: 0x00000000000be100 __feature_https
70408 src/bin-i386-pcbios/ipxe.pxe.tmp.map:000be100 g O .textdata»00000004 .hidden __feature_https
70409 src/bin-i386-pcbios/ns8390.rom.tmp.map: 0x0000000000027330 __feature_https
70410 src/bin-i386-pcbios/ns8390.rom.tmp.map:00027330 g O .textdata»··00000004 .hidden __feature_https
70411 src/bin-i386-pcbios/pcnet32.rom.tmp.map: 0x0000000000027490 __feature_https
70412 src/bin-i386-pcbios/pcnet32.rom.tmp.map:00027490 g O .textdata»·00000004 .hidden __feature_https
70413 src/bin-i386-pcbios/rtl8139.rom.tmp.map: 0x0000000000027c50 __feature_https
70414 src/bin-i386-pcbios/rtl8139.rom.tmp.map:00027c50 g O .textdata»·00000004 .hidden __feature_https
70415 src/bin-i386-pcbios/undionly.kkpxe.tmp.map: 0x0000000000027190 __feature_https
70416 src/bin-i386-pcbios/undionly.kkpxe.tmp.map:00027190 g O .textdata»··00000004 .hidden __feature_https
70417 src/bin-i386-pcbios/undionly.kpxe.tmp.map: 0x0000000000027190 __feature_https
70418 src/bin-i386-pcbios/undionly.kpxe.tmp.map:00027190 g O .textdata»···00000004 .hidden __feature_https
70419 src/bin-i386-pcbios/virtio-net.rom.tmp.map: 0x0000000000027bd0 __feature_https
70420 src/bin-i386-pcbios/virtio-net.rom.tmp.map:00027bd0 g O .textdata»··00000004 .hidden __feature_https
70421 src/bin-i386-pcbios/vmxnet3.rom.tmp.map: 0x0000000000026f10 __feature_https
70422 src/bin-i386-pcbios/vmxnet3.rom.tmp.map:00026f10 g O .textdata»·00000004 .hidden __feature_https
70423 dh override_dh_auto_build

That is exactly as intended in the bug.

The rest is a merge that is somewhat complex due to us being ahead for 2 years and thereby gathering a bunch of Delta. A lot can be dropped now - it just is a long squashing session from split to logical.

To help understan...

Read more...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Note: It might be very useful to push this to groovy soon (as part of the merge) and continue the SRU after it has stayed there for a while without negative reports (maybe after my PTO, where I also can better fix up any mess that I might cause).

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI: I checked one more thing with the maintainer - https://bugs.launchpad.net/ipxe/+bug/1882671/comments/22
 about the toggling of config=QEMU

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ok, will do this in several steps.

First bit:

This commit is missing the mention of d/control:
commit f52d57afafbd9d2de5905263d701f87b374194dc
Author: Christian Ehrhardt <email address hidden>
Date: Tue Aug 29 15:11:37 2017 +0200

    d/util/check-rom-sizes, d/rules: check sizes of generated roms (LP: #1713490)

    Signed-off-by: Christian Ehrhardt <email address hidden>

diff --git a/debian/control b/debian/control
index d9b6e476..c85178bf 100644
--- a/debian/control
+++ b/debian/control
@@ -33,6 +33,7 @@ Package: ipxe-qemu
 Architecture: all
 Multi-Arch: foreign
 Depends: ${misc:Depends}
+Breaks: qemu-system-x86 (<< 1:2.11+dfsg-1ubuntu1~)
 Description: PXE boot firmware - ROM images for qemu
  iPXE is network boot firmware. It supports a variety of network cards,
  including some wireless cards, and variety of network protocols (traditional
....

From an older d/changelog, it's this probably: "- d/control: break older qemu versions to ensure the new sized roms"

Do we still need this breaks, btw?

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Since this is a complicated merge, I'm going through the logical in more detail. I think you missed a consolidation step (on purpose perhaps?) as there are commits adding patches and later one removing them:

2ae0e4801eadae1d064ef1c50cfc608ec200caea
DROP (Debian): drop patches no more needed with 20180124.fbe8c52d
This drops:
0002-Don-t-use-libiberty.patch
0004-fix_no-pie_option.patch
0006-build-Fix-.ids.o-creation-for-drivers-not-in-the-all.patch
0007-build-Remove-nested-my-declaration.patch

Which are indeed not in pkg/ubuntu/devel at the moment. But they were added by these commits also inside the logical chunk:
60ca2e05 0002-Don-t-use-libiberty.patch
d4ca5cdc 0004-fix_no-pie_option.patch
c9ce6c19 0006 and 0007 patches

So, since the logical (and I'm talking about lp1884758/old/debian..lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3) adds them and removes them later on, couldn't these just be removed from logical entirely, and not just "scheduled" for removal when you apply this on top of new/debian?

review: Needs Information
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

If yes, with less commits, this would make the merge simpler, and the range-diff output also simples.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Basic regression tests look good (the one error is known until qemu 5.5 lands)

prep (x86_64) : Pass 25 F/S/N 0/0/0 - RC 0 (18 min 68875 lin)
migrate (x86_64) : Pass 575 F/S/N 1/0/0 - RC 1 (167 min 442854 lin)
cross (x86_64) : Pass 52 F/S/N 0/1/3 - RC 0 (105 min 93386 lin)
misc (x86_64) : Pass 146 F/S/N 0/0/0 - RC 0 (71 min 82562 lin)

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The non mentioning of d/control in f52d57afafbd9d2de5905263d701f87b374194dc is a good catch.
In fact the CL is right about what "should happen". This is a delta that can be dropped after Focal.
I've modified the commit and adapted the changelog.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

pushed the updated branch

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

@andreas about the addition&removal of patches like 0002-Don-t-use-libiberty.patch
This was due to the "going ahead of Debian". In fact at some point we reverted to go onto an earlier version, this made these patches "again being required" and later on when moving forward got removed again.

This wasn't exactly the same as when we usually add/remove patches. And I thought the history is more readable the way I presented them.

If you want I can squash them away.
I have made the former logical tag -v6 and pushed a new logical tag.

P.S. That is the reason there are multiple logical..-v* tags in case you ever wonder how I came from split to the final --logical (to be able to trace the steps).

This squashed the following commits into a no-op:
  d4ca5cdc4 DROP (upsteram): - d/p/0004-fix_no-pie_option.patch: correct -no-pie option to build without pie
  60ca2e055 DROP (Debian) - This brings back debian/patches/0002-Don-t-use-libiberty.patch as needed on the older source.
  c9ce6c192 DROP (upstream): fix FTBFS with newer perl versions
  2ae0e4801 DROP (Debian): drop patches no more needed with 20180124.fbe8c52d

The updated lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3 should match what you have expected.

To ssh://git.launchpad.net/~paelzer/ubuntu/+source/ipxe
 * [new tag] lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3-v6 -> lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3-v6
 + 6ced0f108...a0fbb52a7 lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3 -> lp1884758/logical/1.0.0+git-20190109.133f4c4-0ubuntu3 (forced update)

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The discussion on the HTTPS/config enabling/disabling clarified that we don't need to change the config=qemu behavior on legacy roms.

I adapted the changelog and the former
commit 8227ee968a9480153c0419fc15a92dc716826712
Author: Christian Ehrhardt <email address hidden>
Date: Wed Jun 24 13:24:30 2020 +0200

    - d/rules: only enable https on non EFI roms and CONFIG=qemu only on EFI
      roms. This lets EFI handle https itself and avoids breakage in TPL
      manipulations (LP: #1882671)

I force pushed the branch to reflect this

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Thanks for -v6, it made the range-diff easier to read and follow. I saw the version revert, and patches coming back, and later being dropped, and I think it's a perfectly valid case for removing the commits, as consolidating the logical is one of the objectives of that step. I understand that it's quite the squashing, and might make a reviewer wonder about what happened. When I do something like that, I usually mention it in detail in the MP. It's just a matter of preference, and I used your -v6 tag for this review.

Continuing...

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Having checked out the merge branch, with all our changes on top of new/debian, I see that the enable-https patch, from this commit (first one on top of new/debian):

commit 2fa82ccd07a6f1360bc357cf1a45490e347c35c0
Author: Christian Ehrhardt <email address hidden>
Date: Thu Aug 17 08:28:19 2017 +0200

    - d/p/enable-https.patch: Enable HTTPS support.

Is being removed in this commit:
commit 50a9def24c715aa5e44800c2068e43511d14238f
Author: Christian Ehrhardt <email address hidden>
Date: Wed Jun 24 13:24:30 2020 +0200

    - d/rules: only enable https on non EFI roms. This lets EFI handle https
      itself and avoids breakage in TPL manipulations (LP: #1882671)

Two things:

a) the 50a9def24c715aa5e44800c2068e43511d14238f commit is not mentioning that the enable-https.patch patch is being removed

b) d/changelog still lists the enable-https.patch patch as being part of the delta:
  * Merge with Debian unstable (LP: #1884758). Remaining changes:
    - d/p/enable-https.patch: Enable HTTPS support.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks, I amended the change-log to properly mention it being dropped including the reason.
And I reworded the commit that removes it to mention as well.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Ok, very big changes, I'm afraid that's all I can check, that the delta was carried with the changes you said you did, and nothing else stands out to me. Thanks for tackling this, should be less hard the next time!

review: Approve
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

"should be less hard the next time"
- Absolutely-

Thanks for suffering with me :-)

To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/ipxe
 * [new tag] upload/1.0.0+git-20190125.36a4c85-5ubuntu1 -> upload/1.0.0+git-20190125.36a4c85-5ubuntu1

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading ipxe_1.0.0+git-20190125.36a4c85-5ubuntu1.dsc: done.
  Uploading ipxe_1.0.0+git-20190125.36a4c85.orig.tar.xz: done.
  Uploading ipxe_1.0.0+git-20190125.36a4c85-5ubuntu1.debian.tar.xz: done.
  Uploading ipxe_1.0.0+git-20190125.36a4c85-5ubuntu1_source.buildinfo: done.
  Uploading ipxe_1.0.0+git-20190125.36a4c85-5ubuntu1_source.changes: done.
Successfully uploaded packages.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

merged

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches