From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.4290.1596230057479677725 for ; Fri, 31 Jul 2020 14:14:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AYZrrGpS; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596230056; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3ha66JVX7DO91uwjyqImpr6WZkt3bjLWb+eFmGAcJKc=; b=AYZrrGpS4QMpy+VTdEiu82FIlEpR5CM0C3rsZqfQT7DHg+WJRAg5c0sxOd/JCRUvGxawCy YwDMRGD381yjJQDyzqUWbynERDh5UtgIdtegZXJqLhxF0uApghD1e4hTCvdPl5DKQ4Z3W/ zJg1piyTJ78WlItJms9bnQhly+Yj8qs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-472-7sCRxdZuO9Kmf3opxK0chA-1; Fri, 31 Jul 2020 17:14:11 -0400 X-MC-Unique: 7sCRxdZuO9Kmf3opxK0chA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3E7D08017FB; Fri, 31 Jul 2020 21:14:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-58.ams2.redhat.com [10.36.112.58]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3392D1002388; Fri, 31 Jul 2020 21:14:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/1] Add BhyvePkg, to support the bhyve hypervisor To: Sean Brogan , rebecca@bsdio.com, Andrew Fish , Leif Lindholm , Michael D Kinney Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan L Justen , Liming Gao References: <20200713054131.479627-1-rebecca@bsdio.com> <20200713054131.479627-2-rebecca@bsdio.com> <88d22f36-c210-f626-fe70-b2eb3c5fcd3c@redhat.com> <162169B0F92549B8.20689@groups.io> <52be7a58-9e2b-92a0-5f15-22a9dd59b0e4@redhat.com> <48628363-3c1b-7d24-6dbc-95e739ac389d@redhat.com> From: "Laszlo Ersek" Message-ID: <7d432e20-b410-5510-ac29-9396f7ebf982@redhat.com> Date: Fri, 31 Jul 2020 23:14:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 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