From: "Laszlo Ersek" <lersek@redhat.com>
To: Sean Brogan <spbrogan@outlook.com>,
rebecca@bsdio.com, Andrew Fish <afish@apple.com>,
Leif Lindholm <leif@nuviainc.com>,
Michael D Kinney <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io, Ard Biesheuvel <ard.biesheuvel@arm.com>,
Jordan L Justen <jordan.l.justen@intel.com>,
Liming Gao <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] Add BhyvePkg, to support the bhyve hypervisor
Date: Fri, 31 Jul 2020 23:14:04 +0200 [thread overview]
Message-ID: <7d432e20-b410-5510-ac29-9396f7ebf982@redhat.com> (raw)
In-Reply-To: <BN8PR07MB6962FCCA9AB09F8A6DCCED69C84E0@BN8PR07MB6962.namprd07.prod.outlook.com>
Hi Sean,
thank you for reporting this. I apologize for the breakage. Please see
my comments below.
(Rebecca and the stewards should read on as well, please.)
On 07/31/20 19:32, Sean Brogan wrote:
> This patch as committed is breaking CI. It was not captured in PR
> because the PR optimizes to detect packages impacted by the commits and
> the BhyvePkg addition is not depended on by other packages (that are in
> CI).
Yup, both Rebecca and myself put the package through CI.
> BhyvePkg which is nested inside OvmfPkg ( a violation of DEC spec:
> see
> https://edk2-docs.gitbook.io/edk-ii-dec-specification/2_dec_file_overview paragraph
Indeed! I've been unaware of this:
> An EDK II Package (directory) is a directory that contains an EDK II
> package declaration (DEC) file. Only one DEC file is permitted per
> directory. EDK II Packages cannot be nested within other EDK II
> Packages.
Thank you for teaching me this. This is the first time in my 8-9-ish
years with edk2 that I'm participating in the addition of a (not-quite)
top-level package.
We originally intended BhyvePkg to be stand-alone, but that was not
welcomed by many. So we pushed it into OvmfPkg/Bhyve (note: no "Pkg"
suffix on the "Bhyve" subdirectory). While doing so, we should have
eliminated the separate DEC file.
Namely, if we compare both DEC files now:
- OvmfPkg/OvmfPkg.dec
- OvmfPkg/Bhyve/BhyvePkg.dec
there are not many differences (in fact only changes and additions in
BhyvePkg.dec matter, removals (= trimming) don't):
- some copyright notices,
- the BhyveFwCtlLib lib class,
- a different default value for PcdDebugIoPort.
That's all. So it should not be hard for Rebecca to incorporate these
changes into the "main" OvmfPkg/OvmfPkg.dec file. And, the
PcdDebugIoPort value change actually belongs in
"OvmfPkg/Bhyve/BhyvePkgX64.dsc".
Furthermore, the new Bhyve DEC file -- which should be removed -- is
referenced in the following INF files only:
- Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf
- Bhyve/BhyveRfbDxe/BhyveRfbDxe.inf
- Bhyve/Library/BhyveFwCtlLib/BhyveFwCtlLib.inf
Those INF files should be re-pointed to the main OvmfPkg.dec file.
> 4) does not support CI so it is not tested but now that it is in the
> edk2 tree it is causing the other packages to fail.
>
>
> You can see the ReadMe badge showing the broken state of edk2 master.
> The build with logs can be seen here
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=10494&view=logs&j=ec42d809-3c3b-54a9-276c-e54a8b9aaee9&t=596e0656-4def-5804-b10b-1585519aa2e8
> and some of the relevant failures are added below.
>
> [...]
>
> The errors can be easily resolved but the nested packages is a bigger
> problem.
Both the individual errors and the nested package situation (which
indeed violates the DEC spec) should hopefully be resolved by the
suggestions above.
Regarding actual actions: I'm going to be away for a short while now.
Plus, I'm not entirely sure what exactly is being prevented by the
current state of the tree (i.e., how grave the regression is).
(1) If the current issue interferes with work on, and usability of,
other packages (that is, anything *not* OvmfPkg), then I would request
that one of the stewards please revert 656419f922c0 ("Add BhyvePkg, to
support the bhyve hypervisor", 2020-07-31). For such a revert, please
add at once:
Acked-by: Laszlo Ersek <lersek@redhat.com>
This is because the IRL stuff I've got queued up does not allow me to
participate in the revert, urgently, either from the reviewer side, or
even from the submitter side. (I wouldn't like to simply push a revert
without formal review, and I don't have time to *post* the revert
urgently). I was about to disappear for a bit, and logged back in only
because I snuck a peek on the mailing list archive, and noticed the
problem report.
After the revert, Rebecca and I can collaborate on the next version of
the patch (I can review that incrementally against the one being
reverted under this option).
(2) If, on the other hand, the current issue is restricted to OvmfPkg
(and even OvmfPkg platforms other than bhyve can be built), then I'd
like to ask that we keep commit 656419f922c0, and that Rebecca please
submit an incremental fix (per the above suggestions, assuming they work).
... Upon re-reading your comment "causing the other packages to fail", I
think we have case (1); if that's right, then please proceed accordingly.
Thank you, and again I apologize for the mess. :(
Laszlo
next prev parent reply other threads:[~2020-07-31 21:14 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-13 5:41 [PATCH 0/1] Add BhyvePkg, to support the bhyve hypervisor Rebecca Cran
2020-07-13 5:41 ` [PATCH 1/1] " Rebecca Cran
2020-07-13 16:16 ` [edk2-devel] " Laszlo Ersek
2020-07-13 20:24 ` Rebecca Cran
2020-07-13 16:55 ` Laszlo Ersek
2020-07-13 18:09 ` Laszlo Ersek
2020-07-13 20:25 ` Rebecca Cran
[not found] ` <162169B0F92549B8.20689@groups.io>
2020-07-31 5:17 ` Rebecca Cran
2020-07-31 13:07 ` Laszlo Ersek
2020-07-31 13:10 ` Laszlo Ersek
2020-07-31 17:32 ` Sean
2020-07-31 20:28 ` Rebecca Cran
2020-07-31 21:14 ` Laszlo Ersek [this message]
2020-08-01 15:54 ` Laszlo Ersek
2020-08-01 21:10 ` Laszlo Ersek
2020-07-13 5:43 ` [PATCH 0/1] " Liming Gao
2020-07-13 5:48 ` Rebecca Cran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7d432e20-b410-5510-ac29-9396f7ebf982@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox