[edk2-devel] [PATCH 1/1] Add BhyvePkg, to support the bhyve hypervisor

[Laszlo Ersek]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63598): https://edk2.groups.io/g/devel/message/63598
Mute This Topic: https://groups.io/mt/75472166/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-